ENH: Test calling the scripts #78

jhlegarreta · 2024-12-30T17:39:40Z

Test the CLI scripts:

Adapt and transfer the contents of the test_scripts.py test script to a more modern and compact approach that uses the pytest pytest-console-script plugin and that does not require fiddling with the location of the files. This allows to test that the CLI scripts are installed appropriately/called using the names defined in the pyproject.toml project.scripts section.
Split the tests into two different files following the scripts being tested: tract_math and tract_querier.
Remove the test_scripts.py file.

Add the pytest-console-script dependency so that the scripts can be called when executing pytest.

Convert the output.values() ordered dictionary values to a list prior to slicing in decorator.py. Fixes:

"/src/tract_querier/scripts/cli_tract_math.py",
 line 109, in main\n    operations[args.operation]
(\n  File "/src/tract_querier/tract_querier/tract_math/decorator.py",
 line 148, in wrapper\n    process_output(func(*option_and_args))\n
  File "/src/tract_querier/tract_querier/tract_math/decorator.py",
 line 199, in process_output\n
    first_value = output.values()[0]\n
TypeError: \'odict_values\' object is not subscriptable\n')

raised locally when running the script tests.

jhlegarreta · 2024-12-30T17:43:53Z

PR #77 should be merged before this one, then I'd rebase on master and push-force and CI tests should come out green.

jhlegarreta · 2025-01-14T14:59:06Z

e.g. https://github.com/demianw/tract_querier/actions/runs/12770291868/job/35594799004?pr=78#step:6:52:

tract_querier/tests/test_tract_math.py::test_help_option[inprocess] PASSED [ 80%]
tract_querier/tests/test_tract_querier.py::test_help_option[inprocess] PASSED [ 82%]

Ready to be merged.

jhlegarreta · 2025-01-15T13:31:26Z

@demianw This is ready to be merged.

demianw · 2025-01-15T13:58:37Z

@jhlegarreta I think that if this is good idea.

However, the testing file tract_querier/tests/test_scripts.py file which is aiming at testing the same things but without the dependency on pytest-console-scripts. Specifically

tract_querier/tract_querier/tests/test_scripts.py

Line 31 in afb437b

def test_tract_querier_help():

is testing some of the same you are testing here.

We should choose one of the two strategies and stick to it and not have two parallel systems performing the same checks.

jhlegarreta · 2025-01-15T14:17:10Z

Oh, that's true Deminan, sorry. However, that seems not to be running:
https://github.com/demianw/tract_querier/actions/runs/12770291868/job/35594797321?pr=78

Even if I must say that I believe I left behind modifying the file names in PR #77:
https://github.com/demianw/tract_querier/blob/master/tract_querier/tests/test_scripts.py#L13-L21

The test has not been running all this time either, e.g. two runs prior to merging PR #77:
https://github.com/demianw/tract_querier/actions/runs/12709947683/job/35429996385
https://github.com/demianw/tract_querier/actions/runs/12349224155/job/34459614753

I do not know why that is, but these are my thoughts:

Retrieving the path to run this test manually as done in https://github.com/demianw/tract_querier/blob/master/tract_querier/tests/test_scripts.py#L13-L21 seems an additional overhead.
The advantage of this PR is that we have evidence that the installation of these scripts following https://github.com/demianw/tract_querier/blob/master/pyproject.toml#L48-L50 is working.

Let me know what your thoughts are. I may not have the bandwidth to investigate why test_scripts.py has not been running, but I am happy if you manage to investigate that and make it run.

demianw · 2025-01-15T14:22:29Z

It's a good feedback. So just move all the tests there to the new framework that you started. Does that seem a good solution to you ?

jhlegarreta · 2025-01-15T14:24:30Z

So just move all the tests there to the new framework that you started. Does that seem a good solution to you ?

Sounds good. Will do today/in the coming days. Thanks.

jhlegarreta · 2025-01-15T16:56:20Z

OK, I have had a look at integrating the tests into the structure proposed in this PR and I believe one of the reasons why this has not been running might be due to:

tract_querier/tract_querier/tests/datasets.py

Line 31 in afb437b

@unittest.skip("temporarily disabled")

midas.kitware.com is no longer available, so we will need to make the testing data available elsewhere if we want these tests to be included. Related to issue #72. #72 (comment) offered some possibilities that would work for this case as well.

demianw · 2025-01-16T09:06:06Z

Do you have a preference for the place to drop the files ?

OK, I have had a look at integrating the tests into the structure proposed in this PR and I believe one of the reasons why this has not been running might be due to:

tract_querier/tract_querier/tests/datasets.py

Line 31 in afb437b

@unittest.skip("temporarily disabled")

midas.kitware.com is no longer available, so we will need to make the testing data available elsewhere if we want these tests to be included. Related to issue #72. #72 (comment) offered some possibilities that would work for this case as well.

jhlegarreta · 2025-01-16T18:34:44Z

Do you have a preference for the place to drop the files ?

x-ref PR #88: test files were added to OSF. OSF works for the immediate purposes of moving forward. Thanks Demian.

jhlegarreta · 2025-01-17T16:43:54Z

Doc build failure is unrelated (addressed in PR #89). This is ready to be merged for when PR #88 will be finalized and merged.

Tests are passing because actually exercising the scripts is being skipped:

tract_querier/tests/test_tract_math.py::test_tract_math_count[inprocess] SKIPPED [ 79%]
tract_querier/tests/test_tract_querier.py::test_tract_querier_query[inprocess] SKIPPED [ 83%]

https://github.com/demianw/tract_querier/actions/runs/12833173193/job/35787565543?pr=78#step:6:53
https://github.com/demianw/tract_querier/actions/runs/12833173193/job/35787565543?pr=78#step:6:55

due to the decorator:

tract_querier/tract_querier/tests/datasets.py

Line 31 in cc5580f

@unittest.skip("temporarily disabled")

It is being removed in PR #88, so we should be good after that.

tract_querier/tract_math/decorator.py

Move test files to OSF and refactor the `datasets.py` module: - Use the new URLs of the test files stored in OSF. - Add the `b` prefix to the MD5 hashes of the files to match the binary string returned by the hash computation method. - Do not try to encode the byte object with the file contents: some of the test files are binary files, and can only be read in binary format; trying to read them in text mode raises a `UnicodeDecodeError` error when calling `encode('utf-8')` on them, and calling `encode('utf-8')` on the bytes read raises a `AttributeError: 'bytes' object has no attribute 'encode'` error. Thus, read them in binary mode and compute the hash on the returned binary data without trying to encode them. Reading the files using chunks is not implemented as the files are not that large. - Remove the unnecessary dependency on `unittest`.

jhlegarreta · 2025-01-24T17:45:24Z

PR #88 is no longer relevant (x-ref #88 (comment)), and tests are being exercised and are passing now, e.g.:

tract_querier/tests/test_tract_math.py::test_help_option[inprocess] PASSED [ 76%]
tract_querier/tests/test_tract_math.py::test_tract_math_count[inprocess] PASSED [ 79%]
tract_querier/tests/test_tract_querier.py::test_help_option[inprocess] PASSED [ 81%]
tract_querier/tests/test_tract_querier.py::test_tract_querier_query[inprocess] PASSED [ 83%]

https://github.com/demianw/tract_querier/actions/runs/12954696522/job/36137183798?pr=78#step:6:52

Test the CLI scripts: - Adapt and transfer the contents of the `test_scripts.py` test script to a more modern and compact approach that uses the `pytest` `pytest-console-script` plugin and that does not require fiddling with the location of the files. This allows to test that the CLI scripts are installed appropriately/called using the names defined in the `pyproject.toml` `project.scripts` section. - Split the tests into two different files following the scripts being tested: `tract_math` and `tract_querier`. - Remove the `test_scripts.py` file. Add the `pytest-console-script` dependency so that the scripts can be called when executing `pytest`. Convert the `output.values()` ordered dictionary values to a list prior to slicing in `decorator.py`. Fixes: ``` "/src/tract_querier/scripts/cli_tract_math.py", line 109, in main\n operations[args.operation] (\n File "/src/tract_querier/tract_querier/tract_math/decorator.py", line 148, in wrapper\n process_output(func(*option_and_args))\n File "/src/tract_querier/tract_querier/tract_math/decorator.py", line 199, in process_output\n first_value = output.values()[0]\n TypeError: \'odict_values\' object is not subscriptable\n') ``` raised locally when running the script tests.

Prefer retrieving the next iterator element over slicing.

Use `getfullargspec` instead of deprecated `getargspec` to get the names and default values of function parameters. Fixes: ``` inspect. getargspec() is deprecated since Python 3.0, use inspect. signature() or inspect. getfullargspec() ``` reported by IDE inspection tool.

jhlegarreta force-pushed the TestCallingScripts branch from 1c10bd0 to 62b882c Compare December 30, 2024 18:00

jhlegarreta force-pushed the TestCallingScripts branch from 62b882c to 34b8161 Compare January 14, 2025 14:56

demianw mentioned this pull request Jan 16, 2025

Moved the test files to OSF #88

Open

jhlegarreta force-pushed the TestCallingScripts branch from 34b8161 to 96883d3 Compare January 17, 2025 16:39

jhlegarreta force-pushed the TestCallingScripts branch 2 times, most recently from 9d30828 to 509e5c6 Compare January 17, 2025 18:15

demianw reviewed Jan 24, 2025

View reviewed changes

tract_querier/tract_math/decorator.py Outdated Show resolved Hide resolved

jhlegarreta force-pushed the TestCallingScripts branch from 24d8aaf to 096f980 Compare January 24, 2025 14:33

jhlegarreta requested a review from demianw January 24, 2025 16:05

jhlegarreta force-pushed the TestCallingScripts branch from 85d6107 to b31e868 Compare January 24, 2025 17:39

jhlegarreta added 3 commits January 24, 2025 12:48

PERF: Prefer retrieving the next iterator element over slicing

8f40d02

Prefer retrieving the next iterator element over slicing.

jhlegarreta force-pushed the TestCallingScripts branch from b31e868 to 42334dc Compare January 24, 2025 17:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENH: Test calling the scripts #78

ENH: Test calling the scripts #78

jhlegarreta commented Dec 30, 2024 •

edited

Loading

jhlegarreta commented Dec 30, 2024

jhlegarreta commented Jan 14, 2025

jhlegarreta commented Jan 15, 2025

demianw commented Jan 15, 2025

jhlegarreta commented Jan 15, 2025 •

edited

Loading

demianw commented Jan 15, 2025

jhlegarreta commented Jan 15, 2025

jhlegarreta commented Jan 15, 2025

demianw commented Jan 16, 2025

jhlegarreta commented Jan 16, 2025

jhlegarreta commented Jan 17, 2025 •

edited

Loading

jhlegarreta commented Jan 24, 2025

ENH: Test calling the scripts #78

Are you sure you want to change the base?

ENH: Test calling the scripts #78

Conversation

jhlegarreta commented Dec 30, 2024 • edited Loading

jhlegarreta commented Dec 30, 2024

jhlegarreta commented Jan 14, 2025

jhlegarreta commented Jan 15, 2025

demianw commented Jan 15, 2025

jhlegarreta commented Jan 15, 2025 • edited Loading

demianw commented Jan 15, 2025

jhlegarreta commented Jan 15, 2025

jhlegarreta commented Jan 15, 2025

demianw commented Jan 16, 2025

jhlegarreta commented Jan 16, 2025

jhlegarreta commented Jan 17, 2025 • edited Loading

jhlegarreta commented Jan 24, 2025

jhlegarreta commented Dec 30, 2024 •

edited

Loading

jhlegarreta commented Jan 15, 2025 •

edited

Loading

jhlegarreta commented Jan 17, 2025 •

edited

Loading