Skip to content
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

pytest required to use yt.testing #4507

Closed
chrishavlin opened this issue Jun 14, 2023 · 4 comments · Fixed by #4508
Closed

pytest required to use yt.testing #4507

chrishavlin opened this issue Jun 14, 2023 · 4 comments · Fixed by #4508
Labels
docs enhancement Making something better pytest
Milestone

Comments

@chrishavlin
Copy link
Contributor

This is likely a bit of an edge case, but because we import pytest at the top of yt.testing, one needs pytest installed to import yt.testing.

I commonly use the fake_ datasets in various ways, often in fresh environments and I know of at least one place in the docs where we explicitly import yt.testing (https://yt-project.org/doc/examining/loading_via_functions.html), both of which result in import errors if you only do a pip install yt or even a pip install yt[full].

Any objection to me fixing that?

I the minimal fix is to just move that import pytest to within the only function that uses it (requires_module_pytest). alternatively, I think yt.testing has grown to the point where it might make sense to split it up a bit -- maybe move all of the fake_ dataset constructors to their own module in order to separate dataset construction from test execution utilities?

@chrishavlin chrishavlin added enhancement Making something better docs pytest labels Jun 14, 2023
@matthewturk
Copy link
Member

I also ran into this! I think moving the fake stuff somewhere else would be fine, or even hiding it inside something like load_sample() somehow, I dunno.

@neutrinoceros
Copy link
Member

For context #3771
No objection to nesting the imports. Splitting the module may not be worth the backward compatibility layer needed to not break downstream users.

@chrishavlin
Copy link
Contributor Author

Oh! I remember that discussion now. Thanks for the reminder!

I'll think a bit about splitting the module, but if it gets annoying I'll settle for moving the import... more soon...

@chrishavlin
Copy link
Contributor Author

Ok, splitting out the fake_ datasets did indeed prove rather annoying... I may think more about that in the future, but for now I just submitted a PR that simply nests the pytest import.

@neutrinoceros neutrinoceros added this to the 4.3.0 milestone Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs enhancement Making something better pytest
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants