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: Fix some bare pytest raises #31105

Merged
merged 5 commits into from
Jan 17, 2020

Conversation

ShaharNaveh
Copy link
Member

@ShaharNaveh ShaharNaveh commented Jan 17, 2020

Follow up for #30998

@ShaharNaveh ShaharNaveh force-pushed the TST-refactored-internals branch from 5ff8410 to c4f5f1d Compare January 17, 2020 15:10
@WillAyd
Copy link
Member

WillAyd commented Jan 17, 2020

@MomIsBestFriend might be misreading but I don't really understand this PR. Is this de-parametrizing an existing test?

@ShaharNaveh
Copy link
Member Author

@MomIsBestFriend might be misreading but I don't really understand this PR. Is this de-parametrizing an existing test?

Correct, because I had troubles in #30998 to put a correct "match" value (trying to remove bare pytest raises statement).

I was having something that looked kinda like this:

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"
         )

(That value did not work)

I think, IMO the "de-parameterization" functions is more readable and maintainable (And I was able to fix the bare pytests raises statement).

@WillAyd
Copy link
Member

WillAyd commented Jan 17, 2020

I think should keep the original parametrization. The error messages are of secondary importance so don't need to be exact. You can just pick a simpler pattern like cannot (perform|add)

@ShaharNaveh
Copy link
Member Author

ShaharNaveh commented Jan 17, 2020

I think should keep the original parametrization. The error messages are of secondary importance so don't need to be exact. You can just pick a simpler pattern like cannot (perform|add)

I know that msg (#31105 (comment)) can be prettier using some regex "magic".
The problem is that the parameterized test is also raising an external error message, which can be a bit trickier to "ignore" that specific message, thing that will probably that there will be the need for some nested if statements.


I will commit the parameterized test, with the nested if statement(s).
And just pick the one you like more :)

Also, side question, does the parameterized test is more performant? (trying to see why the de-parameterized test is less likeable)

@WillAyd
Copy link
Member

WillAyd commented Jan 17, 2020

Also, side question, does the parameterized test is more performant?

I don't think it matters but even if it did should strongly prefer parametrized tests to breaking out individually; the latter is tougher to extend and maintain over time and leads to more lapses in coverage

@ShaharNaveh
Copy link
Member Author

@WillAyd You were 100% right!
it's just one if statement, and it does looks much better than the de-parameterized tests

@ShaharNaveh ShaharNaveh changed the title TST/REF: Fix some bare pytest raises TST: Fix some bare pytest raises Jan 17, 2020
pandas/tests/internals/test_internals.py Outdated Show resolved Hide resolved
@WillAyd WillAyd added the Testing pandas testing functions or related to the test suite label Jan 17, 2020
@ShaharNaveh ShaharNaveh reopened this Jan 17, 2020
@ShaharNaveh
Copy link
Member Author

(unrelated)
If someone mention you in a regular comment of a PR/issue that was merge/closed, would you still get notified?

@WillAyd WillAyd added this to the 1.1 milestone Jan 17, 2020
@WillAyd WillAyd merged commit 641346c into pandas-dev:master Jan 17, 2020
@WillAyd
Copy link
Member

WillAyd commented Jan 17, 2020

Thanks @MomIsBestFriend and answer to your question depends on per-user settings

@ShaharNaveh ShaharNaveh deleted the TST-refactored-internals branch January 18, 2020 14:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants