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

Begin pubsub regression suite #765

Merged
merged 2 commits into from
Mar 27, 2015
Merged

Begin pubsub regression suite #765

merged 2 commits into from
Mar 27, 2015

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Mar 27, 2015

Uses #764 as a base.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 27, 2015
@tseaver tseaver mentioned this pull request Mar 27, 2015
9 tasks
@tseaver tseaver added the api: pubsub Issues related to the Pub/Sub API. label Mar 27, 2015
@dhermes
Copy link
Contributor

dhermes commented Mar 27, 2015

Got a lot of unused variables (I assume build failed). Also the do nothing setup and teardownModule aren't needed.

Other questions

  • Why the URI changes?
  • Do you plan on implementing more in this PR?

(Sorry for hacky review. On phone.)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling b906f33 on tseaver:pubsub-begin_regression_suite into b45a909 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor Author

tseaver commented Mar 27, 2015

I rebased to remove #764 and adjust the tests to accommodate the changed URIs.

Per the API docs, the URIs are all based at https://pubsub.googleapis.com/v1beta2, rather than https://www.googleapis.com/pubsub/v1beta1. I know they are right because the regression tests were all 404ing with non-JSON errors before. :)

@tseaver
Copy link
Contributor Author

tseaver commented Mar 27, 2015

I plan to add more tests in later PRs (e.g., once we decide about returning Topic instances from pubsub.api.list_topics()).

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c1ebe23 on tseaver:pubsub-begin_regression_suite into b45a909 on GoogleCloudPlatform:master.

@dhermes
Copy link
Contributor

dhermes commented Mar 27, 2015

Still have unused vars HTTP and SHARED_BUCKETS and an unused import httplib2.

Just call me the human linter :)

Other than LGTM (if you don't don't can you squash these fixed into the original commit, 4 commits for tint change is sadness)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5981067 on tseaver:pubsub-begin_regression_suite into b45a909 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor Author

tseaver commented Mar 27, 2015

@dhermes I removed those variables and the import, and squashed it down to two commits.

tseaver added a commit that referenced this pull request Mar 27, 2015
@tseaver tseaver merged commit 6d08741 into googleapis:master Mar 27, 2015
@dhermes
Copy link
Contributor

dhermes commented Mar 27, 2015

Looks great thanks

@tseaver tseaver deleted the pubsub-begin_regression_suite branch March 27, 2015 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the Pub/Sub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants