Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

TST replace assert_warns* by pytest.warns in module svm/tests #19424

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 11, 2021
Merged

TST replace assert_warns* by pytest.warns in module svm/tests #19424

merged 3 commits into from
Feb 11, 2021

Conversation

shivamgargsya
Copy link
Contributor

@shivamgargsya shivamgargsya commented Feb 10, 2021 •
edited by jeremiedbb
Loading

Reference Issues/PRs

Part of #19414 for svm/tests

What does this implement/fix? Explain your changes.

Removed use of assert_warns, assert_warns_message in svm/tests package

Any other comments?

The fix is part of complete fix for issue #19414

@shivamgargsya shivamgargsya changed the title Fix issue 'Remove the use of assert_warns and assert_warns_message from the tests' for svm/tests [MRG] Fix issue 'Remove the use of assert_warns and assert_warns_message from the tests' for svm/tests Feb 10, 2021
jeremiedbb
jeremiedbb reviewed Feb 10, 2021
View reviewed changes
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 @shivamgargsya. Instead of matching anything, it would be better to explicitly match the warning message we're expecting

@shivamgargsya shivamgargsya changed the title [MRG] Fix issue 'Remove the use of assert_warns and assert_warns_message from the tests' for svm/tests [WIP] Fix issue 'Remove the use of assert_warns and assert_warns_message from the tests' for svm/tests Feb 10, 2021
@shivamgargsya shivamgargsya force-pushed the feature/svm_tests_warning_remove branch from 91f07b6 to 5f4a14e Compare February 10, 2021 16:21
@shivamgargsya shivamgargsya changed the title [WIP] Fix issue 'Remove the use of assert_warns and assert_warns_message from the tests' for svm/tests [MRG] Fix issue 'Remove the use of assert_warns and assert_warns_message from the tests' for svm/tests Feb 10, 2021
@shivamgargsya shivamgargsya changed the title [MRG] Fix issue 'Remove the use of assert_warns and assert_warns_message from the tests' for svm/tests [WIP] Fix issue 'Remove the use of assert_warns and assert_warns_message from the tests' for svm/tests Feb 10, 2021
@shivamgargsya
Copy link
Contributor Author

hi @jeremiedbb Thanks for reviewing PR. I have updated changes to match for specific warning message.

@shivamgargsya shivamgargsya changed the title [WIP] Fix issue 'Remove the use of assert_warns and assert_warns_message from the tests' for svm/tests [MRG] Fix issue 'Remove the use of assert_warns and assert_warns_message from the tests' for svm/tests Feb 10, 2021
jeremiedbb
jeremiedbb approved these changes Feb 10, 2021
View reviewed changes
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.

lgtm, thanks @shivamgargsya !

@glemaitre glemaitre changed the title [MRG] Fix issue 'Remove the use of assert_warns and assert_warns_message from the tests' for svm/tests TST replace assert_warns* by pytest.warns in module svm/tests Feb 11, 2021
@glemaitre glemaitre self-requested a review February 11, 2021 09:43
glemaitre
glemaitre approved these changes Feb 11, 2021
View reviewed changes
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM. I just pushed a quick commit where I replaced the backslash (we try to not use them) and also replace the assert_no_warnigs that was not indicated in the issue.

@glemaitre
Copy link
Member

I will merge the PR when the CIs will be green

@glemaitre glemaitre merged commit a8d6361 into scikit-learn:main Feb 11, 2021
@glemaitre
Copy link
Member

Thank you @shivamgargsya

@shivamgargsya
Copy link
Contributor Author

Thanks @glemaitre for making the changes and merging the commit.

@glemaitre glemaitre mentioned this pull request Apr 22, 2021
Merged
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre approved these changes

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
module:svm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@shivamgargsya @glemaitre @jeremiedbb
Morty Proxy This is a proxified and sanitized view of the page, visit original site.