TST: bare pytest raises in tests/scalar by quangngd · Pull Request #32929 · pandas-dev/pandas (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation25 Commits8 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
- ref [Good first issue] TST: Disallow bare pytest.raises #30999
- tests added / passed
- passes
black pandas - passes
git diff upstream/master -u -- "*.py" | flake8 --diff - whatsnew entry
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for taking some of these!
I have some minor comments
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @quangngd for the PR. generally looks good on first pass. in future can you limit the changes to batches of no more than 5 files.
| @@ -225,15 +231,20 @@ def test_constructor_positional(self): |
|---|
| def test_constructor_keyword(self): |
| # GH 10758 |
| with pytest.raises(TypeError): |
| msg = "function missing required argument 'day'|Required argument 'day'" |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you know where these inconsistencies arise?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we only have function missing required argument 'day'
it would only pass the Linux py36_locale_slow_old_np and Linux py37_locale azure pipeline
The error was raised by pandas/_libs/tslibs/timestamps.pyx:467
ts_input = datetime(**datetime_kwargs)
Maybe it's a python thing?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know all of our pipeline run on py 3 but when looking around, i noticed that when run
Python 3 (3.7.3) would returns TypeError: open() missing required argument 'file' (pos 1) and
Python 2 (2.7.17) would returns TypeError: Required argument 'name' (pos 1) not found
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, this is an indication that the message is not raised by pandas. what's the policy here @MomIsBestFriend?
| @@ -26,14 +26,14 @@ def test_tz_localize_pushes_out_of_bounds(self): |
|---|
| pac = Timestamp.min.tz_localize("US/Pacific") |
| assert pac.value > Timestamp.min.value |
| pac.tz_convert("Asia/Tokyo") # tz_convert doesn't change value |
| with pytest.raises(OutOfBoundsDatetime): |
| with pytest.raises(OutOfBoundsDatetime, match="^$"): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it make sense to have a message here?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the current implementation returns nothing, it's indeed should better have some message but that's out of the issue's scope
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, out of scope. but if you could raise an issue, that'll be great.
| Timestamp("2015-10-25 02:00", tz=tz) |
|---|
| result = Timestamp("2017-03-26 01:00", tz="Europe/Paris") |
| expected = Timestamp("2017-03-26 01:00").tz_localize("Europe/Paris") |
| assert result == expected |
| with pytest.raises(pytz.NonExistentTimeError): |
| msg = "2017-03-26 02:00" |
| with pytest.raises(pytz.NonExistentTimeError, match=msg): |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok to inline short, single use messages
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @quangngd lgtm. @MomIsBestFriend ok to merge?
Labels
pandas testing functions or related to the test suite