-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix the test 'test_user_agent_passed' without skipping it #29422
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I have the context, but I think you could add a brief comment explaining why mocking each
Do you mean the comment of this commit or the comment in code? |
I meant in the code for future reference -- I should've highlighted that this is just a nitpick |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #29422 +/- ##
==========================================
- Coverage 38.34% 38.34% -0.01%
==========================================
Files 693 693
Lines 102199 102237 +38
==========================================
+ Hits 39185 39199 +14
- Misses 61422 61446 +24
Partials 1592 1592
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
* Internally, `bigquery_tools.BigQueryWrapper` has two BigQuery clients: `apache_beam.io.gcp.internal.clients.bigquery.bigquery_v2_client.BigqueryV2` (apitools) and `google.cloud.bigquery.Client`. * The function call of `wrapper.create_temporary_dataset` only uses the first client. To initialize this client, however, we need to skip the credential routine in apitools, or it makes the test crash in our internal test environment (ipv4 loopback enabled). * We also need to simply mock the second client, so it won't raise an exception (if google.cloud.bigquery is not available) during the creation of `BigQueryWrapper`. Notice that if an exception IS raised, the test would be skipped.
e573498
to
f10b925
Compare
Thanks! I put more explanation in the commit message. |
Assigning reviewers. If you would like to opt out of this review, comment R: @jrmccluskey for label python. Available commands:
The PR bot will only process comments in the main thread (not review comments). |
Nice, thanks. Noting for future changes that commit messages are not as easy to lookup as seeing a comment in the code. |
Understood. I will keep that in mind. Thanks for merging. |
Please add a meaningful description for your change here
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
addresses #123
), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, commentfixes #<ISSUE NUMBER>
instead.CHANGES.md
with noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI or the workflows README to see a list of phrases to trigger workflows.