Skip to content

Conversation

@lqiu96
Copy link
Member

@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 requested review from blakeli0 and burkedavison June 6, 2023 19:48
@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
@sonarqubecloud
Copy link

sonarqubecloud 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

// 1 more call during channel refresh
Mockito.verify(mockChannelPrimer, Mockito.times(3))
.primeChannel(Mockito.any(ManagedChannel.class));
pool.shutdown();
Copy link
Contributor

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
Member 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.

@sonarqubecloud
Copy link

sonarqubecloud 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

@sonarqubecloud
Copy link

sonarqubecloud 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
Contributor

@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
Member 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
Member 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