Spinalcordtoolbox: Changes in SCT testing procedures (Replacing usage of sct_testing with pytest)

[joshuacwnewton]

The topic of replacing sct_testing with pytest came up when discussing a recent PR, and has an impact on how future tests are written. See https://github.com/neuropoly/spinalcordtoolbox/pull/2891#issuecomment-690813279:

I think the idea is to progressively get rid of sct_testing, to be replaced exclusively by the more traditional pytest. @cfhammill started an example of using pytest for CLI with his test_sct_run_batch, so I used the same approach. I cannot find the discussion about this though...

I did a little digging and found this Slack thread. I couldn't find any other discussion about this, and the relevant GitHub Wiki section appears to be out of date. This seems like a pretty consequential topic (epic, even?) that should have some documentation, so I thought to make a GH issue to better record this. Once the details are agreed upon, I'm happy to formalize things in the Wiki, as well.

[jcohenadad]

Thank you so much for finding the slack conversation an for initiating this. Since not everybody can access the Slack channel, here is the retransription:

Julien 2 months ago

what do you say about the idea of progressively moving tests from testing/ (run by sct_testing) to unit_testing (run by pytest)? i'm currently dealing with https://github.com/neuropoly/spinalcordtoolbox/pull/2774 and this is a bit of a mess (there are duplicated tests, sometimes with different sensitivity, etc.).
So, in the long term, can we consider having pytest test for the CLI also (e.g. including system call, to catch errors in CLI arguments). If we do that, do you recommend a single test_scripts that will check all SCT scripts, or having the CLI test being part of the relevant test_ script (e.g., sct_deepseg_sc to test inside test_deepseg_sc.py)?

zougloub 2 months ago

I'd say something like test_cmd_${x}.py (tests command-line call) vs. test_${x}.py (tests API), and we can rename unit_testing/ to testing/ when there's nothing left in testing/. Of course someone has to do the dirty work of migrating the tests.

Nick Guenther 2 months ago

The convention from https://packaging.python.org/tutorials/packaging-projects/ is to name it tests/ , and inside to mirror the folder structure of the package, so each spinalcordtoolbox/subpkg/module.py has a tests/subpkg/test_module.py.

Nick Guenther 2 months ago

If we moved the scripts/ to spinalcordtoolbox/cli then we wouldn't need a special naming convention for their tests.

[Drulex]

This is definitely a good initiative. pytest provides a lot of useful infrastructure for testing and we should leverage that.

Some potentially actionable items:

• Update the wiki to deprecate the SCT function testing-compatible and instead indicate that tests should be written using the convention pytest and following the convention above.
• Migrating the current functional tests to pytest. I have nothing against keeping both sct_testing and pytest (while stopping development on sct_testing), however I have hope that we can minimally wrap them to avoid spending too much time on this since it already works.
• Investigate the current "performance" (for lack of a better word) of the unit tests. I am saying this because while implementing unit tests during my refactoring I noticed that the testing data is always re-downloaded on each pytest run which isn't optimal. I remember a discussion about this at some point, but more generally speaking it would be good to make an assessment of the current situation before everyone starts using it and we find later that something is wrong.

[joshuacwnewton]

Migrating the current functional tests to pytest. I have nothing against keeping both sct_testing and pytest (while stopping development on sct_testing), however I have hope that we can minimally wrap them to avoid spending too much time on this since it already works.

A plugin that may be of use here is pytest-console-scripts.

I can envision an analog to sct_testing.py that uses pytest and this plugin, while leaving the existing functional tests alone.

[jcohenadad]

A plugin that may be of use here is pytest-console-scripts.

I can envision an analog to sct_testing.py that uses pytest and this plugin, while leaving the existing functional tests alone.

that seems very appropriate indeed! ironically their CI is failing 🤣 (although to be fair this is a temporary glitch-- their travis is clean)

[Drulex]

A plugin that may be of use here is pytest-console-scripts.

Good find, this looks promising! I think I'm going to time box a half hour to evaluate it.

[Drulex]

I wrote a basic unit_testing/test_cli_deepseg_gm.py:

from pytest_console_scripts import script_runner
import pytest
import logging

logger = logging.getLogger(__name__)

@pytest.mark.script_launch_mode('subprocess')
def test_sct_deepseg_gm(script_runner):
    ret = script_runner.run('sct_testing', '--function',  'sct_deepseg_gm')
    logger.debug(f"{ret.stdout}")
    logger.debug(f"{ret.stderr}")
    assert ret.success
    assert "status: [0]" in ret.stdout
    assert ret.stderr == ''

And then ran it python -m pytest -v -o log_cli=True --log-level=DEBUG unit_testing/test_cli_deepseg_gm.py:

========================== test session starts ========================================
platform linux -- Python 3.8.5, pytest-5.3.5, py-1.8.1, pluggy-0.13.1 -- /usr/bin/python
cachedir: .pytest_cache
rootdir: /home/drulex/work/exmakhina/neuropoly/spinalcordtoolbox, inifile: setup.cfg
plugins: console-scripts-0.2.0, timeout-1.4.2, forked-1.3.0, xdist-2.1.0
collected 1 item                                                                                                                                                                                                                                                                                                              

unit_testing/test_cli_deepseg_gm.py::test_sct_deepseg_gm[subprocess] 
Downloading sct testing data.

--------------------------------------------------------------- live log setup ------------------------------------------------------
WARNING  spinalcordtoolbox.download:download.py:142 Removing existing destination folder “sct_testing_data”
INFO     spinalcordtoolbox.download:download.py:42 Trying URL: https://github.com/sct-data/sct_testing_data/releases/download/r20200904/sct_testing_data-r20200904.zip
INFO     spinalcordtoolbox.download:download.py:63 Downloading: sct_testing_data-r20200904.zip
INFO     spinalcordtoolbox.utils:utils.py:330 Creating temporary folder (/tmp/sct-20200925142152.364366-7rmy4q4q)
INFO     spinalcordtoolbox.download:download.py:89 Unzip data to: /tmp/sct-20200925142152.364366-7rmy4q4q
INFO     spinalcordtoolbox.download:download.py:202 Removing temporary folders...
------------------------------------------------------------ live log call ----------------------------------------------------------
DEBUG    test_cli_deepseg_gm:test_cli_deepseg_gm.py:10 
--
Spinal Cord Toolbox (git-master-7ccc048eb1297cabe1c6c2453fa1e749c2b1c0f1)

Will run through the following tests:
- sequentially: sct_deepseg_gm
Checking sct_deepseg_gm.............................[OK]
status: [0]
Finished! Elapsed time: 17s


DEBUG    test_cli_deepseg_gm:test_cli_deepseg_gm.py:11 
PASSED                                                                                                                                                                                                                                                                                                                  [100%]

========================================================== 1 passed in 19.61s =======================================================

This is magic :open_mouth:

[jcohenadad]

Very cool! one thing to keep in mind though: sct_testing currently supports multiprocessing. This is extremely helpful in reducing total test time. By going with pytest, are we going to loose that capability?

[Drulex]

Very cool! one thing to keep in mind though: sct_testing currently supports multiprocessing. This is extremely helpful in reducing total test time. By going with pytest, are we going to loose that capability?

By default yes, but there is https://docs.pytest.org/en/3.0.1/xdist.html that we can leverage.

At the same time we can parallelize the other unit tests as well, they run sequentially at the moment.

[joshuacwnewton]

Ah, my apologies, I think I may have misunderstood. I thought the goal was try for e.g. pytest -> test_sct_deepseg.py and to move away from sct_testing entirely. But, that test is pytest -> sct_testing -> test_sct_deepseg.py.

@Drulex, you mentioned:

One of the ideas to avoid spending effort on migrating perfectly good tests was to lightly wrap the existing functional tests in a way that allows pytest to run them

So, I think I may have been underestimating the work it would take to convert sct_testing tests to pytest tests.

[Drulex]

Ah, my apologies, I think I may have misunderstood. I thought the goal was try for e.g. pytest -> test_sct_deepseg.py and to move away from sct_testing entirely. But, that test is pytest -> sct_testing -> test_sct_deepseg.py.

