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
[MAINT] Refactor/test utils #3910
Conversation
👋 @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.
For new features:
For bug fixes:
We will review it as quick as possible, feel free to ping us with questions if needed. |
Codecov Report
@@ Coverage Diff @@
## main #3910 +/- ##
=======================================
Coverage 91.76% 91.77%
=======================================
Files 134 134
Lines 15747 15750 +3
Branches 3283 3283
=======================================
+ Hits 14451 14454 +3
Misses 752 752
Partials 544 544
Flags with carried forward coverage won't be shown. Click here to find out more. see 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a couple of comments
|
||
####### | ||
def test_check_niimg_wildcards_one_file_name(img_3d_zeros_eye): | ||
tmp_dir = tempfile.tempdir + os.sep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we replace with tmp_path fixture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tried the most obvious ways and they did not work
thinking that maybe we could improve / fixturize 'testing.write_tmp_imgs', but that would be for another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok agreed
OK it seems that 244f0f3 does not play well with testing on windows |
This reverts commit 244f0f3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx @Remi-Gau
Changes proposed in this pull request:
refactor complex tests in nilearn/_utils/tests
more refactoring to do once [MAINT] Refactor fixtures #3905 is in