-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
STY: use pytest.raises context syntax (series) #24812
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #24812 +/- ##
===========================================
- Coverage 92.38% 42.93% -49.46%
===========================================
Files 166 166
Lines 52382 52382
===========================================
- Hits 48395 22488 -25907
- Misses 3987 29894 +25907
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #24812 +/- ##
=======================================
Coverage 92.38% 92.38%
=======================================
Files 166 166
Lines 52382 52382
=======================================
Hits 48395 48395
Misses 3987 3987
Continue to review full report at Codecov.
|
# with pytest.raises(TypeError, match=msg): | ||
Series(Series(dates).astype('int') / 1000000, dtype='M8[ms]') | ||
# Series(Series(dates).astype('int') / 1000000, dtype='M8[ms]') | ||
pytest.raises(TypeError, lambda x: Series( |
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.
this is raising missing 1 required positional argument: 'x'
because of the lambda and not failing as intended. correcting this with the conversion to context manger is only failing on windows and again from the error message is probably not failing as intended
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.
this should be raising TypeError, what happens when you use the context manager
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.
it is raising TypeError in master because of the lambda. this is a bad test.
correcting it with the context manager raises TypeError: cannot astype a datetimelike from [datetime64[ns]] to [int32]
but only on the windows tests (see ci failures)
i've reverted the changes to the test, but the ci is failing on an unrelated conda error at the moment.
with the changes in this PR there will still be 823 uses of pytest.raises without the cm. so i don't want to get bogged down with bad tests at the moment.
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.
k that's fine (though happy to take your change, though it should raise on windows as well). can you create an issue about this.
thanks! |
msg = (r"The 'datetime64' dtype has no unit\. Please pass in" | ||
r" 'datetime64\[ns\]' instead\.") | ||
with pytest.raises(ValueError, match=msg): | ||
Series(dates, dtype='datetime64') |
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 think this test is also not failing as intended and was masked by another lambda error. will do a follow-up PR to address theses cases.
xref #24332