search

scikit-learn / scikit-learn

scikit-learn: machine learning in Python
https://scikit-learn.org
BSD 3-Clause "New" or "Revised" License
60.23k stars 25.43k forks source link

Remove the use of assert_raises and assert_raises_regex from the tests #14216

Closed NicolasHug closed 3 years ago

NicolasHug commented 5 years ago

~(Saving this for the upcoming sprints, ideally)~

Let's remove the use of assert_raises, assert_raise_message, and assert_raises_regex.

These should be replaced with the pytest context manager:

with pytest.raises(TheException, match="the expected message"):
   function_call_here()

(no need for match in the case of assert_raises and assert_raise_message I guess).

For contributors: pick one of the modules below, and please comment on this issue saying e.g. "I'm working on cluster/tests", to avoid other contributors choosing the same modules.

You can see all the occurrences of the entries that need to be removed with e.g. git grep "assert_raises" sklearn/ensemble/tests/.

Modules that need cleaning

Some more:

Also:

rth commented 5 years ago

Similarly to https://github.com/scikit-learn/scikit-learn/issues/14215#issuecomment-506878019 might be better to do this with nose2pytest or similar tool, in a few PR by a contributor with some experience.

jnothman commented 5 years ago

Yes, I'd rather it be done with an automated tool if possible.

adrinjalali commented 5 years ago

Sunday afternoon, taking refuge from the heat in an air conditioned cafe, taking a stab at this one. Sounds like fun...

sameshl commented 5 years ago

Can I work on this @NicolasHug or should I wait for the sprint ? I can do all of them if you want.

NicolasHug commented 5 years ago

@sameshl sorry, we ended up deciding not to included this as sprint issues since it can be less error-prone doing this automatically

@adrinjalali I can't remember whether you opened a PR for this?

amueller commented 5 years ago

Do we allow "cleaning" the estimator checks? We have a soft dependency on pytest and that would create a hard dependency on pytest for downstream packages that want to call check_estimator. I would be in favor of doing that but we should make that consciously.

sameshl commented 5 years ago

@NicolasHug I will try doing it with nose2py or some automated tool then.

adrinjalali commented 5 years ago

@sameshl IIRC the ones left are not covered by nose2py. I've already applied that tool, the rest need to be done manually, I think.

sameshl commented 5 years ago

@adrinjalali I am ready to take it up and do it manually.

adrinjalali commented 5 years ago

go ahead then :)

sameshl commented 5 years ago

I am starting with cluster/tests

sameshl commented 5 years ago

@thomasjpfan This issue shouldn't close. I will complete the rest too.

adrinjalali commented 5 years ago

@sameshl It automatically got closed because you said "fixes xxxx" in your PR. You should instead say "related to xxx", or "towards xxx"

sameshl commented 5 years ago

@adrinjalali Yeah, It was my bad. Thanks for pointing it out!

sameshl commented 5 years ago

@NicolasHug Could you please update the checklist for this issue. The following have been completed:

adrinjalali commented 5 years ago

Thanks for the list, done @sameshl

amueller commented 5 years ago

are we splitting this up for ease of reviewing? This can be done with a regex, right? @sameshl are you using a regex or doing this manually?

amueller commented 5 years ago

oh or with nose2pytest?

adrinjalali commented 5 years ago

IIRC I did apply nose2pytest to the whole codebase and it's already in. So the leftovers need to be done in a different way.

rth commented 5 years ago

Yeah nose2pytest doesn't support assert_raises, from the readme,

Some Nose functions can be handled via a global search-replace, so a fixer was not a necessity:

assert_raises: replace with pytest.raises

and

Some Nose functions don't have a one-line assert statement equivalent, they have to remain utility functions:

assert_raises_regex

But it still should be possible to do it with a regex I think?

amueller commented 5 years ago

Their docs say:

Some Nose functions can be handled via a global search-replace, so a fixer was not a necessity:

assert_raises: replace with pytest.raises
assert_warns: replace with pytest.warns

And

