Skip to content
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

TST: Remove bare pytest.raises #31079

Merged
merged 11 commits into from
Jan 24, 2020
Merged

Conversation

gdex1
Copy link
Contributor

@gdex1 gdex1 commented Jan 16, 2020

References #30999

Adds match argument to pytest.raises found in the following files. #30999 (comment)

ToDo:

  • pandas/tests/arithmetic/test_numeric
  • pandas/tests/arithmetic/test_object
  • pandas/tests/arithmetic/test_timedelta64
  • pandas/tests/arrays/interval/test_interval
  • add whatsnew entry

@pep8speaks
Copy link

pep8speaks commented Jan 16, 2020

Hello @gdex1! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-01-21 15:20:05 UTC

pandas/tests/arithmetic/test_numeric.py Outdated Show resolved Hide resolved
pandas/tests/arithmetic/test_numeric.py Show resolved Hide resolved
pandas/tests/arithmetic/test_timedelta64.py Outdated Show resolved Hide resolved
pandas/tests/arithmetic/test_object.py Show resolved Hide resolved
@simonjayhawkins simonjayhawkins added Testing pandas testing functions or related to the test suite Clean labels Jan 17, 2020
@@ -281,6 +282,8 @@ def test_add(self):

def test_sub_fail(self):
index = tm.makeStringIndex(100)

# TODO: Make raised error message more informative and test
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for the TODO here instead of adding the message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WillAyd . I was not sure what a good error message for this function would be. Does the error message added in my previous commit work?

@gdex1
Copy link
Contributor Author

gdex1 commented Jan 21, 2020

Could someone give me some direction on resolving the failed integration tests? I do not understand how changing a string to an f-string in my latest commit would cause the error:
Git checkout failed with exit code: 128.

@WillAyd
Copy link
Member

WillAyd commented Jan 21, 2020

I think GitHub CI was temporarily down; just restarted let's see

Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@WillAyd WillAyd added this to the 1.1 milestone Jan 21, 2020
@gdex1 gdex1 requested a review from jreback January 23, 2020 13:52
@jreback jreback merged commit 14f1982 into pandas-dev:master Jan 24, 2020
@jreback
Copy link
Contributor

jreback commented Jan 24, 2020

thanks @gdex1

@gdex1
Copy link
Contributor Author

gdex1 commented Jan 24, 2020

@MomIsBestFriend . I've finished adding match arguments for pytest.raises calls in these files. Should I add an entry to the "whatsnew" file? Or will there be one added later related to the original issue you opened? Thanks.

@ShaharNaveh
Copy link
Member

@gdex1 First, thank you for contributing:)

And the "what's new" is a file that's displayed to users who wants to learn what's changed.

the tests are for us to test the code, the external users do not use these tests and they also don't see these tests, so no you should not create a what's new file.

And there will be no new entry for the related issue :)


Side note:
Don't be afraid to do mistakes (that's how you learn).
I'm sure, if you open a PR (related to this issue) with a what's new entry, you'll receive feedback (and better one).


:)

@gdex1 gdex1 deleted the Disallow-Bare-Pytest branch January 27, 2020 04:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Clean Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants