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

Video Intelligence: Enrich VPCSC tests. #9193

Merged
merged 29 commits into from
Sep 23, 2019
Merged

Conversation

mkudejim
Copy link
Contributor

No description provided.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 10, 2019
@tseaver tseaver changed the title Enrich VPC SC tests for Video Intelligence API. Video Intelligence: Enrich VPCSC tests. Sep 10, 2019
@tseaver tseaver added testing kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Sep 10, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 10, 2019
@mkudejim
Copy link
Contributor Author

The test fails because the GOOGLE_APPLICATION_CREDENTIALS are set to a non VPC SC project. I have a dedicated service account key file that can be used to set GOOGLE_APPLICATION_CREDENTIALS, how can I add that to the Kokoro test runner?

@busunkim96
Copy link
Contributor

Hi @mkudejim - the approach we've been taking with the VPCSC tests is that a subset of them are run in the CI for this repo (the tests that don't require VPCSC) and that the full set are run elsewhere with VPCSC enabled.

We sorted this out for Monitoring a few weeks ago. You can reference their system tests and some additional setup in the noxfile.py

@mkudejim
Copy link
Contributor Author

Thank you for the info @busunkim96. I have made it so that VPCSC tests are only run when in vpcsc is set to true. PTAL.

@mkudejim mkudejim requested a review from busunkim96 September 13, 2019 01:25
@busunkim96 busunkim96 added the api: videointelligence Issues related to the Video Intelligence API API. label Sep 13, 2019
@tseaver tseaver added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 16, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 16, 2019
@busunkim96
Copy link
Contributor

@mkudejim The client libraries team and VPCSC team is going to have a meeting in about a week to clarify what we would like from these system tests. We'll hopefully have clearer guidance to provide at that point.

@busunkim96 busunkim96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 23, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 23, 2019
@busunkim96 busunkim96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 23, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 23, 2019
@busunkim96 busunkim96 merged commit 10a824b into googleapis:master Sep 23, 2019
@tseaver
Copy link
Contributor

tseaver commented Nov 6, 2019

@busunkim96, @mkudejim I would like for us to back out the changes in this PR, because:

  • It removes an actual system test for the client provided by the google-cloud-videouintelligence library.
  • The remaining tests for VPCSC functionality do not use the client at all, but make bare HTTP calls using the requests library.

I would also like to back out PR #8607, and look at how to build in real system tests which exercise the VPCSC constraints.

@busunkim96
Copy link
Contributor

@tseaver

I'd also prefer to go the route of real system tests that also exercise VPCSC constraints, but those take longer to write, and the product team folks tasked with writing these tests don't seem to have the bandwith for.

I did not notice that this PR was deleting a previously existing system test. That should definitely be re-added.

I'll send a PR to revert tomorrow morning.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: videointelligence Issues related to the Video Intelligence API API. cla: yes This human has signed the Contributor License Agreement. testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants