Copilot commented on code in PR #8191:
URL: https://github.com/apache/hbase/pull/8191#discussion_r3182416278
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionTracing.java:
##########
@@ -73,23 +68,23 @@ public class TestHRegionTracing {
private static byte[] VALUE = Bytes.toBytes("value");
- @Rule
- public final OpenTelemetryRule traceRule = OpenTelemetryRule.create();
+ @RegisterExtension
+ public static final OpenTelemetryExtension traceRule =
OpenTelemetryExtension.create();
- @Rule
- public final TableNameTestRule tableNameRule = new TableNameTestRule();
+ @RegisterExtension
+ public final TableNameTestExtension tableNameRule = new
TableNameTestExtension();
private WAL wal;
private HRegion region;
- @AfterClass
+ @AfterAll
public static void tearDownAfterClass() throws IOException {
UTIL.cleanupTestDir();
}
- @Before
- public void setUp() throws IOException {
+ @BeforeEach
+ public void setUp() throws Throwable {
Review Comment:
OpenTelemetryExtension is registered as a static extension, but the test
does not clear previously exported spans between test methods. This can cause
the collected span list to grow across tests and make assertions more fragile.
Consider calling traceRule.clearSpans() at the start of `@BeforeEach` (or
otherwise resetting the exporter) to keep tests isolated.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMetricsTableRequests.java:
##########
@@ -35,27 +34,23 @@
import org.apache.hadoop.hbase.regionserver.metrics.MetricsTableRequests;
import org.apache.hadoop.hbase.testclassification.RegionServerTests;
import org.apache.hadoop.hbase.testclassification.SmallTests;
-import org.junit.ClassRule;
-import org.junit.Test;
-import org.junit.experimental.categories.Category;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
-@Category({ RegionServerTests.class, SmallTests.class })
+@Tag(RegionServerTests.TAG)
+@Tag(SmallTests.TAG)
public class TestMetricsTableRequests {
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestMetricsTableRequests.class);
-
@Test
public void testMetricsTableLatencies() {
TableName tn1 = TableName.valueOf("table1");
TableName tn2 = TableName.valueOf("table2");
MetricsTableRequests requests1 = new MetricsTableRequests(tn1, new
Configuration());
MetricsTableRequests requests2 = new MetricsTableRequests(tn2, new
Configuration());
Review Comment:
This test registers metrics in MetricRegistries.global(), but does not clear
the global registry after each test. Other RegionServer metrics tests typically
call MetricRegistries.global().clear() in `@AfterEach` to avoid cross-test
interference. Consider adding an `@AfterEach` that clears the global registry
(and any created per-test registries) to keep the suite isolated.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestMajorCompaction.java:
##########
@@ -121,16 +114,17 @@ public TestMajorCompaction(String compType) {
(byte) (thirdRowBytes[START_KEY_BYTES.length - 1] + 2);
}
- @Before
- public void setUp() throws Exception {
- this.htd = UTIL.createTableDescriptor(
- TableName.valueOf(name.getMethodName().replace('[', 'i').replace(']',
'i')),
- ColumnFamilyDescriptorBuilder.DEFAULT_MIN_VERSIONS, 3,
HConstants.FOREVER,
- ColumnFamilyDescriptorBuilder.DEFAULT_KEEP_DELETED);
+ @BeforeEach
+ public void setUp(TestInfo testInfo) throws Exception {
+ this.name = testInfo.getTestMethod().get().getName();
+ this.htd =
+ UTIL.createTableDescriptor(TableName.valueOf(name.replace('[',
'i').replace(']', 'i')),
+ ColumnFamilyDescriptorBuilder.DEFAULT_MIN_VERSIONS, 3,
HConstants.FOREVER,
+ ColumnFamilyDescriptorBuilder.DEFAULT_KEEP_DELETED);
Review Comment:
In this parameterized JUnit5 `@TestTemplate` class, TableName is derived
only from the test method name (via TestInfo#getTestMethod). That means all
parameter invocations for the same method will reuse the same TableName, which
can cause collisions/flakiness if invocations run concurrently or if prior
state isn't fully cleaned. Consider incorporating the constructor parameter
(compType) or another per-invocation discriminator into the TableName (and
sanitize to valid TableName chars).
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestHRegionTracing.java:
##########
@@ -102,7 +97,7 @@ public void setUp() throws IOException {
region = UTIL.createLocalHRegion(info, desc);
}
Review Comment:
In setUp(), a region is created with HRegion.createHRegion(...) using the
explicitly created WAL, but then `region` is immediately overwritten by
UTIL.createLocalHRegion(...). This looks like an accidental leftover from the
migration and can leak the first region and/or leave `wal` out of sync with the
actual region under test. Consider using only one of these creation paths (and
ensure the WAL/region lifecycle matches what you close in tearDown).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
