TST: resolve issues with test_constructor_dtype_datetime64 #24868

simonjayhawkins · 2019-01-21T21:12:00Z

while changing pytest.raises to use the context manager and check the error message it appeared that a test was not raising exceptions as originally intended.

the test was related to a legacy numpy related issue #3414

WillAyd · 2019-01-21T21:27:31Z

pandas/tests/series/test_constructors.py

@@ -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"


Hmm is this not intentional to have around still?

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.

@WillAyd

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

pandas/pandas/core/dtypes/cast.py

Lines 644 to 646 in af82b2a

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

pandas/pandas/tests/series/test_constructors.py

Lines 680 to 681 in 17a6bc5

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!

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)

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.

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

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

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

codecov · 2019-01-21T21:45:54Z

Codecov Report

Merging #24868 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #24868   +/-   ##
=======================================
  Coverage   92.38%   92.38%           
=======================================
  Files         166      166           
  Lines       52412    52412           
=======================================
  Hits        48423    48423           
  Misses       3989     3989

Flag	Coverage Δ
#multiple	`90.81% <ø> (ø)`	⬆️
#single	`42.89% <ø> (ø)`	⬆️

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 12bb6d0...5418c75. Read the comment docs.

codecov · 2019-01-21T21:45:55Z

Codecov Report

Merging #24868 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #24868      +/-   ##
==========================================
- Coverage   91.25%   91.25%   -0.01%     
==========================================
  Files         172      172              
  Lines       52963    52963              
==========================================
- Hits        48330    48329       -1     
- Misses       4633     4634       +1

Flag	Coverage Δ
#multiple	`89.82% <ø> (ø)`	⬆️
#single	`41.74% <ø> (ø)`	⬆️

Impacted Files	Coverage Δ
pandas/util/testing.py	`88.98% <0%> (-0.1%)`	⬇️

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 86879ac...13e364b. Read the comment docs.

jreback · 2019-01-22T12:14:33Z

pandas/tests/series/test_constructors.py

@@ -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"


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

WillAyd · 2019-02-12T05:39:53Z

Can you merge master and address latest comments?

Also, it may just be me but I would find it super helpful going forward if you could use a more descriptive title here and for future PRs. We have a rather large backlog of PRs and issues so if it's not descriptive it's more work than it needs to be on the review end to keep track of what the PR is trying to address

WillAyd · 2019-02-19T04:16:53Z

Closing as stale - ping if you'd like to continue

simonjayhawkins · 2019-03-12T19:50:18Z

@WillAyd : i'll need to resolve this at some point before we can activate the code checks for pytest.raises not been used as a context manager.

This PR was closely related to #24827. There is no longer any activity there. So i'll include that here and change the title to relate to that issue.

So if you could reopen this PR that'll be great. Thanks.

WillAyd · 2019-03-12T19:59:32Z

@simonjayhawkins done!

simonjayhawkins · 2019-03-12T20:06:57Z

thanks. @WillAyd. I believe the comments have been addressed. will merge master and uncomment the current solution to #24827 which i believe @jreback was happy with, see #24812 (comment) and use np.intp to resolve the windows issue. Then see where we go from there.

jreback · 2019-03-13T15:45:10Z

thanks @simonjayhawkins note that the test itself is pretty long (not your change), so can consider breaking it up a bit in the future.

