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: Clean up resources in gRPCStreamDirectController and ChannelPool tests #1691

Merged
merged 11 commits into from
Jun 12, 2023

Conversation

lqiu96
Copy link
Contributor

@lqiu96 lqiu96 commented May 22, 2023

Clean up the resources that are created in the the GrpcDirectStreamControllerTest and ChannelPool tests.

Fixes: #1704

@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label May 22, 2023
@lqiu96 lqiu96 changed the title chore: Fix gRPCStreamDirectController flaky test chore: Clean up resources in gRPCStreamDirectController and ChannelPool tests May 22, 2023
@lqiu96 lqiu96 marked this pull request as ready for review June 6, 2023 19:48
@lqiu96 lqiu96 requested a review from a team as a code owner June 6, 2023 19:48
@lqiu96 lqiu96 added the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 6, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Jun 6, 2023
@sonarcloud
Copy link

sonarcloud bot commented Jun 6, 2023

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@@ -241,12 +253,14 @@ public void channelPrimerIsCalledPeriodically() throws IOException {
// 1 more call during channel refresh
Mockito.verify(mockChannelPrimer, Mockito.times(3))
.primeChannel(Mockito.any(ManagedChannel.class));
pool.shutdown();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this test is a unit test for ChannelPool, I would assume every test is creating its own channel pool, so it's worth it to extract pool to a field, the ndo shutdown and awaitTermination in a clean up method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah! Good point. Updating.

@sonarcloud
Copy link

sonarcloud bot commented Jun 6, 2023

[java_showcase_integration_tests] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@sonarcloud
Copy link

sonarcloud bot commented Jun 6, 2023

[java_showcase_unit_tests] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

lqiu96 pushed a commit that referenced this pull request Jun 8, 2023
…essor job (#1691) (#1007)

* chore: install dependencies through requirements file
Source-Link: https://togithub.com/googleapis/synthtool/commit/35f4cbaf1295a726cb43fd4471129ec74b48e04e
Post-Processor: gcr.io/cloud-devrel-public-resources/owlbot-java:latest@sha256:821ab7aba89af2c7907e29297bba024d4cd5366d0684e5eb463391cdf4edc9ee
Copy link
Collaborator

@blakeli0 blakeli0 left a comment

Choose a reason for hiding this comment

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

LGTM. Separately, I would like to understand what exactly this test is testing and maybe we can remove some of the test cases. e.g. Looks like we are using a fake server in unit test for testing RetrySettings, which is essentially a local server integration tests that is already covered by showcase tests.

@lqiu96
Copy link
Contributor Author

lqiu96 commented Jun 12, 2023

LGTM. Separately, I would like to understand what exactly this test is testing and maybe we can remove some of the test cases. e.g. Looks like we are using a fake server in unit test for testing RetrySettings, which is essentially a local server integration tests that is already covered by showcase tests.

Good point. I think we are due for an audit for a gax's unit tests. This test pre-dates the showcase test suite so the mock server may not be needed anymore. I'll create a generic issue for auditing and cleaning up the unit tests.

This PR is intended only to hopefully mitigate this flaky test. In the future, we may determine that this unit test is unnecessary.

@lqiu96
Copy link
Contributor Author

lqiu96 commented Jun 12, 2023

Created an issue at #1771

@lqiu96 lqiu96 merged commit 8cbea70 into main Jun 12, 2023
@lqiu96 lqiu96 deleted the main-fix-grpcstreamdirectcontroller_flaky_test branch June 12, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: m Pull request size is medium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flaky GrpcDirectStreamControllerTest.testRetryNoRaceCondition: test timed out
2 participants