Repository navigation
test(spanner): unflake closeWhileStreamIsStartingInvokesOnDoneSynchronously - #14920
sakthivelmanii wants to merge 1 commit into
Conversation
…nously ## Problem - The test `closeWhileStreamIsStartingInvokesOnDoneSynchronously` in `GrpcResultSetTest` throws a `ClassCastException`: `com.google.cloud.spanner.OpenCensusSpan cannot be cast to com.google.cloud.spanner.OpenTelemetrySpan`. - This flaky failure is deterministic when tests are run sequentially in the same JVM fork (e.g. `SessionImplTest` followed by `GrpcResultSetTest`). - The static `activeTracingFramework` state in `SpannerOptions` is set to `OPEN_TELEMETRY` by other tests calling `SpannerOptions.enableOpenTelemetryTraces()` without proper cleanup. - As a result, when `GrpcResultSetTest` passes a hardcoded `OpenCensusSpan` to `TraceWrapper`, the wrapper attempts to cast it to `OpenTelemetrySpan`, leading to the `ClassCastException`. ## Solution - Added `@BeforeClass` in `GrpcResultSetTest` to explicitly call `SpannerOptions.resetActiveTracingFramework()`. - This ensures the static tracing state is reset to `OPEN_CENSUS` before the tests in this class run, preventing state pollution.
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds comprehensive unit tests to GrpcResultSetTest.java covering data availability, streaming delegation, and various concurrency and race conditions during stream closure and error handling. However, there is a critical issue where the @BeforeClass method beforeClass() is declared twice with the exact same signature, which will cause a compilation error and needs to be resolved.
| @org.junit.BeforeClass | ||
| public static void beforeClass() { | ||
| SpannerOptions.resetActiveTracingFramework(); | ||
| } | ||
|
|
||
| @org.junit.BeforeClass | ||
| public static void beforeClass() { | ||
| SpannerOptions.resetActiveTracingFramework(); | ||
| } |
There was a problem hiding this comment.
The @BeforeClass method beforeClass() is declared twice with the exact same signature. This will cause a compilation error. Please remove the duplicate method declaration.
| @org.junit.BeforeClass | |
| public static void beforeClass() { | |
| SpannerOptions.resetActiveTracingFramework(); | |
| } | |
| @org.junit.BeforeClass | |
| public static void beforeClass() { | |
| SpannerOptions.resetActiveTracingFramework(); | |
| } | |
| @BeforeClass | |
| public static void beforeClass() { | |
| SpannerOptions.resetActiveTracingFramework(); | |
| } |
References
- Do not use fully qualified class names if there is no class name conflict in the file and the class is already imported, as it unnecessarily reduces code readability.
There was a problem hiding this comment.
Code Review
This pull request introduces several unit tests to GrpcResultSetTest.java to verify data availability, stream closing behaviors, race conditions, and error handling in GrpcResultSet. The feedback highlights a critical compilation error caused by a duplicated @BeforeClass method beforeClass(), which must be removed.
| @org.junit.BeforeClass | ||
| public static void beforeClass() { | ||
| SpannerOptions.resetActiveTracingFramework(); | ||
| } | ||
|
|
||
| @org.junit.BeforeClass | ||
| public static void beforeClass() { | ||
| SpannerOptions.resetActiveTracingFramework(); | ||
| } |
There was a problem hiding this comment.
The @BeforeClass method beforeClass() is duplicated. This will cause a compilation error in Java because two methods with the same name and signature cannot coexist in the same class. Please remove the duplicate method.
| @org.junit.BeforeClass | |
| public static void beforeClass() { | |
| SpannerOptions.resetActiveTracingFramework(); | |
| } | |
| @org.junit.BeforeClass | |
| public static void beforeClass() { | |
| SpannerOptions.resetActiveTracingFramework(); | |
| } | |
| @BeforeClass | |
| public static void beforeClass() { | |
| SpannerOptions.resetActiveTracingFramework(); | |
| } |
References
- Do not use fully qualified class names if there is no class name conflict in the file and the class is already imported, as it unnecessarily reduces code readability.
|
Closing in favour of #14901 which already fixed this issue. |
Pull request was closed
Problem
closeWhileStreamIsStartingInvokesOnDoneSynchronouslyinGrpcResultSetTestthrows aClassCastException:com.google.cloud.spanner.OpenCensusSpan cannot be cast to com.google.cloud.spanner.OpenTelemetrySpan.SessionImplTestfollowed byGrpcResultSetTest).activeTracingFrameworkstate inSpannerOptionsis set toOPEN_TELEMETRYby other tests callingSpannerOptions.enableOpenTelemetryTraces()without proper cleanup.GrpcResultSetTestpasses a hardcodedOpenCensusSpantoTraceWrapper, the wrapper attempts to cast it toOpenTelemetrySpan, leading to theClassCastException.Solution
@BeforeClassinGrpcResultSetTestto explicitly callSpannerOptions.resetActiveTracingFramework().OPEN_CENSUSbefore the tests in this class run, preventing state pollution.