Replace deprecated setup() and teardown()

[penguinpee]

Those were compatibility functions for porting from nose. They are now
deprecated and have been removed from pytest.

This will make all tests compatible with pytests 8.x.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.19%. Comparing base (f40ac39) to head (82c8588).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1325      +/-   ##
==========================================
- Coverage   92.23%   92.19%   -0.04%     
==========================================
  Files          99       98       -1     
  Lines       12460    12397      -63     
  Branches     2557     2557              
==========================================
- Hits        11493    11430      -63     
  Misses        644      644              
  Partials      323      323              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[penguinpee]

We saw some other tests failing as well. But I couldn't figure out where the fault was (nibabel, pytest, our build env). If you are interested, I can open an issue.

We are currently testing with pytest 8.2.1. The tests started failing with 8.1.1. I noticed you currently have pytest pinned to <8.1 for some doctestplus test issues. We don't run those. So the issue we are seeing seems another one.

[emollier]

Hi, I was going to prepare a merge request for changes permitting migration to pytest beyond 8.1 and found the present ticket. For information, we followed this as Debian bug #1071461. The problem stems from the fact that setup and teardown were compatibility functions for porting from Nose, but were deprecated, and pytest's variants should be used instead. The correction took the following form on our end:

--- nibabel.orig/nibabel/streamlines/tests/test_streamlines.py
+++ nibabel/nibabel/streamlines/tests/test_streamlines.py
@@ -21,7 +21,7 @@
 DATA = {}
 
 
-def setup():
+def setup_function():
     global DATA
     DATA['empty_filenames'] = [pjoin(data_path, 'empty' + ext) for ext in FORMATS.keys()]
     DATA['simple_filenames'] = [pjoin(data_path, 'simple' + ext) for ext in FORMATS.keys()]
--- nibabel.orig/nibabel/tests/test_deprecated.py
+++ nibabel/nibabel/tests/test_deprecated.py
@@ -15,12 +15,12 @@
 from nibabel.tests.test_deprecator import TestDeprecatorFunc as _TestDF
 
 
-def setup():
+def setup_module():
     # Hack nibabel version string
     pkg_info.cmp_pkg_version.__defaults__ = ('2.0',)
 
 
-def teardown():
+def teardown_module():
     # Hack nibabel version string back again
     pkg_info.cmp_pkg_version.__defaults__ = (pkg_info.__version__,)

For reasons I still fail to make sense of, nibabel/tests/test_deprecated.py remained off the radar until running the test suite in very specific integration test conditions (namely through Debian's autopkgtest).

In hope this helps,
Étienne.

[penguinpee]

Thanks for chiming in. test_deprecated.py was not on my list of troublesome test. From what you describe it seems the root cause is the same. I'd be willing to amend / extend this PR.

The other tests we saw failing came from test_pkg_info.py and test_removalschedule.py. Since we are now talking failing test for pytests >= 8.1.1 in general, I might as well spill the beans:

____________________________ test_cmp_pkg_version_0 ____________________________

    def test_cmp_pkg_version_0():
        # Test version comparator
>       assert cmp_pkg_version(nib.__version__) == 0
E       AssertionError: assert 1 == 0
E        +  where 1 = cmp_pkg_version('5.2.1')
E        +    where '5.2.1' = nib.__version__

nibabel/tests/test_pkg_info.py:28: AssertionError
____________________________ test_unremoved_module _____________________________

    @mock.patch(_sched('MODULE'), [('3.0.0', ['nibabel.nifti1'])])
    def test_unremoved_module():
>       with pytest.raises(AssertionError):
E       Failed: DID NOT RAISE <class 'AssertionError'>

nibabel/tests/test_removalschedule.py:163: Failed

Before extending this PR, I'd appreciate one of the developers chiming in, so I know which direction they'd prefer.

[emollier]

Sure, proceed as you'de prefer. :)

[effigies]

Hi all, thanks for finding this! I think a module-level setup_module() makes sense in both of these cases. I don't think any of these require setup_function(). And I agree with not using TestCase.setUp() unless there is a specific reason to.

I think I had noticed at one point that test_removalschedule had failed to warn of an unfinished removal, so that's been on my mental list to look into. If fixing that fits into this PR, please go ahead.

[penguinpee]

I just force pushed an update to the initial PR using setup_module() throughout. Now all tests happily succeed again. Including the two sets of tests I couldn't explain previously. Thanks @emollier for pointing me in the right direction.

[emollier]

Hi Chris, Chris Markiewicz, on 2024-05-29:

I think a module-level `setup_module()` makes sense in both of these cases. I don't think any of these require `setup_function()`.

I agree about sticking to setup_module. I was still making sense of the required changes when I have introduced the setup_function, as it resolved the failure at the time of writing the patch, but it could miss test entries in future classes and methods that might otherwise require it. Have a nice day, :) Étienne.

[emollier]

Sandro, you're welcome! Have a nice day :)

[effigies]

Thanks!