TST: Implement maximally-specific/strict fixtures for arithmetic tests #23734
Conversation
|
Hello @jbrockmendel! Thanks for submitting the PR.
|
Codecov Report
@@ Coverage Diff @@
## master #23734 +/- ##
=========================================
Coverage ? 92.24%
=========================================
Files ? 161
Lines ? 51403
Branches ? 0
=========================================
Hits ? 47417
Misses ? 3986
Partials ? 0
Continue to review full report at Codecov.
|
pandas/tests/arithmetic/conftest.py
Outdated
|
|
||
|
|
||
| @pytest.fixture(params=params) | ||
| def box5_and_tz(request): |
There was a problem hiding this comment.
Do we think this is really needed? (all the timezones) Because with all combinations, you get a quite combinatorial explosion (using this makes a single test into 30 tests)
And if you then further combine it with other fixtures ..
(I don't know how much it is used)
There was a problem hiding this comment.
combinatorial explosion is an entirely reasonable concern.
Note that this fixture is taking the place of existing usages of box[...], tz_naive_fixture, so the net effect on the number of cases is not as big as it might look at first glance. The big benefit is that tests that previously were either skipped or marked with a # FIXME are now explicitly+strictly xfailed. (some of these can be un-xfailed and simplified if/when the issue behind #23730 is fixed)
More generally the timezone fixtures may be leading to more tests than we really need/want, especially given the overhead of pytest collection. Two thoughts on this I've been mulling over:
-
We've gotten rid of a lot of for-loops and replaced them with parametrization/fixtures. Swinging back in the other direction may be worth considerin. Using the
--showlocalsflag will cause the pytest error messages to tell us where in the loop an error occurred, obviating one of the disadvantages of looping. -
Instead of using a fixed set of timezone strings, have a smaller randomized set. We could randomize over the whole space of possible dateutil+pytz+stdlib tzinfos, giving corner cases fewer shadows in which to hide.
There was a problem hiding this comment.
Yes, in general those fixtures are certainly good.
There is for example one place where it is combined with the two_hours fixture (6 different ways for timedelta-like), giving 180 tests.
Maybe more specific for this case: are both 'US/Eastern' and 'Asia/Tokyo' needed? (did we had bugs in the past that were related to that?)
And the same for the dateutil ones: did we have bugs in the past in the arithmetics related to having a dateutil timezone?
There was a problem hiding this comment.
No idea about why tz_naive_fixture includes exactly what it does.
If "the tests are unnecessarily thorough" is the biggest problem we have this week, it's a pretty darn good week.
There was a problem hiding this comment.
so the timezones that are chosen, have been sources of (since fixed) bugs in the past. We have a pretty good representative sample of various edge cases on timezones and i think we have just about the right amount, not too few and not too many, after all its just 2 per timezone library type + UTC + None.
jorisvandenbossche
added
Testing
|
Travis failures are the segfault and a Hypothesis |
|
can you rebase |
pandas/tests/arithmetic/conftest.py
Outdated
|
|
||
|
|
||
| @pytest.fixture(params=params) | ||
| def box5_and_tz(request): |
There was a problem hiding this comment.
so the timezones that are chosen, have been sources of (since fixed) bugs in the past. We have a pretty good representative sample of various edge cases on timezones and i think we have just about the right amount, not too few and not too many, after all its just 2 per timezone library type + UTC + None.
| """ | ||
| return request.param | ||
|
|
||
|
|
||
| # aliases so we can use the same fixture twice in a test |
There was a problem hiding this comment.
these need self-obvious names
| params[n] = param | ||
|
|
||
|
|
||
| @pytest.fixture(params=params) |
There was a problem hiding this comment.
so my comment above was fairly general in nature. I don't think you need to test tz's other than [None, 'UTC'] or maybe use 'US/Eastern' rather than UTC to make sure that no times are shifted.
you actually just care if there IS a timezone or not.
|
Closing until consensus is reached on #23730. |
@jbrockmendel How does this depend on that PR? I thought this PR looked OK to almost merge? (some minor comment of Jeff) |
This PR papers over a problem which that PR solves. (though granted, that one is going nowhere fast) |
|
Ah, because here also the transpose versions of the fixtures are included? |
gets rid of all remaining "FIXME" comments in tests/arithmetic.
Should go after #23681 and #23642 (will need to be rebased)