-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
TST: lock down timeseries now tests, xref #18666 #18709
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #18709 +/- ##
==========================================
- Coverage 91.6% 91.56% -0.05%
==========================================
Files 153 153
Lines 51273 51273
==========================================
- Hits 46970 46947 -23
- Misses 4303 4326 +23
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #18709 +/- ##
==========================================
- Coverage 91.61% 91.59% -0.02%
==========================================
Files 153 153
Lines 51361 51361
==========================================
- Hits 47053 47046 -7
- Misses 4308 4315 +7
Continue to review full report at Codecov.
|
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.
some comments
@@ -211,7 +211,7 @@ def _test_parse_iso8601(object ts): | |||
if ts == 'now': | |||
return Timestamp.utcnow() | |||
elif ts == 'today': | |||
return Timestamp.utcnow().normalize() |
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.
side issue, do we really need this actual test routine? can't we just inspect the actual output?
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.
Best guess is that this is intended to be double-sure that a test specifically hits string_to_dts (since coverage is hard with cython). I'd have no real objection to losing this function.
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.
yeah i think ok to remove
npnow = np.datetime64('now').astype('datetime64[ns]') | ||
pdnow = pd.to_datetime('now') | ||
pdnow2 = pd.to_datetime(['now'])[0] | ||
# These should all be equal with infinite perf |
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.
give a little room, I have seen flakiness with these kinds of tests, maybe < 1e7
or something
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.
Confused. You want more room or less room? 1e7 < 1e10.
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.
nvm. you are comparing for 10s here. maybe add a comment.
nptoday = np.datetime64('today').astype('datetime64[ns]') | ||
pdtoday = pd.to_datetime('today') | ||
pdtoday2 = pd.to_datetime(['today'])[0] | ||
# These should all be equal with infinite perf |
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.
same & add a blank line
failing on windows. |
Weird since only comment/whitespace change since last run. Error is in pandas.util.testing, may just have to skip on windows. |
thanks! |
See the comment attached to
test_to_datetime_today
. This test is not deterministic, will fail to detect the change introduced by #18666 1 hour out of each day.git diff upstream/master -u -- "*.py" | flake8 --diff