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

More gax paging fixes #1870

Merged
merged 3 commits into from
Jun 20, 2016
Merged

More gax paging fixes #1870

merged 3 commits into from
Jun 20, 2016

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Jun 20, 2016

Work toward fixing Pubsub system tests using GAX.

Correcting incomplete fixes in #1855.

Note that Topic.pull is still problematic (#1869).

@tseaver tseaver added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. api: pubsub Issues related to the Pub/Sub API. labels Jun 20, 2016
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 20, 2016
@daspecster daspecster mentioned this pull request Jun 20, 2016
Merged
7 tasks
daspecster
daspecster reviewed Jun 20, 2016
View reviewed changes
gcloud/pubsub/_gax.py
response = self._gax_api.publish(topic_path, message_pbs)
event = self._gax_api.publish(topic_path, message_pbs)
if not event.is_set():
import pdb; pdb.set_trace()

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tseaver
Copy link
Contributor Author

tseaver commented Jun 20, 2016

@daspecster I've fixed the lint issues with 2f347b8. The coverage gap was actually introduced in #1868: see #1871 for a fix.

@tseaver
Copy link
Contributor Author

tseaver commented Jun 20, 2016

@daspecster I don't know why coveralls shows 100%. When I run tox -e cover locally, it shows a missing branch from line 206 -> exit of gcloud/test__helpers.py: that is what led me to #1871.

/me looks: I don't think coveralls is doing branch coverage. :(

@daspecster
Copy link
Contributor

daspecster commented Jun 20, 2016 •
edited
Loading

Looks like it doesn't lemurheavy/coveralls-public#31

@tseaver
Copy link
Contributor Author

tseaver commented Jun 20, 2016

@daspecster I've rebased against master after merging #1871.

@daspecster
Copy link
Contributor

daspecster commented Jun 20, 2016 •
edited
Loading

Maybe this should be a separate issue, but perhaps we should have travis run the cover task separately with the coveralls task?

Made an issue for this: #1872

@daspecster
Copy link
Contributor

This LGTM @tseaver. Just to confirm, setting page_size = 0, the cloud side will just return whatever their max is for page_size? Or should it be None?

@tseaver
Copy link
Contributor Author

tseaver commented Jun 20, 2016

@daspecster

Just to confirm, setting page_size = 0, the cloud side will just return whatever their max is for page_size? Or should it be None?

GAX-generated wrappers use 0 as the default for page_size.

@daspecster
Copy link
Contributor

Ok, let's merge!

@tseaver
Copy link
Contributor Author

tseaver commented Jun 20, 2016

BTW, Travis used to run cover: we dropped it to allow the initial run to complete more quickly (a false economy, maybe).

@tseaver tseaver merged commit 8c609e9 into googleapis:master Jun 20, 2016
@tseaver tseaver deleted the 1855-moar_gax_paging_fixes branch June 20, 2016 18:56
This was referenced Jun 20, 2016
Closed
Closed
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: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tseaver @daspecster @googlebot