Transition to GitHub Workflow

[RobertRosca]

No description provided.

[RobertRosca]

Not too sure why changing to the workflows lowered coverage...?

[takluyver]

If you look on codecov (follow the 'Details' link) you can see where it thinks coverage has changed:

https://codecov.io/gh/European-XFEL/EXtra-geom/pull/34/changes

It looks like most of what's gone missing is plotting related. I would guess the coverage from running the notebooks is not being included somehow. Maybe the codecov Python package does something different from the codecov Github action to find coverage info?

[RobertRosca]

Ah yes that is possible, thinking about it more it's (maybe) the issue where some of the tests run by loading extra-datageom (edit: forgot what repo this was apparently) from a relative path in the package directory, and the notebook tests run by loading it from the installation directory under site-packages.

If I'm right then this wasn't an issue before as the dev-install step in the Makefile ran python3 -m pip install -U -e .[test], since it used the -e flag then both would be loading the module from the package directory and codecov wouldn't get confused.

Could use codecov.yml's path-fixing to solve this issue, or I could change the installation step to use pip install -e ".[test]", both would solve the issue. There's... some other fix as well which escapes me now. Do you have any other suggestions Thomas?

[takluyver]

You can also set something in the .coveragerc config file to tell it to consider certain paths equivalent: https://coverage.readthedocs.io/en/coverage-5.3/config.html#paths

I've got a vague memory this is tricky to get right, but I don't remember the details.

[RobertRosca]

Oh yes that's what I meant by codecov.yml, I'll use .coveragerc instead since that's a bit more generic.

[RobertRosca]

Spoke to @tmichela about this a bit, the main reason that these kind of issues happen is that the 'traditional' project layout of having the module defined in the root directory of the project.

This has, to me, very unexpected behaviour where running pip install . followed by pytest that the tests do not use the package you just installed with pip but instead imports it as a relative import. It causes the issues we were just discussing, and can also cause problems where your tests can pass even if you've not created the setup correctly (e.g. missing package_data, issues with build/compilation steps).

A while ago I started using the src layout to avoid these occasional problems. The move to a src layout is pretty simple (move extra_geom to src/extra_geom and add a line package_dir={"": "src"} to setup.py) and is, to me, a more 'correct' solution to this problem.

The coverage config path fix or using pip install -e . are just workaround for the real issue which is that the tests aren't actually running on the package as it would be when installed by a user.

What do you think @takluyver? Are you for, against, or indifferent to moving to a src layout for extra-data and extra-geom (and our other projects as well but these two would be the most important).

[takluyver]

Would codecov pick up any of the coverage if we're testing the installed version rather than the code in the repository? If it doesn't recognise that the installed files are equivalent to the source files, then it will just show that 0% of the source files were executed.

I switched it to an editable install a couple of years back to get the same thing working: European-XFEL/karabo_data#41 . I think I tried at the time to do it 'properly', with equivalent paths, and couldn't get it to work, so I took the easy option. But I can't remember exactly what I tried, so feel free to work it out.

[takluyver]

If this isn't a 5 minute fix, though, let's revert to an editable install for now so we can merge this PR.

[RobertRosca]

Yeah I just added the -e flag, coverage looks good now.

I'll do another PR to add constraints/dependabot support like for extra-data, and then try out the src layout later on.

[takluyver]

Thanks!