[#1675][FOLLOWUP] fix(test): Fix various flaky tests #1730
Conversation
| @@ -921,6 +921,7 @@ | |||
| <java.io.tmpdir>${project.build.directory}/tmp</java.io.tmpdir> | |||
| <project.version>${project.version}</project.version> | |||
| <jacoco-agent.destfile>target/jacoco.exec</jacoco-agent.destfile> | |||
| <test.mode>true</test.mode> | |||
There was a problem hiding this comment.
You can see the 2nd point listed above
There was a problem hiding this comment.
Since RssUtils#getHostIp is a method in a utility class and does not accept any input parameters, we can only use environment variables to inform the internal logic that the unit test is being executed. This approach requires the least amount of changes and is the most reasonable.
There was a problem hiding this comment.
Em... it seems not enough reasonable. If we should support the similar scenary, we should limit to use it in the test mode.
There was a problem hiding this comment.
When running unit tests, they are executed on a single machine and do not interact with other machines. Therefore, unit tests should not require a valid broadcast IP address. Currently, we do not support scenarios where the broadcast IP address of the host machine is 0.0.0.0. I think this is an unreasonable limitation.
There was a problem hiding this comment.
I got it. You should add the comment, too.
|
Maybe this should be committed to 0.9 as well. |
|
We can merge this. |
|
Merged to master & branch 0.9. |
### What changes were proposed in this pull request? Fix various flaky tests: 1. After using `SimpleClusterManager`, release resources at once, which might cause tests to fail sometimes. 2. The compiler machine may not necessarily have a valid broadcast address (maybe in a pod created by K8S, and the broadcast address could be 0.0.0.0). Unit tests should running successfully on such machines. We need to support this scenario. 3. Refactor the `URI.create` logic in `SimpleClusterManagerTest` to support cross-platform operation, such as running tests on Windows. 4. Use `IntegrationTestBase#shutdownServers` to reduce duplicated codes. ### Why are the changes needed? For #1675. ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Unnecessary.
What changes were proposed in this pull request?
Fix various flaky tests:
SimpleClusterManager, release resources at once, which might cause tests to fail sometimes.URI.createlogic inSimpleClusterManagerTestto support cross-platform operation, such as running tests on Windows.IntegrationTestBase#shutdownServersto reduce duplicated codes.Why are the changes needed?
For #1675.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Unnecessary.