Skip to content
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

TST: resolve issues with test_constructor_dtype_datetime64 #24868

Merged
merged 10 commits into from
Mar 13, 2019
19 changes: 15 additions & 4 deletions pandas/tests/series/test_constructors.py
Original file line number Diff line number Diff line change
Expand Up @@ -688,10 +688,21 @@ def test_constructor_dtype_datetime64(self):
pytest.raises(TypeError, lambda x: Series(
Series(dates).astype('int') / 1000000, dtype='M8[ms]'))

msg = (r"The 'datetime64' dtype has no unit\. Please pass in"
Copy link
Member

Choose a reason for hiding this comment

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

Hmm is this not intentional to have around still?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is explicitly tested by test_astype_generic_timestamp_no_frequency. however i've now updated this test so that the error message check is more explicit.

Copy link
Member Author

Choose a reason for hiding this comment

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

@WillAyd

i'm guessing that the intended error message for this test would have been originally that raised by

raise TypeError("cannot astype a datetimelike from [{from_dtype}] "
"to [{to_dtype}]".format(from_dtype=arr.dtype,
to_dtype=dtype))

and before the conversion to pytest.raises context manager the test was
pytest.raises(TypeError,
lambda x: Series(dates, dtype='datetime64'))

which would have been raising TypeError: missing 1 required positional argument: 'x'

since there was no message check the tested function raising TypeError would have incorrectly been considered a test pass

so this test was never intended to test for The 'datetime64' dtype has no unit message.

hope that makes it clear!

Copy link
Contributor

Choose a reason for hiding this comment

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

so I DO want this test to remain, can you simply change it so it is testing with dtype='datetime64', IOW it raises.

Copy link
Contributor

Choose a reason for hiding this comment

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

This might actually be a duplicate test (if so, then can remove)

Copy link
Member Author

Choose a reason for hiding this comment

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

so I DO want this test to remain, can you simply change it so it is testing with dtype='datetime64', IOW it raises.

This might actually be a duplicate test (if so, then can remove)

testing with dtype='datetime64' is explicitly tested by another test: test_astype_generic_timestamp_no_frequency.

however within this PR i've also changed test_astype_generic_timestamp_no_frequency so that the error message check is more explicit so that it could be removed here.

Copy link
Contributor

@jreback jreback Jan 26, 2019

Choose a reason for hiding this comment

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

ok, you are going to remove this test? rather, this case?

Copy link
Member Author

Choose a reason for hiding this comment

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

not removing... since it now passes, additionally constructed the expected result and compared.

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, you are going to remove this test? rather, this case?

so have now added some cases to test_constructor_dtype_datetime64 similar to those in test_constructor_dtype_timedelta64 and also added the exception message check to tests/frame/test_dtypes.py

so could probably remove some of the # GH3414 related stuff

r" 'datetime64\[ns\]' instead\.")
with pytest.raises(ValueError, match=msg):
Series(dates, dtype='datetime64')
expected = Series([
datetime(2013, 1, 1),
datetime(2013, 1, 2),
datetime(2013, 1, 3),
])
result = Series(dates, dtype='datetime64[ns]')
tm.assert_series_equal(result, expected)

expected = Series([
pd.NaT,
datetime(2013, 1, 2),
datetime(2013, 1, 3),
])
result = Series([np.nan] + dates[1:], dtype='datetime64[ns]')
tm.assert_series_equal(result, expected)

# invalid dates can be help as object
result = Series([datetime(2, 1, 1)])
Expand Down