Skip to content

TST Replace assert_raises* by pytest.raises in model_selection/tests/test_validation.py#19381

Closed
cycks wants to merge 1 commit intoscikit-learn:mainfrom
cycks:remove_use_of_assert_raises_from_test_validation
Closed

TST Replace assert_raises* by pytest.raises in model_selection/tests/test_validation.py#19381
cycks wants to merge 1 commit intoscikit-learn:mainfrom
cycks:remove_use_of_assert_raises_from_test_validation

Conversation

@cycks
Copy link
Copy Markdown
Contributor

@cycks cycks commented Feb 6, 2021

Reference Issues/PRs

References #14216

What does this implement/fix? Explain your changes.

replace assert_raises* by pytest.raise in model_selection/tests/test_validation.py

Any other comments?

cc: (pair programming partner) @mohaseeb

@reshamas
Copy link
Copy Markdown
Member

reshamas commented Feb 6, 2021

#DataUmbrella sprint

thomasjpfan
thomasjpfan approved these changes Feb 6, 2021
View reviewed changes
Copy link
Copy Markdown
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 @cycks !

LGTM

@thomasjpfan thomasjpfan changed the title replace assert_raises* by pytest.raise in model_selection/tests/test_… TST Replace assert_raises* by pytest.raise in model_selection/tests/test_validation.py Feb 6, 2021
lorentzenchr
lorentzenchr reviewed Feb 6, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

Could you also replace assert_raise_message?

@glemaitre glemaitre changed the title TST Replace assert_raises* by pytest.raise in model_selection/tests/test_validation.py TST Replace assert_raises* by pytest.raises in model_selection/tests/test_validation.py Feb 7, 2021
@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented Feb 7, 2021

@cycks We forgot to add in the original issue the assert_raise_message but as mentioned by @lorentzenchr we would like to change them as well.

@cycks
Copy link
Copy Markdown
Contributor Author

cycks commented Feb 7, 2021

@glemaitre I am working on it right now I will submit a pr once I am done.

@glemaitre glemaitre mentioned this pull request Feb 11, 2021
Closed
@reshamas
Copy link
Copy Markdown
Member

reshamas commented Feb 15, 2021

Hello @cycks @mohaseeb
How is this PR going? If you have questions on writing tests, this PR might be a helpful reference: #19388

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Feb 18, 2021

I am working on it right now I will submit a pr once I am done.

As @glemaitre said in #19432, please push your commit to the branch of this current PR instead to finalize it.

@mohaseeb mohaseeb mentioned this pull request Mar 1, 2021
Merged
@mohaseeb
Copy link
Copy Markdown
Contributor

mohaseeb commented Mar 1, 2021

I'm working at completing this PR.

Due to lack of permissions to push to this PR branch, a new branch starting from this PR branch is created. #19592 is the new corresponding PR.

@lorentzenchr
Copy link
Copy Markdown
Member

lorentzenchr commented Mar 1, 2021

Continued in #19592.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lorentzenchr lorentzenchr lorentzenchr left review comments

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees

No one assigned

Labels

module:model_selection

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@cycks @reshamas @glemaitre @ogrisel @mohaseeb @lorentzenchr @thomasjpfan