* master: (22 commits) Fixturize tests/frame/test_operators.py (pandas-dev#25641) Update ValueError message in corr (pandas-dev#25729) DOC: fix some grammar and inconsistency issues in the User Guide (pandas-dev#25728) ENH: Add public start, stop, and step attributes to RangeIndex (pandas-dev#25720) Make Rolling.apply documentation clearer (pandas-dev#25712) pandas-dev#25707 - Fixed flakiness in stata write test (pandas-dev#25714) Json normalize nan support (pandas-dev#25619) TST: resolve issues with test_constructor_dtype_datetime64 (pandas-dev#24868) DEPR: Deprecate box kwarg for to_timedelta and to_datetime (pandas-dev#24486) BUG: Preserve name in DatetimeIndex.snap (pandas-dev#25585) Fix concat not respecting order of OrderedDict (pandas-dev#25224) CLN: remove pandas.core.categorical (pandas-dev#25655) TST/CLN: Remove more Panel tests (pandas-dev#25675) Pinned pycodestyle (pandas-dev#25701) DOC: update date of 0.24.2 release notes (pandas-dev#25699) BUG: Fix error in replace with strings that are large numbers (pandas-dev#25616) (pandas-dev#25644) BUG: fix usage of na_sentinel with sort=True in factorize() (pandas-dev#25592) BUG: Fix to_string output when using header (pandas-dev#16718) (pandas-dev#25602) CLN: Remove unused test code (pandas-dev#25670) CLN: remove Panel from concat error message (pandas-dev#25676) ... # Conflicts: # doc/source/whatsnew/v0.25.0.rst

simonjayhawkins added 2 commits January 21, 2019 19:06

np bug no more

4376036

add np.nan case

5418c75

WillAyd reviewed Jan 21, 2019

View reviewed changes

WillAyd added Testing pandas testing functions or related to the test suite Datetime Datetime data dtype labels Jan 21, 2019

update test_astype_generic_timestamp_no_frequency

b31f171

jreback requested changes Jan 22, 2019

View reviewed changes

WillAyd closed this Feb 19, 2019

simonjayhawkins changed the title ~~TST: follow-up to #24812~~ [WIP] TST: resolve issues with test_constructor_dtype_datetime64 Mar 12, 2019

WillAyd reopened this Mar 12, 2019

simonjayhawkins added 7 commits March 12, 2019 20:23

Merge remote-tracking branch 'upstream/master' into series-np-bug

eda7576

troubleshoot - results of ci no longer available

57962f7

astype('int') fails on windows

89e8758

Merge remote-tracking branch 'upstream/master' into series-np-bug

75db09b

add astype exception message checks

968f048

fix msg in test_astype_to_incorrect_datetimelike

6425dcd

Merge remote-tracking branch 'upstream/master' into series-np-bug

13e364b

simonjayhawkins changed the title ~~[WIP] TST: resolve issues with test_constructor_dtype_datetime64~~ TST: resolve issues with test_constructor_dtype_datetime64 Mar 13, 2019

jreback added this to the 0.25.0 milestone Mar 13, 2019

jreback approved these changes Mar 13, 2019

View reviewed changes

jreback merged commit 69ae24b into pandas-dev:master Mar 13, 2019

simonjayhawkins deleted the series-np-bug branch March 13, 2019 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TST: resolve issues with test_constructor_dtype_datetime64 #24868

TST: resolve issues with test_constructor_dtype_datetime64 #24868

simonjayhawkins commented Jan 21, 2019 •

edited

Loading

WillAyd Jan 21, 2019

simonjayhawkins Jan 21, 2019

simonjayhawkins Jan 22, 2019

jreback Jan 22, 2019

jreback Jan 22, 2019

simonjayhawkins Jan 24, 2019

jreback Jan 26, 2019 •

edited

Loading

simonjayhawkins Mar 12, 2019

simonjayhawkins Mar 13, 2019

codecov bot commented Jan 21, 2019

codecov bot commented Jan 21, 2019 •

edited

Loading

jreback Jan 22, 2019

WillAyd commented Feb 12, 2019

WillAyd commented Feb 19, 2019

simonjayhawkins commented Mar 12, 2019

WillAyd commented Mar 12, 2019

simonjayhawkins commented Mar 12, 2019

jreback commented Mar 13, 2019

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

	pytest.raises(TypeError,
	lambda x: Series(dates, dtype='datetime64'))

TST: resolve issues with test_constructor_dtype_datetime64 #24868

TST: resolve issues with test_constructor_dtype_datetime64 #24868

Conversation

simonjayhawkins commented Jan 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jreback Jan 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 21, 2019

Codecov Report

codecov bot commented Jan 21, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

WillAyd commented Feb 12, 2019

WillAyd commented Feb 19, 2019

simonjayhawkins commented Mar 12, 2019

WillAyd commented Mar 12, 2019

simonjayhawkins commented Mar 12, 2019

jreback commented Mar 13, 2019

simonjayhawkins commented Jan 21, 2019 •

edited

Loading

jreback Jan 26, 2019 •

edited

Loading

codecov bot commented Jan 21, 2019 •

edited

Loading