[MAINT] Refactor fixtures #3905

Remi-Gau · 2023-08-11T20:34:17Z

Relates to Refactor pytest fixtures so that they are reusable #3331

Changes proposed in this pull request:

In nilearn/conftest.py adds:

constants for common affines and shapes
functions to return those constant
fixtures to use those constants
functions and fixtures to use 3D or 4D NifTiImages filled with zeros, ones, or random numbers

Apply those changes in the tests that have already been refactored

github-actions · 2023-08-11T20:34:34Z

👋 @Remi-Gau Thanks for creating a PR!

Until this PR is ready for review, you can include the [WIP] tag in its title, or leave it as a github draft.

Please make sure it is compliant with our contributing guidelines. In particular, be sure it checks the boxes listed below.

PR has an interpretable title.
PR links to Github issue with mention Closes #XXXX (see our documentation on PR structure)
Code is PEP8-compliant (see our documentation on coding style)
Changelog or what's new entry in doc/changes/latest.rst (see our documentation on PR structure)

For new features:

There is at least one unit test per new function / class (see our documentation on testing)
The new feature is demoed in at least one relevant example.

For bug fixes:

There is at least one test that would fail under the original bug conditions.

We will review it as quick as possible, feel free to ping us with questions if needed.

Remi-Gau · 2023-08-11T22:43:25Z

oops... introduced some test pollution
will see if https://github.com/asottile/detect-test-pollution can help

codecov · 2023-08-12T00:13:52Z

Codecov Report

Merging #3905 (e0bcc40) into main (457c79e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3905   +/-   ##
=======================================
  Coverage   91.76%   91.76%           
=======================================
  Files         134      134           
  Lines       15747    15747           
  Branches     3283     3283           
=======================================
  Hits        14451    14451           
  Misses        752      752           
  Partials      544      544

Flag	Coverage Δ
macos-latest_3.10	`91.68% <ø> (?)`
macos-latest_3.11	`91.68% <ø> (?)`
macos-latest_3.8	`91.64% <ø> (?)`
macos-latest_3.9	`91.64% <ø> (?)`
ubuntu-latest_3.10	`91.68% <ø> (ø)`
ubuntu-latest_3.11	`91.68% <ø> (ø)`
ubuntu-latest_3.8	`91.64% <ø> (?)`
ubuntu-latest_3.9	`91.64% <ø> (ø)`
windows-latest_3.10	`91.62% <ø> (?)`
windows-latest_3.11	`91.62% <ø> (ø)`
windows-latest_3.8	`91.58% <ø> (ø)`
windows-latest_3.9	`91.58% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

ymzayek · 2023-08-17T09:43:06Z

nilearn/conftest.py

@Remi-Gau why is it that for mni affine the fixture returns the AFFINE_MNI array itself whereas with the eye affine the fixture returns a function that returns AFFINE_EYE array? Same question for shape_3d_default vs shape_4d_default

General answer.

I went for

# global variable X = "some value" # function def _x() return X # fixture @pytest.mark.fixture() def x(): return _x()

When some other fixtures needed to access the value of X and change it.

So to avoid having that fixture change the global (AKA "best way to make things break") I added a function as a go between.

I have not done that systematically because I have not had the need for it for all of those constants. But I think that it would make sense to have the same pattern for all, to avoid this confusion.

nilearn/decomposition/tests/test_base.py

nilearn/regions/tests/test_region_extractor.py

ymzayek · 2023-08-18T09:00:34Z

@Remi-Gau thanks for the explanations. I don't necessarily think you have to change the way you did things if you have a good reason but a comment or docstrings for the helper functions and fixtures might be sufficient. And this could take a bit of extra work to get a good "base" for fixtures that we can then iterate on but I think it will improve testing quite a bit so thanks!

Remi-Gau · 2023-08-18T09:35:35Z

@Remi-Gau thanks for the explanations. I don't necessarily think you have to change the way you did things if you have a good reason but a comment or docstrings for the helper functions and fixtures might be sufficient. And this could take a bit of extra work to get a good "base" for fixtures that we can then iterate on but I think it will improve testing quite a bit so thanks!

Good point will add some comments and doc strings.

nilearn/conftest.py

nilearn/plotting/tests/test_img_plotting/test_plot_roi.py

nilearn/plotting/tests/test_img_plotting/test_img_plotting.py

ymzayek · 2023-08-21T08:54:54Z

nilearn/conftest.py

@@ -89,7 +94,114 @@ def close_all():
 )


-def _mni_3d_img(affine=MNI_AFFINE):
+def _affine_mni():


Does anything prevent from always using this even inside conftest? What is the benefit of still having the global?

yeah I was thinking that the global was progressively becoming useless.

will try to simplify.

I actually removed all constants.

But isn't that a breaking change ? I'd rather have deprecation cycles...

looking through all the files changed, all those constants only appear in tests so I don't think deprecation is necessary there.

are we actually storing anything in conftest.py that is not just for testing? if so we should probably move it somewhere else

bthirion

LGTM. Simply woried about API breakage in conftest.py

bthirion · 2023-08-21T19:33:53Z

nilearn/conftest.py

@@ -89,7 +94,114 @@ def close_all():
 )


-def _mni_3d_img(affine=MNI_AFFINE):
+def _affine_mni():


But isn't that a breaking change ? I'd rather have deprecation cycles...

Remi-Gau · 2023-08-21T19:45:37Z

I was thinking of adding a bit of contributing doc regarding the use of fixtures: should I do it in this PR? It feels more appropriate but it may make the PR bigger.

bthirion · 2023-08-21T19:46:00Z

I was just thinking that some extra package may rely on it ...

bthirion · 2023-08-21T19:46:40Z

I was thinking of adding a bit of contributing doc regarding the use of fixtures: should I do it in this PR? It feels more appropriate but it may make the PR bigger.

Should be in this PR I think.

Remi-Gau · 2023-08-21T20:06:29Z

I was just thinking that some extra package may rely on it ...

except pytest plugins I think that no other package would expect its config information to go in there (at least I kind of hope so)

ymzayek · 2023-08-22T08:05:40Z

I was just thinking that some extra package may rely on it ...

except pytest plugins I think that no other package would expect its config information to go in there (at least I kind of hope so)

In addition the changes in conftest.py have not been released in a stable version yet

Remi-Gau · 2023-08-22T21:29:33Z

CONTRIBUTING.rst

Updated the doc on the tests to talk about fixtures and improve this section in general with a couple of extra links.

nilearn/conftest.py

CONTRIBUTING.rst

ymzayek · 2023-08-23T09:35:40Z

CONTRIBUTING.rst

+(for example you need to generate some data, or a NiftiImage object or a file...)
+you can use `pytest fixtures <https://docs.pytest.org/en/6.2.x/fixture.html>`_
+to help you mock this data
+(more information on pytest fixtures in `this video <https://www.youtube.com/watch?v=ScEQRKwUePI>`_).
+
+Fixture are recognizable because they have a `@pytest.fixture` decorator.
+Fixtures that are shared by many tests modules can be found in `nilearn/conftest.py`
+but some fixures specific to certain modules can also be kept in that testing module.


maybe we can be more explicit in instructing to first see if any of the available fixtures can be used before writing a new one

added one sentence to make it explicit.

CONTRIBUTING.rst

ymzayek · 2023-08-23T09:38:44Z

CONTRIBUTING.rst

 Tests must be seeded to avoid random failures.
 For objects using random seeds (e.g. scikit-learn estimators), pass either
-a  `np.random.RandomState` or an `int` as the seed.
-When your test use random numbers,  those must be generated through:
+a `np.random.RandomState` or an `int` as the seed.


Maybe for a separate PR but shouldn't we update to use the Generator API as mentioned in #3331 (comment)?

I updated it here already: what do you think?

Yes, good with me! At some point we can update our tests to use it but that really should be in a separate PR

agreed

I will take care of it soon but it should be done in a separate or several PRs

Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>

…_fixtures

ymzayek

Thx @Remi-Gau !

Remi-Gau added 4 commits August 11, 2023 11:24

rename mni affine fixture

8c587b8

further fixture refactroring

e59ef4c

