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- Fixing issue with test_parquet test unexpectedly passing #24480

Merged
merged 7 commits into from
Dec 30, 2018

Conversation

RjLi13
Copy link
Contributor

@RjLi13 RjLi13 commented Dec 29, 2018

This pr aims to make the failing test_parquet not strict since it now passes at least on my machine of Mac 10.14 running the latest python 3.7 and conda.

@codecov
Copy link

codecov bot commented Dec 29, 2018

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #24480   +/-   ##
=======================================
  Coverage   92.32%   92.32%           
=======================================
  Files         166      166           
  Lines       52298    52298           
=======================================
  Hits        48285    48285           
  Misses       4013     4013
Flag Coverage Δ
#multiple 90.74% <ø> (ø) ⬆️
#single 43.06% <ø> (ø) ⬆️

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 80295f9...d9bc62d. Read the comment docs.

@codecov
Copy link

codecov bot commented Dec 29, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24480      +/-   ##
==========================================
- Coverage   92.32%   92.31%   -0.01%     
==========================================
  Files         166      166              
  Lines       52328    52335       +7     
==========================================
+ Hits        48310    48312       +2     
- Misses       4018     4023       +5
Flag Coverage Δ
#multiple 90.73% <ø> (-0.01%) ⬇️
#single 43.04% <ø> (ø) ⬆️
Impacted Files Coverage Δ
pandas/core/arrays/datetimes.py 97.77% <0%> (-0.67%) ⬇️
pandas/compat/__init__.py 57.91% <0%> (-0.39%) ⬇️
pandas/core/series.py 93.73% <0%> (ø) ⬆️

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 8dd9cdd...41427a3. Read the comment docs.

@@ -201,7 +201,8 @@ def test_options_get_engine(fp, pa):


@pytest.mark.xfail(is_platform_windows() or is_platform_mac(),
Copy link
Contributor

@TomAugspurger TomAugspurger Dec 29, 2018

Choose a reason for hiding this comment

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

Can you try removing the xfail entirely? It looks like it may be unnecessary now.

Copy link
Contributor Author

@RjLi13 RjLi13 Dec 29, 2018

Choose a reason for hiding this comment

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

I did do that in another branch and it passed the CI tests of travis, but it seems there's discussion of pyarrow and fastparquet as well as windows causing problems? Not completely sure.

Edit: Also passed CI tests in azure

@jreback jreback added Testing pandas testing functions or related to the test suite IO Parquet parquet, feather labels Dec 29, 2018
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

add pyarrow and fastparquet to this build: https://github.com/pandas-dev/pandas/blob/master/ci/deps/azure-macos-35.yaml as these are not being tested. I suspect this will actually still fail. I think then this test needs to be conditionally skipped on older pyarrow / fastparquet versions. We could add another macosx build that is a bit more modern, or just relax the numpy on this build to have it install a more modern pyarrow.

cc @datapythonista

@jreback
Copy link
Contributor

jreback commented Dec 29, 2018

@datapythonista do we have the xfails shows somewhere? e.g. https://dev.azure.com/pandas-dev/pandas/_build/results?buildId=6035

@datapythonista
Copy link
Member

I don't exactly understand the rationale behind the builds, but a modern Mac build sounds reasonable.

Without know much about the builds we have, what would make sense to me is to have a "base" dependencies file, and have a build with the same conda deps in Linux, Windows and Mac. Happy to work on this if it makes sense.

Regarding xfails, I wanted to standardize the output of all the builds in #23115 (which still needs a bit of work). Not sure what is the preferred output, I think we have -r sxX in some places (like the windows builds).

@jreback jreback added this to the 0.24.0 milestone Dec 29, 2018
@jreback
Copy link
Contributor

jreback commented Dec 29, 2018

Seems to be passing now.

@RjLi13
Copy link
Contributor Author

RjLi13 commented Dec 29, 2018

So to catch up a bit, we still need the xfail because of pyarrow and fastparquet having differing versions on windows? Or is it Mac now supports cross-compat with differing reading/writing engines, but Windows doesn't?

@jreback
Copy link
Contributor

jreback commented Dec 29, 2018

no i think this is ok now

@RjLi13
Copy link
Contributor Author

RjLi13 commented Dec 29, 2018

@jreback I have an unpublished branch that removes the xfail completely from both windows and mac. It seems to be passing the CI tests. Can you advise if we should take out the xfail completely?

@jreback
Copy link
Contributor

jreback commented Dec 29, 2018

yeah might be good to try that

@RjLi13
Copy link
Contributor Author

RjLi13 commented Dec 30, 2018

Looks like removing the xfail works, seems whatever issue arose from running this test (old dependency maybe?) is gone now.

@TomAugspurger TomAugspurger merged commit 681e75c into pandas-dev:master Dec 30, 2018
@TomAugspurger
Copy link
Contributor

Thanks @RjLi13!

thoo added a commit to thoo/pandas that referenced this pull request Dec 30, 2018
* upstream/master:
  DOC: Fixing broken references in the docs (pandas-dev#24497)
  DOC: Splitting api.rst in several files (pandas-dev#24462)
  Fix misdescription in escapechar (pandas-dev#24490)
  Floor and ceil methods during pandas.eval which are provided by numexpr (pandas-dev#24355)
  BUG: Pandas any() returning false with true values present (GH pandas-dev#23070) (pandas-dev#24434)
  Misc separable pieces of pandas-dev#24024 (pandas-dev#24488)
  use capsys.readouterr() as named tuple (pandas-dev#24489)
  REF/TST: replace capture_stderr with pytest capsys fixture (pandas-dev#24496)
  TST- Fixing issue with test_parquet test unexpectedly passing (pandas-dev#24480)
  DOC: Doc build for a single doc made much faster, and clean up (pandas-dev#24428)
  BUG: Fix+test timezone-preservation in DTA.repeat (pandas-dev#24483)
  Implement reductions from pandas-dev#24024 (pandas-dev#24484)
thoo added a commit to thoo/pandas that referenced this pull request Dec 30, 2018
…strings

* upstream/master:
  TST: Skip db tests unless explicitly specified in -m pattern (pandas-dev#24492)
  Mix EA into DTA/TDA; part of 24024 (pandas-dev#24502)
  DOC: Fix building of a single API document (pandas-dev#24506)
  DOC: Fixing broken references in the docs (pandas-dev#24497)
  DOC: Splitting api.rst in several files (pandas-dev#24462)
  Fix misdescription in escapechar (pandas-dev#24490)
  Floor and ceil methods during pandas.eval which are provided by numexpr (pandas-dev#24355)
  BUG: Pandas any() returning false with true values present (GH pandas-dev#23070) (pandas-dev#24434)
  Misc separable pieces of pandas-dev#24024 (pandas-dev#24488)
  use capsys.readouterr() as named tuple (pandas-dev#24489)
  REF/TST: replace capture_stderr with pytest capsys fixture (pandas-dev#24496)
  TST- Fixing issue with test_parquet test unexpectedly passing (pandas-dev#24480)
  DOC: Doc build for a single doc made much faster, and clean up (pandas-dev#24428)
  BUG: Fix+test timezone-preservation in DTA.repeat (pandas-dev#24483)
  Implement reductions from pandas-dev#24024 (pandas-dev#24484)
@RjLi13 RjLi13 deleted the fix_parquet_test branch December 31, 2018 00:16
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO Parquet parquet, feather Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TST: Test_parquet failing
4 participants