Skip to content
This repository was archived by the owner on Mar 9, 2026. It is now read-only.

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

Merged
plamut merged 6 commits into
masterfrom
riversnake-366
Apr 6, 2021
Merged

chore: pass explicit credentials in all unit tests creating clients#369
plamut merged 6 commits into
masterfrom
riversnake-366

Conversation

@jimfulton

@jimfulton jimfulton commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

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 🦕

Jim Fulton added 2 commits April 5, 2021 17:09
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

google-cla Bot commented Apr 6, 2021

Copy link
Copy Markdown

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
Copy Markdown
Contributor Author

@googlebot I signed it!

@google-cla

google-cla Bot commented Apr 6, 2021

Copy link
Copy Markdown

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.

@plamut plamut left a comment

Copy link
Copy Markdown
Contributor

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

plamut commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

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

@jimfulton

Copy link
Copy Markdown
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
Copy Markdown
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

plamut commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

plamut commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

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
Copy Markdown

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
Copy Markdown
Contributor Author

@googlebot I signed it!

@jimfulton

Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

@googlebot I signed it!

@tseaver

tseaver commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor Author

@kokoro: force-run

@tseaver

tseaver commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

@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

@plamut plamut left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

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

@tseaver

tseaver commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor Author

@tseaver , I also pushed a commit.

@jimfulton

Copy link
Copy Markdown
Contributor Author

@googlebot I signed it!

@google-cla

google-cla Bot commented Apr 6, 2021

Copy link
Copy Markdown

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 April 6, 2021 21:17
@plamut

plamut commented Apr 6, 2021

Copy link
Copy Markdown
Contributor

@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

@pradn pradn left a comment

Copy link
Copy Markdown
Contributor

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 subscribe to this conversation on GitHub. Already have an account? Sign in.

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