refactor fixtures in nilearn/regions

89efd6f

refactor fixtures in decomposition

6dd1961

fix test pollution

65d952c

Remi-Gau marked this pull request as ready for review August 11, 2023 22:53

Remi-Gau mentioned this pull request Aug 12, 2023

[MAINT] Refactor/test utils #3910

Merged

1 task

ymzayek reviewed Aug 17, 2023

View reviewed changes

nilearn/decomposition/tests/test_base.py Show resolved Hide resolved

nilearn/regions/tests/test_region_extractor.py Outdated Show resolved Hide resolved

Remi-Gau added 2 commits August 17, 2023 21:39

try to get around the global var problem

7e23819

Merge remote-tracking branch 'upstream/main' into refactor_fixtures

4259dd8

add some doc in fixtures

c77863a

ymzayek reviewed Aug 21, 2023

View reviewed changes

Remi-Gau added this to the release 0.10.2 milestone Aug 21, 2023

rm constants

e9fc3a5

bthirion reviewed Aug 21, 2023

View reviewed changes

Remi-Gau added 2 commits August 22, 2023 16:45

add rng fixture

2989d0a

add doc about fixture

e910b67

Remi-Gau commented Aug 22, 2023

View reviewed changes

nilearn/conftest.py Show resolved Hide resolved

ymzayek reviewed Aug 23, 2023

View reviewed changes

Remi-Gau and others added 5 commits August 24, 2023 05:49

Apply suggestions from code review

8c3b110

Co-authored-by: Yasmin <63292494+ymzayek@users.noreply.github.com>

update contributing

e830f4b

Merge remote-tracking branch 'origin/refactor_fixtures' into refactor…

f79c1dc

…_fixtures

Merge branch 'main' into refactor_fixtures

f6ff611

fix tests

e0bcc40

ymzayek approved these changes Aug 24, 2023

View reviewed changes

Remi-Gau merged commit 9d5409c into nilearn:main Aug 24, 2023
29 checks passed

Remi-Gau deleted the refactor_fixtures branch August 24, 2023 13:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MAINT] Refactor fixtures #3905

[MAINT] Refactor fixtures #3905

Remi-Gau commented Aug 11, 2023

github-actions bot commented Aug 11, 2023 •

edited by Remi-Gau

Remi-Gau commented Aug 11, 2023

codecov bot commented Aug 12, 2023 •

edited

ymzayek Aug 17, 2023

Remi-Gau Aug 17, 2023

ymzayek commented Aug 18, 2023

Remi-Gau commented Aug 18, 2023

ymzayek Aug 21, 2023

Remi-Gau Aug 21, 2023

Remi-Gau Aug 21, 2023

bthirion Aug 21, 2023

Remi-Gau Aug 21, 2023

bthirion left a comment

bthirion Aug 21, 2023

Remi-Gau commented Aug 21, 2023

bthirion commented Aug 21, 2023

bthirion commented Aug 21, 2023

Remi-Gau commented Aug 21, 2023

ymzayek commented Aug 22, 2023

Remi-Gau Aug 22, 2023

ymzayek Aug 23, 2023

Remi-Gau Aug 24, 2023

ymzayek Aug 23, 2023

Remi-Gau Aug 24, 2023

ymzayek Aug 24, 2023

Remi-Gau Aug 24, 2023

ymzayek left a comment

[MAINT] Refactor fixtures #3905

[MAINT] Refactor fixtures #3905

Conversation

Remi-Gau commented Aug 11, 2023

github-actions bot commented Aug 11, 2023 • edited by Remi-Gau

Remi-Gau commented Aug 11, 2023

codecov bot commented Aug 12, 2023 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ymzayek commented Aug 18, 2023

Remi-Gau commented Aug 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bthirion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Remi-Gau commented Aug 21, 2023

bthirion commented Aug 21, 2023

bthirion commented Aug 21, 2023

Remi-Gau commented Aug 21, 2023

ymzayek commented Aug 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ymzayek left a comment

Choose a reason for hiding this comment

github-actions bot commented Aug 11, 2023 •

edited by Remi-Gau

codecov bot commented Aug 12, 2023 •

edited