-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
TST: parameterize more tests #45131
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: parameterize more tests #45131
Conversation
mroeschke
commented
Dec 30, 2021
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
@jbrockmendel if you can have a quick glance here |
@@ -1810,55 +1810,50 @@ def test_dt64ser_sub_datetime_dtype(self): | |||
# TODO: This next block of tests came from tests.series.test_operators, | |||
# needs to be de-duplicated and parametrized over `box` classes | |||
|
|||
def test_operators_datetimelike_invalid(self, all_arithmetic_operators): | |||
# these are all TypeEror ops | |||
@pytest.mark.parametrize( |
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 dont think this improves the clarity of this test. id suggest something like:
def test_whatever(all_arithmetic_operators):
op_str = all_arithmetic_operators
if op_str in [add, sub, radd, rsub]:
return # or skip, or use a different fixture; these are tested elsewhere
tdser = [timedelta(minutes=5, seconds=3), NaT, timedelta(minutes=5, seconds=3)]
dt64ser = [Timestamp("20111230"), Timestamp("20120101"), NaT]
dt64tz_ser = ...
for iter_over_pairs([tdser, dt64ser, dt64tz_ser]):
with pytest.raises...
op(whatever)
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.
Hmm at least with this version an error with any of the dtlike-ser
won't prevent the other dtlike-ser
from being tested due to the parameterize
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.
Fair enough. I might be overly-wary of over-parametrizing in the arithmetic tests since there's a ton of them
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.
Hopefully with a second pass we can remove the need for this test entirely.
comments on a few, the rest look good |
@@ -58,6 +58,17 @@ def adjust_negative_zero(zero, expected): | |||
return expected | |||
|
|||
|
|||
def compare_op(series, other, op): |
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.
something like this you might want o make a first class assert_* function (but ok for now)
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.
thanks @mroeschke pls followup as needed