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

chore: pass explicit credentials in all unit tests creating clients #369

Merged
merged 6 commits into from
Apr 6, 2021

Conversation

jimfulton
Copy link
Contributor

@jimfulton jimfulton commented Apr 6, 2021

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #366 🦕

To allow unit tests to run without connecting to the google APIs.
@product-auto-label product-auto-label bot added the api: pubsub Issues related to the googleapis/python-pubsub API. label Apr 6, 2021
@google-cla
Copy link

google-cla bot commented Apr 6, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Apr 6, 2021
@jimfulton
Copy link
Contributor Author

@googlebot I signed it!

@google-cla
Copy link

google-cla bot commented Apr 6, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

This looks good, the tests now pass even if the GOOGLE_APPLICATION_CREDENTIALS env variable is unset.

Also thanks for removing the duplicate tests, they make no sense now that Python 2 does not have to be supported anymore and string literals are former unicode instances.

@plamut
Copy link
Contributor

plamut commented Apr 6, 2021

The CLA still needs to be signed so that the bot gives the green light for that check.

@jimfulton
Copy link
Contributor Author

We have a bit of a DRY violation with creds = mock.Mock(spec=credentials.Credentials).

Any reason not to make creds a global?

@jimfulton
Copy link
Contributor Author

The CLA still needs to be signed so that the bot gives the green light for that check.

Yeah, I signed it. How long does it take to verify?

@plamut
Copy link
Contributor

plamut commented Apr 6, 2021

Playing a devil's advocate - the code might modify that mock which could affect other tests using the same mock. It's probably not applicable here, though. :)

Since pytest is used, we can instead create a fixture that creates a client with mock credentials, and inject the instance into tests. What do you think?

@jimfulton
Copy link
Contributor Author

Playing a devil's advocate - the code might modify that mock which could affect other tests using the same mock. It's probably not applicable here, though. :)

Since pytest is used, we can instead create a fixture that creates a client with mock credentials, and inject the instance into tests. What do you think?

+1

@plamut
Copy link
Contributor

plamut commented Apr 6, 2021

Yeah, I signed it. How long does it take to verify?

@jimfulton It's almost instant, but you need to ping the bot. ;)

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

Edit: Provided that the CLA signing is indeed complete, I honestly don't remember all the details (if any) anymore.

@googleapis googleapis deleted a comment from google-cla bot Apr 6, 2021
@googleapis googleapis deleted a comment from google-cla bot Apr 6, 2021
@googleapis googleapis deleted a comment from google-cla bot Apr 6, 2021
@googleapis googleapis deleted a comment from google-cla bot Apr 6, 2021
@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googleapis googleapis deleted a comment from google-cla bot Apr 6, 2021
@googleapis googleapis deleted a comment from google-cla bot Apr 6, 2021
@googleapis googleapis deleted a comment from google-cla bot Apr 6, 2021
@googleapis googleapis deleted a comment from google-cla bot Apr 6, 2021
@jimfulton
Copy link
Contributor Author

@googlebot I signed it!

@jimfulton
Copy link
Contributor Author

@googlebot I signed it!

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

memo Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers
* It's possible we don't have your GitHub username or you're using a different email address on your commit. Check [your existing CLA data](https://cla.developers.google.com/clas) and verify that your [email is set on your git commits](https://help.github.com/articles/setting-your-email-in-git/).
Corporate signers
* Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to [go/cla#troubleshoot](http://go/cla#troubleshoot) ([Public version](https://opensource.google/docs/cla/#troubleshoot)).

* The email used to register you as an authorized contributor must be the email used for the Git commit. Check [your existing CLA data](https://cla.developers.google.com/clas) and verify that your [email is set on your git commits](https://help.github.com/articles/setting-your-email-in-git/).

* The email used to register you as an authorized contributor must also be [attached to your GitHub account](https://github.com/settings/emails).

information_source Googlers: Go here for more info.

@googlebot I signed it!

@jimfulton
Copy link
Contributor Author

@googlebot I signed it!

@tseaver
Copy link
Contributor

tseaver commented Apr 6, 2021

@jimfulton I would guess that your CLA is associated with a different e-mail address than the one you used to sign the git commits.

@jimfulton
Copy link
Contributor Author

@kokoro: force-run

@tseaver
Copy link
Contributor

tseaver commented Apr 6, 2021

@jimfulton The failed build is due to a flaky systest (#352, which @plamut has a fix ready for in #361). We could go ahead and get that PR reviewed and merged and then merge here.

I you to decide to trigger another build here, add the kokoro: force-run label to the issue.

@tseaver tseaver changed the title Fix: Unit tests must not use/expect credentials from the environment #366 chore: pass explicit credentials in all unit tests creating clients Apr 6, 2021
Copy link
Contributor

@plamut plamut left a comment

Choose a reason for hiding this comment

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

The credentials fixture is nice, LGTM.

@jimfulton
Copy link
Contributor Author

@tseaver I hear I have to be in a special group for kokoro: force-run to work.

@tseaver
Copy link
Contributor

tseaver commented Apr 6, 2021

@jimfulton Looks like Kokoro did run at 17:02Z, well after you added the label at 16:21Z. I have no clue why the bot didn't remove the label, though.

@jimfulton
Copy link
Contributor Author

@tseaver , I also pushed a commit.

@jimfulton
Copy link
Contributor Author

@googlebot I signed it!

@google-cla
Copy link

google-cla bot commented Apr 6, 2021

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Apr 6, 2021
@jimfulton jimfulton marked this pull request as ready for review April 6, 2021 21:17
@jimfulton jimfulton requested a review from a team as a code owner April 6, 2021 21:17
@plamut
Copy link
Contributor

plamut commented Apr 6, 2021

@jimfulton Congratulations on your first accepted PR! 🎉

@plamut plamut merged commit c707f81 into master Apr 6, 2021
@plamut plamut deleted the riversnake-366 branch April 6, 2021 21:35
Copy link
Contributor

@pradn pradn left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for your contribution!

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 googleapis/python-pubsub API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unit tests must not use/expect credentials from the environment
5 participants