Skip to content

TST Change assert from sklearn to pytest style in tests/test_pipeline.py #19678

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 9 commits into from
Mar 16, 2021

Conversation

azihna
Copy link
Contributor

@azihna azihna commented Mar 15, 2021

Reference Issues/PRs
References #14216

What does this implement/fix? Explain your changes.
Changed the assert_raises, assert_message, assert_warns in tests/test_pipeline.py to pytest.raises().
Added additional message checks for tests that seemed like could benefit from further clarification.

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.

Some formatting details but otherwise LGTM.

azihna and others added 4 commits March 15, 2021 11:07
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
@azihna
Copy link
Contributor Author

azihna commented Mar 15, 2021

Using re.escape() causes some problems with regex pattern. Would you mind if I reverted it to the previous state and add a comment explaining the issue? @ogrisel

@ogrisel
Copy link
Member

ogrisel commented Mar 15, 2021

Hum sorry read too quickly and did not get that error_message_1 was actually a regexp. Please go ahead and revert this change back.

azihna and others added 2 commits March 16, 2021 19:06
Co-authored-by: Jérémie du Boisberranger <34657725+jeremiedbb@users.noreply.github.com>
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 @azihna !

@jeremiedbb jeremiedbb merged commit edc4f15 into scikit-learn:main Mar 16, 2021
marrodion pushed a commit to marrodion/scikit-learn that referenced this pull request Mar 17, 2021
@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.

3 participants