Skip to content

Add PubSub system test for fix in #4174 #4190

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
Merged
tseaver merged 9 commits into from
Oct 17, 2017
Merged

Add PubSub system test for fix in #4174 #4190

tseaver merged 9 commits into from
Oct 17, 2017

Conversation

@jeffkpayne
Copy link
Contributor

@jeffkpayne jeffkpayne commented Oct 14, 2017

@lukesneeringer Here's my attempt at a system test for the fix in #4174

Let me know how I can improve it, etc. Thanks.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 14, 2017
@jeffkpayne
Copy link
Contributor Author

jeffkpayne commented Oct 14, 2017

Not sure why, but this is showing the commit from my previous PR. I fetched and merged upstream/master, but maybe I should have rebased on master? Sorry for any inconvenience!

@jeffkpayne
Copy link
Contributor Author

jeffkpayne commented Oct 14, 2017

@dhermes This includes a change to the unit test that broke on #4174 in the Python 2.7 environment. Please note that the test in question passed on my machine before this change. Maybe due to the difference of environments between my Mac and CircleCI's machines?

dhermes
dhermes reviewed Oct 14, 2017
View reviewed changes
pubsub/tests/system.py Outdated

from __future__ import absolute_import

from datetime import datetime

This comment was marked as spam.

Sign in to view

tseaver
tseaver suggested changes Oct 16, 2017
View reviewed changes
pubsub/tests/unit/pubsub_v1/subscriber/test_policy_thread.py Outdated
# Actually run the method and prove that the callback was
# called in the expected way.
policy.on_response(response)
time.sleep(1)

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@jeffkpayne
Replace potentially flaky time.sleep with proper use of mock-based as…
97afe3e 
…sertions in test_on_response, per code review
@jeffkpayne
Copy link
Contributor Author

jeffkpayne commented Oct 17, 2017

Thought about factoring out:

    submit_calls = [m for m in executor.method_calls if m[0] == 'submit']

and

    add_done_callback_calls = [
        m for m in future.method_calls if m[0] == 'add_done_callback']

into a function, but wasn't sure if that was overkill or where to put it...

@jeffkpayne
Copy link
Contributor Author

jeffkpayne commented Oct 17, 2017

Gah, coverage. Fixing now...

@jeffkpayne
Copy link
Contributor Author

jeffkpayne commented Oct 17, 2017

OK, so nox -s cover locally from the pubsub directory gives me:

(nox-python-3.6)
nox > Running session cover
nox > /Users/jpayne/miniconda3/envs/nox-python-3.6/bin/python -m virtualenv /Users/jpayne/repos/bombora-google-cloud-python/pubsub/.nox/cover -p python3.6
nox > chdir /Users/jpayne/repos/bombora-google-cloud-python/pubsub
nox > pip install --upgrade coverage pytest-cov
nox > coverage report --show-missing --fail-under=100
Name                                                   Stmts   Miss Branch BrPart  Cover   Missing
--------------------------------------------------------------------------------------------------
google/cloud/pubsub.py                                     5      0      0      0   100%
google/cloud/pubsub_v1/__init__.py                         5      0      0      0   100%
google/cloud/pubsub_v1/_gapic.py                          26      0     16      0   100%
google/cloud/pubsub_v1/publisher/__init__.py               3      0      0      0   100%
google/cloud/pubsub_v1/publisher/batch/__init__.py         0      0      0      0   100%
google/cloud/pubsub_v1/publisher/batch/base.py            27      0      6      0   100%
google/cloud/pubsub_v1/publisher/batch/thread.py          66      0     14      0   100%
google/cloud/pubsub_v1/publisher/client.py                41      0     14      0   100%
google/cloud/pubsub_v1/publisher/exceptions.py             6      0      0      0   100%
google/cloud/pubsub_v1/publisher/futures.py               47      0     14      0   100%
google/cloud/pubsub_v1/subscriber/__init__.py              3      0      0      0   100%
google/cloud/pubsub_v1/subscriber/_consumer.py            56      0      8      0   100%
google/cloud/pubsub_v1/subscriber/_helper_threads.py      46      0      8      0   100%
google/cloud/pubsub_v1/subscriber/_histogram.py           37      0     16      0   100%
google/cloud/pubsub_v1/subscriber/client.py               20      0      4      0   100%
google/cloud/pubsub_v1/subscriber/message.py              30      0      0      0   100%
google/cloud/pubsub_v1/subscriber/policy/__init__.py       0      0      0      0   100%
google/cloud/pubsub_v1/subscriber/policy/base.py          94      0     24      0   100%
google/cloud/pubsub_v1/subscriber/policy/thread.py        52      0     11      0   100%
google/cloud/pubsub_v1/types.py                           18      0      2      0   100%
--------------------------------------------------------------------------------------------------
TOTAL                                                    582      0    137      0   100%
nox > coverage erase
nox > Session cover was successful.

showing 100% for everything, but CircleCI shows 98% for google/cloud/pubsub_v1/subscriber/policy/thread.py. Am I missing something?

@jeffkpayne
Copy link
Contributor Author

jeffkpayne commented Oct 17, 2017

Gah, now I see it locally. Adding a test for thread._callback_completed()...

@tseaver tseaver merged commit 1b7c6bd into googleapis:master Oct 17, 2017
@tseaver
Copy link
Contributor

tseaver commented Oct 17, 2017

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lukesneeringer lukesneeringer Awaiting requested review from lukesneeringer

2 more reviewers

@dhermes dhermes dhermes left review comments

@tseaver tseaver tseaver approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

4 participants

@jeffkpayne @tseaver @dhermes @googlebot