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

tests(monitoring): normalize VPCSC configuration in systests #9615

Merged
merged 5 commits into from
Nov 12, 2019
Merged

tests(monitoring): normalize VPCSC configuration in systests #9615

merged 5 commits into from
Nov 12, 2019

Conversation

tseaver
Copy link
Contributor

@tseaver tseaver commented Nov 6, 2019

Toward #9580.

@tseaver
tests(monitoring): do not set VPCSC env vars via nox.
02cc72e
@tseaver
tests(monitoring): normalize VPCSC configuration in systests
9a4ce29 
- Use 'vpcsc_config' for VPCSC environment, skipping.
- Skip VPCSC-only tests at class scope.
- Split '_is_rejected' to avoid 'list()' call for APIs not returning iterators.
- Remove never-reached branch in '_do_test'.
- Avoid swallowing unexpected exceptions in '_is_rejected'.
@tseaver tseaver added testing api: monitoring Issues related to the Cloud Monitoring API. labels Nov 6, 2019
@tseaver tseaver requested a review from busunkim96 as a code owner November 6, 2019 17:16
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Nov 6, 2019
@tseaver tseaver mentioned this pull request Nov 6, 2019
Closed
10 tasks
@tseaver tseaver added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 6, 2019
@tseaver
Copy link
Contributor Author

tseaver commented Nov 6, 2019

I tagged with "do not merge" because I believe we should refactor all these un-idiomatic tests along the lines of the changes in #9618.

@tseaver tseaver removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 12, 2019
@tseaver
Copy link
Contributor Author

tseaver commented Nov 12, 2019

@busunkim96 I removed the "do not merge" label after updating the tests to follow the pattern in #9618.

busunkim96
busunkim96 approved these changes Nov 12, 2019
View reviewed changes
Copy link
Contributor

@busunkim96 busunkim96 left a comment

Choose a reason for hiding this comment

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

Thank you for fixing all the VPCSC tests!

@busunkim96
Copy link
Contributor

Looks like one of the test files needs to be blackened:

black --check google tests docs
would reformat /tmpfs/src/github/google-cloud-python/monitoring/tests/system/test_vpcsc_v3.py
Oh no! 💥 💔 💥
1 file would be reformatted, 64 files would be left unchanged.
Command black --check google tests docs failed with exit code 1
Session lint failed.

@tseaver tseaver merged commit 505d553 into googleapis:master Nov 12, 2019
@steinwaywhw
Copy link
Contributor

Hi @tseaver shall we have a GVC? These tests are auto generated, not authored by human. The next test refresh will override any manual refactoring you've done.

@busunkim96
Copy link
Contributor

Hi @steinwaywhw, I believe your team maintains a script to make these tests? It'd be best for your generator to produce tests that look close to the present format (looking for the actual error that will be raised). The other VPCSC tests have also been changed to use this format.

Feel free to put something on my calendar if you would like to chat further.

@yoshi-automation yoshi-automation mentioned this pull request Jan 29, 2020
Merged
parthea pushed a commit that referenced this pull request Oct 21, 2023
@tseaver
tests(monitoring): normalize VPCSC configuration in systests (#9615)
c684392 
Toward #9580.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@busunkim96 busunkim96 busunkim96 approved these changes

Assignees
No one assigned
Labels
api: monitoring Issues related to the Cloud Monitoring API. cla: yes This human has signed the Contributor License Agreement. testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tseaver @busunkim96 @steinwaywhw @googlebot