Skip to content

TST Remove pytest.warns(None) and replace it with warnings.catch_warnings in test_pipeline.py #23089

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

Merged
merged 5 commits into from
Apr 9, 2022

Conversation

lorentzbao
Copy link
Contributor

Reference Issues/PRs

Related to #22572

What does this implement/fix? Explain your changes.

Replaces deprecated pytest.warns(None) in test_pipeline.py with a warnings.catch_warnings() context handler.

Any other comments?

・Replaced all deprecated pytest.warns(None) in test.pipeline.py.
・I think the original code checks if it raises a UserWarning, please correct me if I am wrong.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

For future reviewers: From the PR that added this check: #9716, I think this check was to catch a deprecation warning in 0.18 that is no longer in the library.

lorentzbao and others added 3 commits April 10, 2022 01:15
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
@@ -8,6 +8,7 @@
import itertools

import pytest
import warnings
Copy link
Member

Choose a reason for hiding this comment

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

CI linting error:

Suggested change
import warnings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Have fixed this error.

Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

Thanks @lorentzbao

@jeremiedbb jeremiedbb merged commit bd9336d into scikit-learn:main Apr 9, 2022
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Apr 29, 2022
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants