replace nose with pytest

KrisThielemans commented 1 year ago

Still needs further work on test_algebra.py

Related issues

fixes #1148

Checklist before requesting a review

[x] The code builds and runs on my machine
[x] CHANGES.md has been updated with any functionality change

Contribution Notes

Please tick the following:

[x] The content of this Pull Request (the Contribution) is intentionally submitted for inclusion in SIRF (the Work) under the terms and conditions of the Apache-2.0 License.

KrisThielemans commented 1 year ago

@paskino @evgueni-ovtchinnikov with this you can test most Python tests (not the demo ones) by doing (after SIRF installation)

cd src/Registration/pReg/tests
python3 -m pytest

(the normal ctest stuff should work as well). Added the -v option to see what it's doing, and the -s option to get all output.

There are 2 remaining problems:

I tried to skip TestDataContainerAlgebra but now it skips the derived classes (such as TestGadgetronAcquisitionDataAlgebra as well

pReg tests give me the following warnings 4 times after doing everything

Exception ignored in: <bound method NiftiImageData3D.__del__ of <sirf.Reg.NiftiImageData3D object at 0x7ffbb5109e80>>
Traceback (most recent call last):
File "/home/sirfuser/devel/install/python/sirf/Reg.py", line 591, in __del__
TypeError: 'NoneType' object is not callable

I don't get them with python3 -m nose test_pReg.py -v. Still, I guess this isn't a pytest problem but a SIRF problem.

KrisThielemans commented 1 year ago

errors like this

_______ ERROR collecting src/xGadgetron/pGadgetron/tests/test_algebra.py _______
import file mismatch:
imported module 'test_algebra' has this __file__ attribute:
  /home/runner/work/SIRF/SIRF/src/Registration/pReg/tests/test_algebra.py
which is not the same as the test file we want to collect:
  /home/runner/work/SIRF/SIRF/src/xGadgetron/pGadgetron/tests/test_algebra.py
HINT: remove __pycache__ / .pyc files and/or use a unique basename for your test file modules

It seems to get confused between different directories. No idea why...

KrisThielemans commented 1 year ago

It seems to get confused between different directories. No idea why...

Answer is at https://stackoverflow.com/a/59446854/15030207. It seems we need to give test_algebra.py different names.

KrisThielemans commented 1 year ago

It seems to get confused between different directories. No idea why...

Answer is at https://stackoverflow.com/a/59446854/15030207. It seems we need to give test_algebra.py different names.

The confusion was that pytest was run in CMAKE_SOURCE_DIR (i.e. .../SIRF) and not CMAKE_SOURCE_DIR. Hence, the conflict. However, I'm now running them from CMAKE_SOURCE_DIR anyway by specifying paths (as we did for `nose).

Being optimistic, I tried to also revive the coverage report. I did this letting ctest store the output for each module in a separate .coverage-* file, followed by a coverage combine. I've also used (new-ish) coverage capabilities to effectively rename directory paths to avoid the problem that coverage listed files in the installation directory. This resolves the problem that we had on Travis-ci, where we used sed to replace installation locations with source locations https://github.com/SyneRBI/SIRF/blob/cccd83dcc52c8d1107f94bfeba47c096d9eba5bb/.travis.yml#L263 but this failed as .coverage is an SQL database, so sed manipulations broke it.

So, now in our log output we have

Combined data file .coverage-Gadgetron
Combined data file .coverage-Reg
Combined data file .coverage-STIR
Name                                     Stmts   Miss  Cover
------------------------------------------------------------
src/Registration/pReg/Reg.py               850    212    75%
src/xGadgetron/pGadgetron/Gadgetron.py    1085    668    38%
src/xSTIR/pSTIR/STIR.py                   1458    835    43%
------------------------------------------------------------
TOTAL                                     3393   1715    49%

So this works ok, although it does seem quite brittle.

KrisThielemans commented 1 year ago

I thought I'd try to re-enable uploads onto coveralls.io then (using https://github.com/AndreMiras/coveralls-python-action as recommended by several on https://github.com/coverallsapp/github-action/issues/4), but that is failing. We do get an upload (at https://coveralls.io/builds/54589585) but it is empty. Checking the output over the first step

cd .
No source for /home/runner/work/SIRF/SIRF/src/Registration/pReg/Reg.py
No source for /home/runner/work/SIRF/SIRF/src/xGadgetron/pGadgetron/Gadgetron.py
No source for /home/runner/work/SIRF/SIRF/src/xSTIR/pSTIR/STIR.py

Am I missing something obvious here? It does like the repo is in /home/runner/work/SIRF/SIRF/ (see also here).

I note that the doc of the action says to use relative_files=True, but I guess I'm effectively doing that, as also shown in the coverage report above.

@casperdcl do you have any suggestions?

KrisThielemans commented 1 year ago

In my own VM

cd ~/devel/SIRF
COVERALLS_REPO_TOKEN=<some stuff from coveralls.io> coveralls

worked fine.

KrisThielemans commented 1 year ago

more comments on the coverage struggles

the reason the coveralls action reported No source for /home/runner/work/SIRF/SIRF/src/Registration/pReg/Reg.py etc is that it runs in a docker container where /home/runner/work/SIRF/SIRF is mounted as /github/workflow, hence the file doesn't exist.
the next problem was my trick of our path in coveragerc-* meant that coverage does report the relative paths, even though it still stored the full path in the .coverage-* files.

So, yes, we need relative_files=True, but there are some problems in that pytest-cov says you then need to specify source in the .coveragerc as opposed to the command line, but that doesn't work here for some reason (it reports "module not imported"). So, still struggling.

KrisThielemans commented 1 year ago

Latest status on coverage (testing locally as well): coverage computed correctly for each subdir, e.g. output of the pytest runs

: ----------- coverage: platform linux, python 3.6.9-final-0 -----------
6: Name                           Stmts   Miss  Cover
6: --------------------------------------------------
6: src/Registration/pReg/Reg.py     838    212    75%
6: --------------------------------------------------
6: TOTAL                            838    212    75%
....
- ----------- coverage: platform linux, python 3.6.9-final-0 -----------
2: Name                                     Stmts   Miss  Cover
2: ------------------------------------------------------------
2: src/xGadgetron/pGadgetron/Gadgetron.py    1077    779    28%
2: ------------------------------------------------------------
2: TOTAL                                     1077    779    28%

No paths anymore in the .coverage-* files.

However, the output of coverage combine then gets rid of the coverage itself, e.g.

Name                                     Stmts   Miss  Cover
------------------------------------------------------------
src/Registration/pReg/Reg.py               838    838     0%
src/xGadgetron/pGadgetron/Gadgetron.py    1077   1077     0%
------------------------------------------------------------
TOTAL                                     1915   1915     0%

Sigh. no idea why this happens.

KrisThielemans commented 1 year ago

This "works" in the sense that all tests are run and ok. The final coverage step still reports 0% coverage, but I'll create an issue for that.

@paskino, @evgueni-ovtchinnikov I've addded a little bit of documentation in the DeveloperGuide.md. Please check.

However, I'll merge this now such that we can move ahead.

SyneRBI / SIRF