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

Update whichmodule tests in cloudpickle to work with numpy 2.2. #546

Merged
merged 2 commits into from
Jan 14, 2025

Conversation

petrmitrichev
Copy link
Contributor

  1. test_non_module_object_passing_whichmodule_test was not testing what it was supposed to test, because after some refactoring passing "name" to "_whichmodule" became mandatory for the module lookup to occur. Changed the test to pass "func" name.

  2. test_importing_multiprocessing_does_not_impact_whichmodule was no longer testing what it was supposed to test after the numpy 2.2 release which caused the numpy symbols to have a module attached, and therefore the module lookup was no longer occuring (and the test was failing). Changed the test to avoid dependency on numpy, instead testing with our own function.

1) test_non_module_object_passing_whichmodule_test was not testing what it was supposed to test,
because after some refactoring passing "name" to "_whichmodule" became mandatory for the module lookup to occur.
Changed the test to pass "func" name.

2) test_importing_multiprocessing_does_not_impact_whichmodule was no longer testing what it was supposed
to test after the numpy 2.2 release which caused the numpy symbols to have a module attached, and therefore
the module lookup was no longer occuring.
Changed the test to avoid dependency on numpy, instead testing with our own function.
@petrmitrichev
Copy link
Contributor Author

To clarify, when I write "was not testing what it was supposed to test", I mean that:

  1. test_non_module_object_passing_whichmodule_test was passing even with this line commented out.
  2. test_importing_multiprocessing_does_not_impact_whichmodule was failing with numpy 2.2 as the new return value became just "numpy". But adding "numpy" to the list of valid answers would make it pass even with this line commented out.

Copy link
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

I agree, this is an improvement in test maintainability and correctness.

Thanks for the fix.

@ogrisel ogrisel merged commit f390192 into cloudpipe:master Jan 14, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants