Test setup and teardown

[grlee77]

Description

closes #3439

Many of the test folders have an __init__.py file containing the following code:

from ..testing import setup_test, teardown_test

def setup():
    setup_test()

def teardown():
    teardown_test()

As far as I can tell, pytest never calls these setup or teardown methods (I added print statements and ran pytest with the -s option, but nothing was visible). The intention of the setup_test method seems to have been to cause error on any warnings raised in the tests, while teardown_test restores the default errorstate.

In this PR, I have removed all of these from the __init__.py files and just added it in one place in the top-level conftest.py file instead, where I verified that pytest does indeed call them. This has resulted in failures on a number of warnings that were previously uncaught. I fixed most of these in this PR, but there are a couple things to follow up on:

1.) I did not try to address the warnings in the skimage.io module, but have just set them not to error for now.
2.) There is a test case that fails in the viewer module (due to a call to data.imread, which hasn't existed for some time). I changed this to io.imread, but the test still fails locally. Apparently this one never gets run on CI? I haven't yet looked into it further.

Checklist

• Docstrings for all functions
• Gallery example in ./doc/examples (new features only)
• Benchmark in ./benchmarks, if your changes aren't covered by an
  existing benchmark
• Unit tests
• Clean style in the spirit of PEP8

For reviewers

• Check that the PR title is short, concise, and will make sense 1 year
  later.
• Check that new functions are imported in corresponding __init__.py.
• Check that new features, API changes, and deprecations are mentioned in
  doc/release/release_dev.rst.

[hmaarrfk]

what does this do? we need this?

[hmaarrfk]

I see so this should really be open as an issue, for us to address the warnings appropriatly.

[grlee77]

Yes, I opened a new issue at #5337 just now where the specific warnings that I observed locally are listed. 3 are deprecated parameters names for tifffile and the rest are ResourceWarnings about unclosed image files.

[hmaarrfk]

Couuld you reference the pytest docs that explain the fixture you used. Do these setup/teardown get called every test? module? session?

[pep8speaks]

Hello @grlee77! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-08-15 10:08:44 UTC

[grlee77]

Couuld you reference the pytest docs that explain the fixture you used. Do these setup/teardown get called every test? module? session?

They are two of the hooks described under pytest_runtest_protocol. They are run for each test and will apply to all tests since this conftest.py file is located in the root.
See also: pytest_runtest_setup and pytest_runtest_teardown and conftest.py per-directory plugins.

If we only want it enabled on some modules, I think we have to instead move it into a conftest.py file within each module where we want it to run.

There may very well be other, better ways to do this with pytest. If so, please let me know.

[grlee77]

I had to add some additional warning skip cases for things that caused warnings when using the minimum dependencies. It looks like everything is passing here now (aside from one Azure timeout that seems to occur somewhat regularly in recent PRs)

[jni]

Great! I've made a minor code suggestion, otherwise this is a very satisfying cleanup PR, thanks @grlee77!

[hmaarrfk]

The only thing I'm worried about is the fact that warnings appear with strange combinations of software that we don't test for.

Updated numpy, but old secondary library.

Do we have a way of disabling test failures?

Does this switch work?
https://scikit-image.org/docs/stable/install.html#warnings-during-testing-phase

[grlee77]

The only thing I'm worried about is the fact that warnings appear with strange combinations of software that we don't test for.

Yes, that concerns me a bit too. The question is if the annoyance of updating these outweighs the benefit of finding potential issues early. (One bug in remove_small_holes was identified and fixed while working on this PR)

Perhaps it should be disabled by default and only 1 or 2 CI jobs can enable the strict warning checks via an environment variable.

Does this switch work?
https://scikit-image.org/docs/stable/install.html#warnings-during-testing-phase

Good question, let me check into this.

[hmaarrfk]

@grlee77 unfortunately it isn't just an "annoyance" but really puts the breaks on downstream packagers doing their job.

Failing on warnings because a warning in our test suite that was addressed in Scipy 1.6 for numpy 1.20 but the particular distribution is pinned to scipy 1.5 is not fun for them.

[grlee77]

Does this switch work?
https://scikit-image.org/docs/stable/install.html#warnings-during-testing-phase

Good question, let me check into this.

This SKIMAGE_TEST_STRICT_WARNINGS flag is only used to control whether the expected_warnings context manager raises exceptions if additional warnings are found (it defaults to True). Since the majority of tests are not run within this context manager, most are unaffected by that env var.

Should we have a separate environment variable that controls the use of the global warnings behavior as in this PR?

[grlee77]

For reference, the original test setup/teardown functions that I removed from the __init__.py files originate from #1313 which is from >6 years ago (when we were still using nose for testing). Most likely we just unintentionally did not preserve the original behavior when updating to pytest.

[grlee77]

@grlee77 unfortunately it isn't just an "annoyance" but really puts the breaks on downstream packagers doing their job.

Failing on warnings because a warning in our test suite that was addressed in Scipy 1.6 for numpy 1.20 but the particular distribution is pinned to scipy 1.5 is not fun for them.

Okay, I was not thinking about that case. If I change it so that these errors are disabled by default, but can be enabled upon request, than that would be fine wouldn't it?

[hmaarrfk]

Yes. I would be in favor of that.

[hmaarrfk]

Thanks!