Some Nose functions don't have a one-line assert statement equivalent, they have to remain utility functions:

assert_raises_regex
assert_raises_regexp # deprecated by Nose

but I'm pretty sure these can be done with a regex.

amueller commented 5 years ago

lol @rth

sameshl commented 5 years ago

are we splitting this up for ease of reviewing?

Yes. I thought it might be easier this way.

This can be done with a regex, right? @sameshl are you using a regex or doing this manually?

I am not exactly sure about how I might do that with regex. So I am doing it with a couple of vim macros I made myself.

glemaitre commented 5 years ago

Just a note here. We can also replace assert_raise_message. This is the same deal.

sameshl commented 5 years ago

@adrinjalali Another update to the checklist of completed :

rth commented 5 years ago

Another update to the checklist of completed :

Done. Thanks!

Note that sklearn/utils/estimator_checks.py should be excluded, as we can't import pytest therE.

Batalex commented 4 years ago

I am working on utils/tests/.

halsawadi commented 3 years ago

Me and @feras-oughali will be working on sklearn/ensemble/tests/

Haidar13 commented 3 years ago

Me and @stevekola are working on sklearn/neighbors/tests/

mohaseeb commented 3 years ago

Me and @cycks are working on sklearn/model_selection/tests/

YasmeenAlsaedy commented 3 years ago

Me and @abdulelahsm are working on `sklearn/linear_model/tests/`

isaacknjama commented 3 years ago

I am working on sklearn/tests/test_dummy.py with @marenwestermann

ogrisel commented 3 years ago

Running git grep "assert_raises" sklearn/tests/ yields many matches in subfiles. Maybe you want to open a PR just for one such specific subfile first.

azihna commented 3 years ago

working on sklearn/tests/test_base.py

azihna commented 3 years ago

One question can be should we also replace assert_warning_message and assert_no_warnings with pytest.warns(..) syntax instead. Details.

rth commented 3 years ago

Yes, you can replace them as well. Though if the resulting diff is already large, better to do it in a separate PR.

azihna commented 3 years ago

@NicolasHug Can you please update the checklist: sklearn/tests is now completed.

NicolasHug commented 3 years ago

Looks like all modules in my original message have been done. @jeremiedbb , you re-opened this issue recently: was it just waiting for #19864 to be merged, or are there more modules that need some cleaning?

jeremiedbb commented 3 years ago

git grep assert_raises still finds occurences (excluding estimator_checks). Same for assert_raises_regexp and assert_raise_message. I guess some PRs pretended to clean a whole module while only cleaning a subset.

azihna commented 3 years ago

Would you like to keep those for another sprint or should we keep cleaning the remaining? Also, it seems like sklearn/mixture is not included in the original list. Was that intentional?

NicolasHug commented 3 years ago

I think we can just get these done now. I upadted the list with some more stuff

glemaitre commented 3 years ago

+1 with @NicolasHug opinion

azihna commented 3 years ago

@NicolasHug The remaining two in sklearn/tests are:

marenwestermann commented 3 years ago

I'm working on sklearn/metrics/cluster/tests/test_unsupervised.py.

marenwestermann commented 3 years ago

Btw: sklearn/metrics/cluster/tests/test_unsupervised.py appears twice in the list at the top.

azihna commented 3 years ago

All of the remaining open points should be closed when #19999 and #20104 are merged.

azihna commented 3 years ago

@NicolasHug The following files have been cleaned on #20104:

Based on the PR from #20065, test_unsupervised can also be marked as done because only the function names have assert_raises.

azihna commented 3 years ago

@NicolasHug

lorentzenchr commented 3 years ago

I also don't find any left overs. As discussed, the exception is sklearn/utils/estimator_checks.py (and dependendies) where we do not want to depend on pytest.

A Great Thank You to all contributors!

NicolasHug commented 3 years ago

As explained above we should still remove assert_raises etc. from sklearn/utils/tests/test_estimator_checks.py, but use the custom sklearn/utils/_testing.raises util instead of pytest.