-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
Separate tick tests, use pytest parametrize #18233
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
Separate tick tests, use pytest parametrize #18233
Conversation
pandas/tests/tseries/test_offsets.py
Outdated
assert_func(result2, exp) | ||
@pytest.mark.parametrize('klass', [Series, DatetimeIndex]) | ||
def test_vectorized_offset_addition(self, klass): | ||
if klass is 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.
pass in the correct comparator method as another arg
Codecov Report
@@ Coverage Diff @@
## master #18233 +/- ##
==========================================
- Coverage 91.43% 91.41% -0.02%
==========================================
Files 164 163 -1
Lines 50091 50091
==========================================
- Hits 45800 45791 -9
- Misses 4291 4300 +9
Continue to review full report at Codecov.
|
pandas/tests/tseries/test_ticks.py
Outdated
tick_classes = [Hour, Minute, Second, Milli, Micro, Nano] | ||
|
||
|
||
def assertEq(offset, base, expected): |
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.
put this in a common.py and import here
don't use camelCase ever! and spell it out
assert_offset_equal
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.
don't use camelCase ever! and spell it out
I agree, this is taken from an existing version in test_offsets. I'll move and rename both.
(Need to get rid of a whole bunch of camelCase in offsets, but that's gonna have to wait until the non-cosmetic stuff gets through).
pandas/tests/tseries/test_ticks.py
Outdated
|
||
|
||
class TestTicks(object): | ||
def test_tick_addition(self): |
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.
parametrize
pandas/tests/tseries/test_ticks.py
Outdated
# --------------------------------------------------------------------- | ||
|
||
|
||
class TestTicks(object): |
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 don't really like classes to be honest, rather than them at the top-level. I would only do this if you have a small number of different things you are testing that don't warrant a module each
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 tend to agree. For now I'm just separating these out because Ticks are going to require a bunch of separate treatment (and the test_offsets file is pretty unwieldy at ~4500 lines)
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.
again, instead of doing this, just make a another level and put things in modules, then all happy, e.g.
pandas/tests/tseries/offsets/test_*
is simplea and clear (and you can even push the conftest to this directly to make it even more obvious).
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.
OK. Note that the TestTick class is not my doing, is just moved.
pandas/tests/tseries/test_ticks.py
Outdated
"\nAt Date: %s" % | ||
(expected, actual, offset, base)) | ||
|
||
# --------------------------------------------------------------------- |
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.
these lines are fine, put a comment (note that I find this much more readable than classes).
|
||
for offset, dt, expected in tests: | ||
assertOnOffset(offset, dt, expected) | ||
offset_cases = [] |
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 would really make these a fixture
pandas/tests/tseries/test_offsets.py
Outdated
@pytest.mark.parametrize('case', on_offset_cases) | ||
def test_onOffset(self, case): | ||
offset, dt, expected = case | ||
assertOnOffset(offset, dt, expected) |
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.
note sure where this is defined, should be assert_OnOffset
Implemented most of the requests. Left some of the parametrization and fixturization for later. In working with the offsets module I'm finding more bugs (e.g #18235). I'd like to wrap this up for now, fix+test some of those, then circle back to this. |
pls do this: #18233 (comment) |
you need to add the new dir in setup.py (test section) |
need to move the |
Ha that’s a polite way of pointing out a screwup. My bad, will rectify. |
so you also need to modify |
ping on green. |
thanks! |
The test output for offsets would be more useful of the tests were parametrized. So this PR implements that for about a third of the tests.