test: fix flakes in HttpJsonDirectServerStreamingCallableTest #2432
Conversation
The flakes seem to stem from parallel execution and the resulting race conditions around static member variables, particularly the `mockService`. Attempting to fix this by using a separate `mockService` for each test. Fixes: #1905. Fixes: #2107. Fixes: #1876. Fixes: #2083. Fixes: #1842. Fixes: #1587. Fixes: #1684.
|
Picking the 1st issue #1905, would you describe (in PR description) why the assertion (the return value of As per its Javadoc (https://github.com/googleapis/sdk-platform-java/blob/d2fe5203c4db1f8ddfd5a9566e6259b53441348a/gax-java/gax-httpjson/src/test/java/com/google/api/gax/httpjson/testing/MockHttpService.java), MockHttpTransport is thread-safe. |
|
@suztomo I don't believe that the thread safety of the The main claim I'm making is that parallel execution of tests with a shared |
@meltsufin If I remember correctly the test methods in a JUnit test class run sequentially, unless pom.xml specifies the parallel property. We can confirm this by inserting the timestamp at the beginning and the end of each test method. I believe the (private) MOCK_SERVICE variable is not accessed by other classes the HttpJsonDirectServerStreamingCallableTest. Based on these, I think the current usage of My memo: testServerStreamingStart (from the main branch) MockHttpService's javadoc describes it's a queue (FIFO = first-in, first-out):
Why would |
|
@suztomo The tests should be written in a way as to support parallel execution, since it's controlled externally from the test itself. I've tried explicitly enabling parallel execution and running the test in gax-java/gax-httpjson/pom.xml: <plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>3.2.5</version>
<configuration>
<parallel>all</parallel>
<useUnlimitedThreads>true</useUnlimitedThreads>
</configuration>
</plugin>Results: This does not happen when run from the |
|
@meltsufin The tests with MockHttpService were written without parallel execution in mind (So it's natural that they fail in parallel execution). I don't think it needs parallel execution.
Would you elaborate this more? I see HttpJsonDirectServerStreamingCallableTest is just a normal JUnit test, something special about this class? |
There is nothing special about the test other than that it doesn't support parallel execution. What I'm saying is that the test class itself cannot tell the plugin not to run in parallel. I'm not sure where parallel execution is enabled, but it seems like it is. It can be anywhere from the CI run command to the plugin configuration. My point is that it's wrong to make the assumption that the test will never be executed in a parallel manner. The symptoms of the flakes and the repo I mentioned point to the notion that it's the problem. |
|
Parallel execution, if configured, fails the build but build failure does not mean parallel execution is configured. (
My hypothesis is |
|
@suztomo As you know, |
|
On my Mac, I checked out the 
Given this observation, I don't think the current flaky-HttpJsonDirectServerStreamingCallableTest branch (342467d) fixes the flakiness problem. |
|
@suztomo Maybe this particular one can also benefit from an increased timeout for scenarios like this. Have you tried increasing? |
|
@suztomo Can you run your test again? I've removed that line because it's no longer necessary as the |
|
|
|
Thank you. My local test passes for testServerStreamingStart and the logic (the mock object is no longer shared and thus no need to wait) makes sense. The testServerStreamingStart fix is for #1905. What about other 6 issues this pull request claims to fix? I checked out the latest commit (1d4508d) and ran 
This matches the observation of #1842. |
|
@meltsufin Shall we merge this pull request with "Fixes #1905" only, rather than trying to fix other 6 issues? In general, it's a good practice to have fine-grained pull requests. |
Which of the other issues it doesn't fix? It's doing one main thing which is making the mockService not shared. |
|
I want PR authors to provide reasoning that the changes indeed fix the issue. Your "I've removed that line because it's no longer necessary as the mockService is no longer shared." was a great example of that (Thank you). I want similar reasoning for other issues as well, if this pull request claims the issues are fixed by this change.
At least #1842 is not addressed as below. I checked out the latest commit (1d4508d) and ran
This matches the observation of #1842. |
#1684 also recurred 
|
The flakes seem to stem from parallel execution and the resulting race conditions around static member variables, particularly the `mockService`. Attempting to fix this by using a separate `mockService` for each test. Fixes: #1905. Fixes: #2107. Fixes: #1876. Fixes: #2083. Fixes: #1587. Fixes: #1684.
The flakes seem to stem from parallel execution and the resulting race conditions around static member variables, particularly the
mockService. Attempting to fix this by using a separatemockServicefor each test.Fixes: #1905.
Fixes: #2107.
Fixes: #1876.
Fixes: #2083.
Related: #1842.
Fixes: #1587.
Fixes: #1684.