MAINT:Fix assert raises in sklearn/tree/tests/#14737
MAINT:Fix assert raises in sklearn/tree/tests/#14737rth merged 6 commits intoscikit-learn:masterfrom sameshl:tests_tree
sklearn/tree/tests/#14737Conversation
|
As a general comment, could you remove |
glemaitre
left a comment
There was a problem hiding this comment.
Apart of the assert_raises_message, LGTM.
| # use values of max_features that are invalid | ||
| est = TreeEstimator(max_features=10) | ||
| assert_raises(ValueError, est.fit, X, y) | ||
| with pytest.raises(ValueError): |
There was a problem hiding this comment.
We should open an issue to refactor theses tests as well to use parametrization.
It might be easier to read. But let's keep it for another PR.
There was a problem hiding this comment.
Yes, that is a good point. We should do that
|
@glemaitre Can you help me figure out why the tests are failing in build? It doesn't look like it is due to a change I made. Thanks! |
sklearn/tree/tests/test_tree.py
Outdated
| "Got {!r} instead.".format(allowed_presort, invalid_presort)) | ||
| est = cls(presort=invalid_presort) | ||
| assert_raise_message(ValueError, msg, est.fit, X, y) | ||
| with pytest.raises(ValueError, match=msg): |
There was a problem hiding this comment.
this one fails because:
The regexp parameter of the match method is matched with the re.search function, so in the above example match='123' would have worked as well.
whereas before it was a simple string comparison, I think.
There was a problem hiding this comment.
@adrinjalali So can you tell me the correct way of writing this case using pytest.raises?
There was a problem hiding this comment.
In this case:
with pytest.raises(ValueError, match=msg.replace('(', '\(').replace(')', '\)')):would work, but I'm not sure if it's the most elegant solution.
There was a problem hiding this comment.
Indeed a bit cumbersome, but nothing better comes immediatiely to mind.
|
@glemaitre I have made the required changes. I also resolved the matching conflict as suggested by @adrinjalali here. Let me know if any other changes are required. |
|
Thanks @rth! |
replaced
assert_raisesandassert_raises_regexwithpytest.raisescontext manager.related to #14216