No you're right, this is one way to do it. What I tried above is basically the "zero effort" way.

Wrapping test_sct_${function} is a cleaner way, but involves a little bit more effort. It depends on how we want to approach this, if we're writing the tests purely for backwards compatibility is there value added in wrapping test_sct vs sct_testing?

[joshuacwnewton]

Wrapping test_sct_${function} is a cleaner way, but involves a little bit more effort.

I may be way off-base here, but I tried a third way... I edited test_sct_deepseg_gm directly, such that it calls sct_deepseg_gm using script_runner.run. See https://github.com/neuropoly/spinalcordtoolbox/pull/2918

It is admittedly a little hacky... I mostly just wanted to quickly demonstrate an idea. Now that I'm implemented it, I'm not very confident this is a worthwhile approach, though, and I'm perfectly happy going with what you've done, @Drulex.

[Drulex]

I updated the testing docs in the wiki (https://github.com/neuropoly/spinalcordtoolbox/wiki/Programming:-Tests). I would appreciate any feedback.

Since the old functional tests have been ported to pytest and the documentation updated, perhaps we can close this issue?

[jcohenadad]

@Drulex the wiki looks good to me, thank you very much for updating it.

However i tried running pytest and ran into the following issue:
bash (venv_sct) julien-macbook:~/temp $ pytest $SCT_DIR ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...] pytest: error: unrecognized arguments: --numprocesses --dist=loadscope /Users/julien/code/sct inifile: /Users/julien/code/sct/setup.cfg rootdir: /Users/julien/code/sct
I am running in master, under SCT's venv, and i made sure to update the deps with pip install -e ..

Note: I followed the procedure here

[Drulex]

From you error it seems xdist is missing from your venv. Did you pip install from the venv?

[Drulex]

Note: I followed the procedure here

I updated the page to remove redundant info.

[jcohenadad]

From you error it seems xdist is missing from your venv. Did you pip install from the venv?

yup.

full details

julien-macbook:~/code/sct $ source python/etc/profile.d/conda.sh
julien-macbook:~/code/sct $ conda activate venv_sct
(venv_sct) julien-macbook:~/code/sct $ pytest 
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --numprocesses --dist=loadscope
  inifile: /Users/julien/code/sct/setup.cfg
  rootdir: /Users/julien/code/sct

(venv_sct) julien-macbook:~/code/sct $ pip install -e .
Obtaining file:///Users/julien/code/sct
Installing collected packages: spinalcordtoolbox
  Attempting uninstall: spinalcordtoolbox
    Found existing installation: spinalcordtoolbox dev
    Uninstalling spinalcordtoolbox-dev:
      Successfully uninstalled spinalcordtoolbox-dev
  Running setup.py develop for spinalcordtoolbox
Successfully installed spinalcordtoolbox
(venv_sct) julien-macbook:~/code/sct $ pytest 
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --numprocesses --dist=loadscope
  inifile: /Users/julien/code/sct/setup.cfg
  rootdir: /Users/julien/code/sct

Running version: 9b91b1ae36b22a92cc0421aa78ebd5be76e2d0b3

[Drulex]

pip install -e does not install dependencies from requirements.txt, it deploys the project in development mode. This basically adds the module to sys.path so that you can change the source code directly and test without having to re-deploy the project.

You need to run pip install -r requirements.txt from the venv to actually install the dependencies.
pip install -e . only needs to be run on the initial install, or anytime setup.py changes (or if you want to install from a different directory).

Alternatively you could also re-run install_sct.
(sorry hadn't noticed earlier you were using -e)

[joshuacwnewton]

I updated the testing docs in the wiki (https://github.com/neuropoly/spinalcordtoolbox/wiki/Programming:-Tests). I would appreciate any feedback.

Thanks much for updating this. I've added some links to pytest documentation where relevant (fixtures, parametrization) but for the most part this is splendid.

[jcohenadad]

You need to run pip install -r requirements.txt from the venv to actually install the dependencies.

oopsi, of course. sorry about that. Working now 😊

[jcohenadad]

+1 for closing this issue. Thank you @Drulex