Skip to content

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

Merged
merged 11 commits into from
Nov 13, 2017

Conversation

jbrockmendel
Copy link
Member

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.

assert_func(result2, exp)
@pytest.mark.parametrize('klass', [Series, DatetimeIndex])
def test_vectorized_offset_addition(self, klass):
if klass is Series:
Copy link
Contributor

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
Copy link

codecov bot commented Nov 12, 2017

Codecov Report

Merging #18233 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Flag Coverage Δ
#multiple 89.22% <ø> (ø) ⬆️
#single 40.39% <ø> (-0.04%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 25% <0%> (-58.34%) ⬇️
pandas/core/reshape/reshape.py 99.27% <0%> (-0.73%) ⬇️
pandas/core/frame.py 97.8% <0%> (-0.1%) ⬇️
pandas/core/api.py 100% <0%> (ø) ⬆️
pandas/tseries/offsets.py 97.11% <0%> (ø) ⬆️
pandas/core/indexes/multi.py 96.39% <0%> (ø) ⬆️
pandas/core/reshape/melt.py
pandas/core/categorical.py 95.79% <0%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40fd6b4...bc4147d. Read the comment docs.

tick_classes = [Hour, Minute, Second, Milli, Micro, Nano]


def assertEq(offset, base, expected):
Copy link
Contributor

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

Copy link
Member Author

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



class TestTicks(object):
def test_tick_addition(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

parametrize

# ---------------------------------------------------------------------


class TestTicks(object):
Copy link
Contributor

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

Copy link
Member Author

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)

Copy link
Contributor

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

Copy link
Member Author

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.

"\nAt Date: %s" %
(expected, actual, offset, base))

# ---------------------------------------------------------------------
Copy link
Contributor

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 = []
Copy link
Contributor

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

@pytest.mark.parametrize('case', on_offset_cases)
def test_onOffset(self, case):
offset, dt, expected = case
assertOnOffset(offset, dt, expected)
Copy link
Contributor

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

@jreback jreback added Frequency DateOffsets Testing pandas testing functions or related to the test suite labels Nov 12, 2017
@jbrockmendel
Copy link
Member Author

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.

@jreback
Copy link
Contributor

jreback commented Nov 12, 2017

pls do this: #18233 (comment)

@jreback
Copy link
Contributor

jreback commented Nov 12, 2017

you need to add the new dir in setup.py (test section)

@jreback
Copy link
Contributor

jreback commented Nov 12, 2017

need to move the pandas/tests/tseries/data dir to offsets. you can test locally on these to find these issue before pushing FYI.

@jbrockmendel
Copy link
Member Author

Ha that’s a polite way of pointing out a screwup. My bad, will rectify.

@jreback
Copy link
Contributor

jreback commented Nov 13, 2017

so you also need to modify setup.py to reflect the moved data dir; the build test checks this.

@jreback jreback added this to the 0.22.0 milestone Nov 13, 2017
@jreback
Copy link
Contributor

jreback commented Nov 13, 2017

ping on green.

@jreback jreback merged commit 7f4c960 into pandas-dev:master Nov 13, 2017
@jreback
Copy link
Contributor

jreback commented Nov 13, 2017

thanks!

No-Stream pushed a commit to No-Stream/pandas that referenced this pull request Nov 28, 2017
@jbrockmendel jbrockmendel deleted the tslibs-offsets-tests branch December 8, 2017 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Frequency DateOffsets 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