-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
TST: insert 'match' to bare pytest raises in pandas/tests/internals/ #30998
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
Changes from 5 commits
f4b0fc4
fc18122
e5a61cd
dcc1be8
1f50d45
05d28c4
9aeae8c
d60be3c
60bed8d
aa46a8e
bff555e
ae8494d
1416011
041d1b5
94be866
f22fba6
a8be057
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -297,7 +297,10 @@ def test_delete(self): | |
assert (newb.values[1] == 1).all() | ||
|
||
newb = self.fblock.copy() | ||
with pytest.raises(Exception): | ||
|
||
msg = "index 3 is out of bounds for axis 0 with size 3" | ||
|
||
with pytest.raises(IndexError, match=msg): | ||
newb.delete(3) | ||
|
||
|
||
|
@@ -321,7 +324,12 @@ def test_can_hold_element(self): | |
|
||
val = date(2010, 10, 10) | ||
assert not block._can_hold_element(val) | ||
with pytest.raises(TypeError): | ||
|
||
msg = ( | ||
"'value' should be a 'Timestamp', 'NaT', " | ||
"or array of those. Got 'date' instead." | ||
) | ||
with pytest.raises(TypeError, match=msg): | ||
arr[0] = val | ||
|
||
|
||
|
@@ -350,7 +358,10 @@ def test_duplicate_ref_loc_failure(self): | |
blocks[1].mgr_locs = np.array([0]) | ||
|
||
# test trying to create block manager with overlapping ref locs | ||
with pytest.raises(AssertionError): | ||
|
||
msg = "Gaps in blk ref_locs" | ||
|
||
with pytest.raises(AssertionError, match=msg): | ||
BlockManager(blocks, axes) | ||
|
||
blocks[0].mgr_locs = np.array([0]) | ||
|
@@ -807,8 +818,15 @@ def test_validate_bool_args(self): | |
invalid_values = [1, "True", [1, 2, 3], 5.0] | ||
bm1 = create_mgr("a,b,c: i8-1; d,e,f: i8-2") | ||
|
||
msg = ( | ||
r'For argument "inplace" expected type bool, received type int.|' | ||
r'For argument "inplace" expected type bool, received type str.|' | ||
r'For argument "inplace" expected type bool, received type list.|' | ||
r'For argument "inplace" expected type bool, received type float.' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe leave out the last word to have this less repetitive? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At a second look, I think I will just have msg = "For argument "inplace" expected type bool, received type (int|str|list|float)." Will this be better? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that should work. Give it a shot. |
||
) | ||
|
||
for value in invalid_values: | ||
with pytest.raises(ValueError): | ||
with pytest.raises(ValueError, match=msg): | ||
bm1.replace_list([1], [2], inplace=value) | ||
|
||
|
||
|
@@ -1027,9 +1045,11 @@ def test_slice_len(self): | |
assert len(BlockPlacement(slice(1, 0, -1))) == 1 | ||
|
||
def test_zero_step_raises(self): | ||
with pytest.raises(ValueError): | ||
msg = "slice step cannot be zero" | ||
|
||
with pytest.raises(ValueError, match=msg): | ||
BlockPlacement(slice(1, 1, 0)) | ||
with pytest.raises(ValueError): | ||
with pytest.raises(ValueError, match=msg): | ||
BlockPlacement(slice(1, 2, 0)) | ||
|
||
def test_unbounded_slice_raises(self): | ||
|
@@ -1132,9 +1152,11 @@ def assert_add_equals(val, inc, result): | |
assert_add_equals(slice(1, 4), -1, [0, 1, 2]) | ||
assert_add_equals([1, 2, 4], -1, [0, 1, 3]) | ||
|
||
with pytest.raises(ValueError): | ||
msg = "iadd causes length change" | ||
|
||
with pytest.raises(ValueError, match=msg): | ||
BlockPlacement(slice(1, 4)).add(-10) | ||
with pytest.raises(ValueError): | ||
with pytest.raises(ValueError, match=msg): | ||
BlockPlacement([1, 2, 4]).add(-10) | ||
|
||
|
||
|
@@ -1216,7 +1238,17 @@ def test_binop_other(self, op, value, dtype): | |
} | ||
|
||
if (op, dtype) in invalid: | ||
with pytest.raises(TypeError): | ||
msg = ( | ||
r"cannot perform __pow\_\_ with this index type: DatetimeArray|" | ||
r"cannot perform __mod\_\_ with this index type: DatetimeArray|" | ||
r"cannot perform __truediv\_\_ with this index type: DatetimeArray|" | ||
r"cannot perform __mul\_\_ with this index type: DatetimeArray|" | ||
r"cannot perform __pow\_\_ with this index type: TimedeltaArray|" | ||
"ufunc 'multiply' cannot use operands with types dtype" | ||
r"\('<m8\[ns\]'\) and dtype\('<m8\[ns\]'\)|" | ||
"cannot add DatetimeArray and Timestamp" | ||
gfyoung marked this conversation as resolved.
Show resolved
Hide resolved
|
||
) | ||
with pytest.raises(TypeError, match=msg): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So in light of the discussion in #30999. Personally, I find that the above code change makes the test much more complicated and more difficult to read and interpret. There is always a trade-off between testing every exact details vs practicality/readabiity/effort to do this, and personally for a case like the above, this is not worth it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks hard to read because it's so monolithic. This test could benefit from a TON of refactoring, including There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@gfyoung I am looking into refactoring this test from the ground up, what would you put as the pytest parameterization(s) ? I hope I can build the test from the ground up, depends on the given parameterization(s). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I might have jumped the gun there in saying we should do A similar point could be made about the combinations we expect to fail because I'm uncertain as to feasability. If we could somehow parameterize properly, we could easily break that part out as a separate test. |
||
op(s, e.value) | ||
else: | ||
# FIXME: Since dispatching to Series, this test no longer | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should test this, this is an internal error message
(and it's also not a very good error message)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that exactly the reason why we should test them in the first place? Puts more light on the quality of the error messages that we raise, internal or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we, in one go, improve the error message. Yes, that is fine, but otherwise I don't think it is worth the effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debatable. If you're someone who has just started working on this codebase, I wouldn't want them to also modify the error message if they don't fully understand what is going. You can easily spend another PR figuring out what the proper error message is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @jorisvandenbossche here I don't think worth testing against error messages for
AssertionError
, since that is purely internalThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't find that to be valid justification. Internal or not, we use error messages for a reason: because they provide some utility in development. If these error messages (not the
Exception
class itself) had no use and won't see the light of day simply because they're internal, we should just get rid of them. To borrow from @jorisvandenbossche, they aren't worth the effort then.Error messages can also serve to differentiate between which error is raised. The
managers.py
file is littered withAssertionError
lines. It would be good to know that the correct one is being raised.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am actually ok with testing these asserts; they are the current behavior. If things change then this test should fail. That said I wouldn; spend too much effort on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, they are useful for that. But also because they are only used in development, they can also easily change when refactoring some internals (without that there are user-facing consequences).
But every test (and certainly a test with a match) takes also effort to write, review and maintain (eg update if you update something internally). It's a trade-off. So I am not saying those internal error messages are not useful at all, but I still think they are not worth the effort to properly test with full coverage.