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.
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.
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.
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): |
pandas/tests/tseries/test_ticks.py
Outdated
| # --------------------------------------------------------------------- | ||
|
|
||
|
|
||
| class TestTicks(object): |
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
note sure where this is defined, should be assert_OnOffset
jreback
added
Frequency
|
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.