Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ENH] Mocks rollout and implementation of testing behaviors #664

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

AlexVCaron
Copy link
Contributor

@AlexVCaron AlexVCaron commented Jan 16, 2023

Quick description

Running tests is long, and is bound to take even longer. To reduce this time, we decided to divide tests in two categories : library testing and scripts testing.

Library testing is fast, done on bite-size pieces of data when possible, and so will run always.

Script testing is slow, done on real-life heavy data from the real-world, and uses extensive parts of the library repetitively. Their reach can be limited with the use of mocks, to prevent repetitive testing and execution of heavy dependencies on the heavy data.

A mock is a replacement (or a patch) that obfuscates a portion of code that can be skipped or modified for the profit of the test case. They are dynamic, meaning that they get instantiated and take effect only at execution, when prescribed. They can be used to modify the effects of a piece of code (change the return value of a function or the attributes and methods of an object) and provide interfaces to inspect code execution (number of times called, with which arguments).

Current implementation :

All bindings into pytest is done by way of a custom plugin, located in scilpy/tests/plugin.py. It handles 3 things :

  • manage lists of modules and packages containing mocks
  • configure CLI options and auto-discover selected mocks
  • exposes fixtures to create, collect and collate auto-discoverable mocks

Mocks can be instantiated in any parts of Scilpy, may it be the scripts or the library. However, they must all be placed inside their associated test paths under the library directory :

  • Decide in which package of scilpy the mock belongs to and add it to <package_location>/tests/fixtures/mocks.py.
  • Use the mock_creator fixture to create the mock(s). Refer to scilpy/reconst/tests/fixtures/mocks.py for an example.

This makes it possible to discover all applicable mocks in pytest infrastructure (they are not applied yet !). Once done, the mock is applied to a test instance using the mock_collector fixture.

Running tests with mocks is simple, add --mocks <list of mocks or all> to your pytest command. To list available mocks, add --mocks all --fixtures

For more information, see scilpy/tests/plugin.py, the module contains a full documentation. Also note that this PR enables all mocks in Jenkins by default.

Type of change

Check the relevant options.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Provide data, screenshots, command line to test (if relevant)

To list fixtures and check mocks are available :
pytest --mocks all --fixtures

2 tests are mocked as of now : test_NODDI_maps and test_execute_angle_aware_bilateral_filtering .

Run them with mocks with :
pytest scripts/tests/test_NODDI_maps.py --mocks all
pytest scripts/tests/test_execute_angle_aware_bilateral_filtering.py --mocks all

Checklist

  • My code follows the style guidelines of this project (run autopep8)
  • I added relevant citations to scripts, modules and functions docstrings and descriptions
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I moved all functions from the script file (except the argparser and main) to scilpy modules
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@arnaudbore arnaudbore self-requested a review January 16, 2023 19:38
@arnaudbore
Copy link
Contributor

@AlexVCaron AlexVCaron changed the title Mocks rollout and implementation of testing behaviors [WIP][ENH] Mocks rollout and implementation of testing behaviors Jan 16, 2023
@frheault
Copy link
Member

Is it possible to have a comparison of timing before and after / with and without MOC?

The code looks good, @arnaudbore do you think we can merge my two PR (python3.10 and the fix) next week and then Alex could go forward with this PR. And as soon as it is working we update the test data, and do a release candidate?

I tested the 4 main flow once, I did not try Freewater. I think I will re-test with a RC1.

@frheault
Copy link
Member

Also, @AlexVCaron can you describe what is in the new archive? What is different?

@AlexVCaron
Copy link
Contributor Author

Also, @AlexVCaron can you describe what is in the new archive? What is different?

TLDR : files are practically the same, but the new ones are the right datatype so comparison of outputs to expectations works for both mocked and unmocked cases.

Three outputs files awaited by the tests have slightly changed to align with unmocked tests, namely :

  • fodf_descoteaux07_sub_full.nii.gz
  • fodf_descoteaux07_sub_sym.nii.gz
  • fodf_descoteaux07_sub_twice.nii.gz

It worked when mocking, since we only check if processing errors were done by the script on those files, but didn't when running the full algorithm on them.

The difference obtained between the output and the one awaited was tiny and after investigations, I think it was because of a type conversion error on my end.

@arnaudbore
Copy link
Contributor

@AlexVCaron
Copy link
Contributor Author

I started thinking about mocks again. Where do we go from here ? There is a few scripts that would benefit from mocking, but we need to address how and when we use the through Jenkins. I also created a sheet to follow improvements that come from using mocking here. I'll update it when new mocks come into action.

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@EmmaRenauld
Copy link
Contributor

We should talk about this at our retreat preparation!

@pep8speaks
Copy link

pep8speaks commented Oct 23, 2023

Hello @AlexVCaron, Thank you for updating !

Line 28:1: E303 too many blank lines (3)
Line 36:28: E128 continuation line under-indented for visual indent

Comment last updated at 2024-01-15 21:32:07 UTC

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@arnaudbore arnaudbore added the WIP Work In Progress label Nov 2, 2023
@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@AlexVCaron AlexVCaron force-pushed the feat/rollout_mocks branch 2 times, most recently from 2ffd734 to f8e5990 Compare December 15, 2023 21:20
@arnaudbore
Copy link
Contributor

@AlexVCaron AlexVCaron force-pushed the feat/rollout_mocks branch 2 times, most recently from 2218c73 to d2d072b Compare December 16, 2023 02:01
@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

@arnaudbore
Copy link
Contributor

Build passed ! Good Job 🍻 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants