HBASE-30089 Rewrite AbstractTestAsyncTableScan and related sub classes#8099
HBASE-30089 Rewrite AbstractTestAsyncTableScan and related sub classes#8099Apache9 wants to merge 1 commit intoapache:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Migrates the async table scan test suite from JUnit 4 parameterized/rules-based execution to a JUnit 5 @TestTemplate + parameter provider model, while restructuring cluster/connection setup and OpenTelemetry test integration.
Changes:
- Replace JUnit 4
@RunWith(Parameterized.class)+@Parameters/@Parameterwith JUnit 5 parameter streams via@HBaseParameterizedTestTemplate. - Rewrite
AbstractTestAsyncTableScanlifecycle from rule chains to@BeforeAll/@AfterAll/@BeforeEach, usingHBaseTestingUtiland a sharedAsyncConnection. - Update tracing assertions to use a stored test name instead of the JUnit 4
TestNamerule.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestRawAsyncTableScan.java | Port parameterization to JUnit 5 template and update connection usage + trace naming. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableScanner.java | Port parameterization to JUnit 5 template and use supplied table factory for scanning. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableScanAll.java | Port parameterization to JUnit 5 template and update trace naming. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/client/TestAsyncTableScan.java | Port parameterization to JUnit 5 template and update connection usage + trace naming. |
| hbase-server/src/test/java/org/apache/hadoop/hbase/client/AbstractTestAsyncTableScan.java | Replace JUnit 4 rules with JUnit 5 lifecycle; rework mini-cluster/connection and OpenTelemetry integration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public final TestName testName = new TestName(); | ||
| @BeforeEach | ||
| public void setUp(TestInfo testInfo) { | ||
| methodName = testInfo.getTestMethod().get().getName(); |
There was a problem hiding this comment.
methodName is set to the Java method name only. With @TestTemplate parameterized invocations, multiple runs of the same test method will typically share this value, which can cause span name collisions (e.g., waitForSpan(hasName(methodName)) may match a span from a different invocation). Consider using testInfo.getDisplayName() (or storing both method name + display name) and using that consistently for TraceUtil.trace(...) and span matchers so each invocation has a unique parent span name.
| methodName = testInfo.getTestMethod().get().getName(); | |
| methodName = testInfo.getDisplayName(); |
| private Supplier<Scan> scanCreater; | ||
|
|
||
| @Parameter(0) | ||
| public String scanType; | ||
|
|
||
| @Parameter(1) | ||
| public Supplier<Scan> scanCreater; | ||
| // scanType is only for displaying | ||
| public TestRawAsyncTableScan(String scanType, Supplier<Scan> scanCreater) { | ||
| this.scanCreater = scanCreater; | ||
| } |
There was a problem hiding this comment.
The field/parameter name scanCreater appears to be a misspelling and is inconsistent with other classes in this PR that use scanCreator. Renaming to scanCreator will improve readability and reduce confusion.
| private Supplier<Scan> scanCreater; | ||
|
|
||
| @Parameter(0) | ||
| public String scanType; | ||
|
|
||
| @Parameter(1) | ||
| public Supplier<Scan> scanCreater; | ||
| // scanType is only for displaying | ||
| public TestAsyncTableScan(String scanType, Supplier<Scan> scanCreater) { | ||
| this.scanCreater = scanCreater; | ||
| } |
There was a problem hiding this comment.
Same as in TestRawAsyncTableScan: scanCreater looks like a misspelling. Rename to scanCreator for consistency with TestAsyncTableScanner/TestAsyncTableScanAll and to avoid propagating the typo.
| UTIL.getHBaseCluster().getRegionServerThreads().stream() | ||
| .map(JVMClusterUtil.RegionServerThread::getRegionServer).forEach( | ||
| rs -> assertEquals( | ||
| "The scanner count of " + rs.getServerName() + " is " | ||
| + rs.getRSRpcServices().getScannersCount(), | ||
| 0, rs.getRSRpcServices().getScannersCount())); | ||
| rs -> assertEquals(0, rs.getRSRpcServices().getScannersCount(), "The scanner count of " | ||
| + rs.getServerName() + " is " + rs.getRSRpcServices().getScannersCount())); |
There was a problem hiding this comment.
rs.getRSRpcServices().getScannersCount() is called twice per region server (once as the assertion actual value and again to build the message). Store the count in a local variable and reuse it so the assertion message always matches the asserted value and avoids duplicate calls.
| private Supplier<AsyncTable<?>> getTable; | ||
|
|
||
| @Parameter(0) | ||
| public String tableType; | ||
| private Supplier<Scan> scanCreator; | ||
|
|
||
| @Parameter(1) | ||
| public Supplier<AsyncTable<?>> getTable; | ||
|
|
||
| @Parameter(2) | ||
| public String scanType; | ||
|
|
||
| @Parameter(3) | ||
| public Supplier<Scan> scanCreator; | ||
| // tableType and scanType are only for displaying | ||
| public TestAsyncTableScanner(String tableType, Supplier<AsyncTable<?>> getTable, String scanType, | ||
| Supplier<Scan> scanCreator) { | ||
| this.getTable = getTable; | ||
| this.scanCreator = scanCreator; | ||
| } |
There was a problem hiding this comment.
These constructor-initialized fields can be made final to document immutability of the test configuration per invocation and prevent accidental reassignment later.
No description provided.