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: Streaming of S3 files #22520

Merged
merged 5 commits into from
Aug 29, 2018
Merged

TST: Streaming of S3 files #22520

merged 5 commits into from
Aug 29, 2018

Conversation

kokes
Copy link
Contributor

@kokes kokes commented Aug 27, 2018

Issue #17135 was resolved by an upstream change, this is just a unit test to avoid regressions.

@kokes
Copy link
Contributor Author

kokes commented Aug 27, 2018

CI will crash and burn as I didn't add botocore as a dependency properly. Any help on this appreciated.

@TomAugspurger
Copy link
Contributor

I think the easiest thing to do is to ensure the test is skipped without botocore, and rely on s3fs pulling it in as a dependency.

@@ -9,6 +9,7 @@ dependencies:
- flake8-comprehensions
- hypothesis>=3.58.0
- moto
- botocore>=1.10.47
Copy link
Contributor

Choose a reason for hiding this comment

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

So this can be removed.

@@ -1,3 +1,7 @@
from six import BytesIO

from botocore.response import StreamingBody
Copy link
Contributor

Choose a reason for hiding this comment

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

And remove this.



def test_streaming_s3_objects():
data = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Then here you can do botocore_response = pytest.importorskip('botocore.response', minversion='1.10.47')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we do this when the version pertains to the base module, not the imported class? I just tried it in a dummy file and all I got was Skipped: module 'botocore.response' has __version__ None, required is: '1.10.47'. So should I do something like this?

import pytest

def my_test_fn():
    pytest.importorskip('botocore', minversion='1.10.47')
    from botocore.response import StreamingBody
    # test

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, your suggestion should work.

@@ -1,3 +1,7 @@
from six import BytesIO
Copy link
Contributor

Choose a reason for hiding this comment

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

from pandas.compat import BytesIO

@codecov
Copy link

codecov bot commented Aug 27, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@fa47b8d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #22520   +/-   ##
=========================================
  Coverage          ?   92.03%           
=========================================
  Files             ?      169           
  Lines             ?    50780           
  Branches          ?        0           
=========================================
  Hits              ?    46737           
  Misses            ?     4043           
  Partials          ?        0
Flag Coverage Δ
#multiple 90.44% <ø> (?)
#single 42.22% <ø> (?)

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 fa47b8d...14634b1. Read the comment docs.

@gfyoung gfyoung added Testing pandas testing functions or related to the test suite IO Data IO issues that don't fit into a more specific label labels Aug 27, 2018
@jreback jreback added this to the 0.24.0 milestone Aug 28, 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.

doc comment ping on green.



def test_streaming_s3_objects():
pytest.importorskip('botocore', minversion='1.10.47')
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add the issue number here as a comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, done

@codecov
Copy link

codecov bot commented Aug 28, 2018

Codecov Report

❗ No coverage uploaded for pull request base (master@fa47b8d). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #22520   +/-   ##
=========================================
  Coverage          ?   92.03%           
=========================================
  Files             ?      169           
  Lines             ?    50780           
  Branches          ?        0           
=========================================
  Hits              ?    46737           
  Misses            ?     4043           
  Partials          ?        0
Flag Coverage Δ
#multiple 90.44% <ø> (?)
#single 42.22% <ø> (?)

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 fa47b8d...14634b1. Read the comment docs.

@jreback jreback merged commit ae567cd into pandas-dev:master Aug 29, 2018
@jreback
Copy link
Contributor

jreback commented Aug 29, 2018

thanks!

@kokes kokes deleted the bug17135 branch August 29, 2018 12:39
Sup3rGeo pushed a commit to Sup3rGeo/pandas that referenced this pull request Oct 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
IO Data IO issues that don't fit into a more specific label Testing pandas testing functions or related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants