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.

Sign up for GitHub

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

Jump to bottom

Using generated apitools client for storage uploads #850

Closed
wants to merge 2 commits into from
Closed

Using generated apitools client for storage uploads #850

wants to merge 2 commits into from

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Apr 29, 2015

See #848

@craigcitro As we discussed this is the "testing sucks" "progress" I've made so far.

Positive outlook: The regression test passes

dhermes added 2 commits April 29, 2015 11:12
@dhermes
Adding generated apitools client for storage API.
e4bdce7 
Also adding
- script to generate files
- sed line to fix up imports
- pep8 and pylint settings to ignore gen'd files
@dhermes
Mostly making Upload work with the generated client.
2224f48
@dhermes dhermes added the api: storage Issues related to the Cloud Storage API. label Apr 29, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 29, 2015
craigcitro
craigcitro reviewed Apr 30, 2015
View reviewed changes
gcloud/storage/blob.py
upload = transfer.Upload.FromStream(file_obj, content_type,
total_bytes,
chunksize=self.chunk_size)
# @craigcitro, can I put `callback` and `finish_callback` here?

This comment was marked as spam.

Sign in to view

@craigcitro
Copy link
Contributor

which tests were failing (other than the linter)?

@dhermes
Copy link
Contributor Author

dhermes commented Apr 30, 2015

I don't think we should worry about which tests are failing. We just need to write new tests with your replay/record framework.

The issue being that we'll want to have the same types of things covered, e.g. multipart and resumable and media and similar.

@dhermes
Copy link
Contributor Author

dhermes commented May 4, 2015

@craigcitro What say you about the replay/record testing features?

@craigcitro
Copy link
Contributor

just imported them into apitools last night; was doing some related cleanup (eg that PR you just LGTMd), and i'm going to cut a release.

planning on sending you a few follow-up commits on top of these.

@dhermes
Copy link
Contributor Author

dhermes commented May 4, 2015

That's awesome! Can't wait.

@dhermes
Copy link
Contributor Author

dhermes commented May 6, 2015

@craigcitro Updates here?

@craigcitro
Copy link
Contributor

apitools release is cut, but then this has been back-burnered. hoping to get to it today/tomorrow while some longer jobs are running.

@dhermes
Copy link
Contributor Author

dhermes commented May 6, 2015

Cool. Someday maybe apitools could have some docs and then Craig can be free 👯

@craigcitro
Copy link
Contributor

actually, it turned out i didn't really need any of the mocking code i added: the existing mocks already had sufficient coverage, i just needed to do a little cleanup.

i've pushed an extra commit to a branch in my fork; i'm happy to do whatever's most convenient for getting you unblocked.

two thoughts:

  1. i wouldn't put much faith in any mocking-based tests for media transfers. our experience shows there are too many corners, and subtle changes on the server side, to really trust that mocks remain valid. in particular, the tests here seem to mostly test the positive cases ("does the upload/download work"), whereas most of the interesting bugs we've run into are around the failure modes (of which there are far too many).
  2. i'd like to come back through and do a little cleanup around the use of apitools (i.e. i think some of the code can now be simplified), but i'm obviously not sitting on a lot of spare time right now. 😏 i'm still optimistic that i'll get to it, though.

@dhermes
Copy link
Contributor Author

dhermes commented May 7, 2015

@craigcitro The values you swapped in aren't the values we expected based on the inputs. This is why I wanted to ditch our existing tests and use something that you have used to test apitools (i.e. your record/replay framework).

@craigcitro
Copy link
Contributor

as mentioned on the commit comments, no one's threading the desired api endpoint into apitools, so there's no way it could know what address the tests were expecting. i guess i don't see the point of the current tests -- is there a use case where you'd point gcloud-python at something other than a google API?

you're welcome to rewrite the tests, but i don't have the time to do that, i'm afraid.

the apitools mocking framework doesn't support much for media testing, unfortunately. it's designed to allow record/replay of API calls, but only has some as-yet-unused media hooks.

@dhermes
Copy link
Contributor Author

dhermes commented May 13, 2015

Can you point me towards the record/replay?

@craigcitro
Copy link
Contributor

https://github.com/google/apitools/blob/master/apitools/base/py/testing/mock.py

@dhermes
Copy link
Contributor Author

dhermes commented Jun 24, 2015

This is very much dead in light of #935

@dhermes dhermes closed this Jun 24, 2015
@dhermes dhermes deleted the use-generated-apitools-client branch August 27, 2015 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dhermes @craigcitro @googlebot