Skip to content

Replaced assert_raises from utils/tests/test_estimator_checks #19709

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 3 commits into from
Mar 19, 2021

Conversation

avigupta2612
Copy link
Contributor

Reference Issues/PRs

Part of #14216 for utils/tests/test_estimator_checks.py

What does this implement/fix? Explain your changes.

Replaced use of assert_raises and assert_raises_regex with pytest.raises in utils/tests/test_estimator_checks.py

Any other comments?

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 @avigupta2612 !

This is the one file we can not use pytest in, because we run build_tools/azure/test_pytest_soft_dependency.sh on it to make sure estimator_checks works without pytest.

I think it would be good to add a comment to the top of this file that explains why this file does not use pytest.

@avigupta2612
Copy link
Contributor Author

Ok, so I should revert back the changes and add the comment on the top.

Copy link
Member

@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.

Minor comment below but otherwise LGTM.

@ogrisel ogrisel merged commit cc1b171 into scikit-learn:main Mar 19, 2021
@ogrisel
Copy link
Member

ogrisel commented Mar 19, 2021

Thanks!

@NicolasHug
Copy link
Member

While we can't use pytest, we can use the internal _testing.raises context manager here, as done in #18418

@avigupta2612 would you like to submit a PR for this?

@avigupta2612
Copy link
Contributor Author

@NicolasHug Yes, I will work on this.

@glemaitre glemaitre mentioned this pull request Apr 22, 2021
12 tasks
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.

5 participants