Skip to content

Conversation

neutrinoceros
Copy link
Member

@neutrinoceros neutrinoceros commented Mar 13, 2024

PR Summary

This fixes part of #4850, and effectively migrates part of the test suite away from nosetest.
It's also based off of #4849

xref pytest-dev/pytest#12113

Opening with a simple find/replace. I'll see which affected modules are still run on nose and wether they can simply be ignored there. If more work is needed to migrate them I'll simply make them nose-only as a quick fix.

@neutrinoceros neutrinoceros force-pushed the test/migrate_test_module_setup branch from d426a38 to 3216abf Compare March 13, 2024 15:19
@neutrinoceros neutrinoceros added this to the 4.3.1 milestone Mar 13, 2024
@neutrinoceros neutrinoceros added bug pytest tests: running tests Issues with the test setup labels Mar 13, 2024
@neutrinoceros
Copy link
Member Author

Interesting... this fails even on pytest but errors resist introspection (this smells like another case of test pollution). Let me see what I can do.

@yut23
Copy link
Member

yut23 commented Mar 13, 2024

Does def teardown() also need to be replaced with def teardown_module()?

@neutrinoceros neutrinoceros force-pushed the test/migrate_test_module_setup branch 2 times, most recently from debe885 to f2f4cb6 Compare March 14, 2024 08:07
@neutrinoceros
Copy link
Member Author

Oh, good point, I didn't think of that 🤦🏻‍♂️
Thanks a lot !

@neutrinoceros neutrinoceros changed the title TST: migrate module-level setup functions to pytest TST: migrate module-level setup/teardown functions to pytest Mar 14, 2024
@neutrinoceros neutrinoceros force-pushed the test/migrate_test_module_setup branch from f2f4cb6 to f87c2f5 Compare March 14, 2024 08:20
@neutrinoceros neutrinoceros force-pushed the test/migrate_test_module_setup branch from d2da8bc to a6e392e Compare March 14, 2024 09:34
@neutrinoceros neutrinoceros linked an issue Mar 14, 2024 that may be closed by this pull request
@neutrinoceros
Copy link
Member Author

This now closes #4856 . Thanks a bunch @yut23 !

--ignore-file=test_gadget_pytest\.py
--ignore-file=test_vr_orientation\.py
--ignore-file=test_particle_trajectories_pytest\.py
--ignore-file=test_image_array\.py
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note to reviewers:
I've added modules that are now pytest-only due to usage of teardown_module. Many other modules have a setup_module method bu no teardown_method, and what they effectively do in setups seems safe (as in, free of side effects), and, to the best of my knowledge, is not actually used in nose.

def setup_module():
    from yt.config import ytcfg

    ytcfg["yt", "internals", "within_testing"] = True

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that makes sense. Do you think that having pytest-only modules will change the relative runtimes of the different test suites?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not in any significant way. The four modules I just made pytest-only (combined) only take a couple seconds to run locally.

@neutrinoceros
Copy link
Member Author

@yt-fido test this please

@neutrinoceros neutrinoceros marked this pull request as ready for review March 14, 2024 11:16
def setup():

def setup_module():
global old_serialize
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it'd be nice to have a pytest fixture for patching yt config options...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that fixtures are flexible enough to do that, but we might be able to achieve this with a context manager wrapped in a decorator ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was thinking that a fixture that used the monkeypatch fixture internally might work -- I was just playing around with making a copy of a YTConfig instance, modifying a value and then using monkeypatch to set yt.config.ytcfg to point to the modified copy and it seemed to work. But it was a little circuitous and a more general context manager would much clearer.

In any case, a discussion for a different issue/PR :)

@neutrinoceros neutrinoceros merged commit 3db19f2 into yt-project:main Apr 1, 2024
@neutrinoceros neutrinoceros deleted the test/migrate_test_module_setup branch April 1, 2024 07:42
meeseeksmachine pushed a commit to meeseeksmachine/yt that referenced this pull request Apr 1, 2024
neutrinoceros added a commit that referenced this pull request Apr 1, 2024
…2-on-yt-4.3.x

Backport PR #4852 on branch yt-4.3.x (TST: migrate module-level setup/teardown functions to pytest)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug pytest tests: running tests Issues with the test setup
Projects
Development

Successfully merging this pull request may close these issues.

TST: incompatibilities with pytest 8 (tracking issue)
4 participants