isolate scalar Timestamp tests from date_range tests #17957
Conversation
|
|
||
| def test_date_range_timestamp_equiv(self): | ||
| rng = date_range('20090415', '20090519', tz='US/Eastern') | ||
| stamp = rng[0] |
There was a problem hiding this comment.
Given that you use this construct several times across tests (i.e. rng = date_range(...)[0]), you might want to consider making that a separate method.
There was a problem hiding this comment.
Given the large number of irons in the fire at the moment, would you be OK with these comments going into #17652 and being addressed in a follow-up?
There was a problem hiding this comment.
@jbrockmendel : I would prefer to make a push to refactor now, as this is a reorganization PR after all. Save it in the end as a last commit to this PR.
There was a problem hiding this comment.
I would prefer to make a push to refactor now, as this is a reorganization PR after all. Save it in the end as a last commit to this PR.
Alright, let's wrap this thing up! Thanks for taking the time to review.
Given that you use this construct several times across tests (i.e. rng = date_range(...)[0]), you might want to consider making that a separate method.
Seems like replacing a 1-line function call with a 1-line method call doesn't really gain anything, no?
| assert ts == stamp | ||
|
|
||
| def test_date_range_timestamp_equiv_from_datetime_instance(self): | ||
| # This test refers to TestTimestampOps.test_addition_subtraction_types |
There was a problem hiding this comment.
Not sure if comments like these are necessary.
|
|
||
| pytest.raises(ValueError, frequencies.to_offset, '100foo') | ||
|
|
||
| pytest.raises(ValueError, frequencies.to_offset, ('', '')) |
There was a problem hiding this comment.
What are the error messages when you make these method calls? I would consider using tm.assert_raises_regex so that we can check the error message if necessary.
There was a problem hiding this comment.
No idea. This is just cut/paste from test_timestamp.
There was a problem hiding this comment.
Gotcha. Run these lines of code and see what error messages you get. Then we can see whether we should check the message in the test.
There was a problem hiding this comment.
Done, pushing updated...
Codecov Report
@@ Coverage Diff @@
## master #17957 +/- ##
==========================================
- Coverage 91.23% 91.22% -0.02%
==========================================
Files 163 163
Lines 50113 50113
==========================================
- Hits 45723 45714 -9
- Misses 4390 4399 +9
Continue to review full report at Codecov.
|
|
Closes #17958. |
|
these changes are fine. if those 2 non-tests (that were not prefixed correctly) are not easy fixes, you can simply xfail them (and then open an issue so we don't lose track of the fix). |
| # see gh-8254 | ||
| assert False |
There was a problem hiding this comment.
what is the issue here? I guess this test wasn't turned on before.
There was a problem hiding this comment.
Woops, yah. That was an earlier check that these were not getting hit. Changing now.
| @@ -592,7 +593,8 @@ def to_datetime_depr(self): | |||
| result = ts.to_datetime() | |||
| assert result == expected | |||
|
|
|||
| def to_pydatetime_nonzero_nano(self): | |||
| def test_to_pydatetime_nonzero_nano(self): | |||
| assert False | |||
|
looks fine. will merge after we release. |
|
thanks! |
Discussed in #17697
git diff upstream/master -u -- "*.py" | flake8 --diff