Copilot commented on code in PR #8046:
URL: https://github.com/apache/hbase/pull/8046#discussion_r3067995633
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/mob/TestMobStoreCompaction.java:
##########
@@ -123,10 +115,14 @@ public TestMobStoreCompaction(Boolean useFileBasedSFT) {
this.useFileBasedSFT = useFileBasedSFT;
}
- @Parameterized.Parameters
- public static Collection<Boolean> data() {
+ public static Stream<Arguments> parameters() {
Boolean[] data = { false, true };
- return Arrays.asList(data);
+ return Arrays.asList(data).stream().map(Arguments::of);
+ }
+
+ @BeforeEach
+ public void setUp(TestInfo testInfo) {
+ testMethodName = testInfo.getTestMethod().get().getName();
}
Review Comment:
In this parameterized test (via @HBaseParameterizedTestTemplate), deriving
the table/test identifier from only testInfo.getTestMethod().get().getName()
means both parameter invocations will reuse the same name. This can cause
collisions between iterations (e.g., leftover FS/table artifacts if a previous
iteration fails), since JUnit4 parameterized method names previously included a
"[index]" suffix. Consider incorporating the parameterized display name (e.g.,
testInfo.getDisplayName()) or the parameter value/index into the generated
name. See the existing pattern in
BasicReadWriteWithDifferentConnectionRegistriesTestBase.java:111-115 which
mixes getDisplayName() into the table name.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/ipc/TestNettyRpcServer.java:
##########
@@ -51,48 +50,48 @@
import org.apache.hadoop.hbase.testclassification.RPCTests;
import org.apache.hadoop.hbase.util.Bytes;
import org.apache.hadoop.hbase.util.LoadTestKVGenerator;
-import org.junit.After;
-import org.junit.Before;
-import org.junit.ClassRule;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.experimental.categories.Category;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-import org.junit.runners.Parameterized.Parameters;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.TestInfo;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.params.provider.Arguments;
import org.apache.hbase.thirdparty.io.netty.handler.ssl.SslHandler;
-@Category({ RPCTests.class, MediumTests.class })
-@RunWith(Parameterized.class)
+@Tag(RPCTests.TAG)
+@Tag(MediumTests.TAG)
+@HBaseParameterizedTestTemplate(name = "{index}: allocatorType={0}")
public class TestNettyRpcServer {
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestNettyRpcServer.class);
-
private static final byte[] FAMILY = Bytes.toBytes("f");
private static final byte[] QUALIFIER = Bytes.toBytes("q");
private static final int NUM_ROWS = 100;
private static final int MIN_LEN = 1000;
private static final int MAX_LEN = 1000000;
protected static final LoadTestKVGenerator GENERATOR = new
LoadTestKVGenerator(MIN_LEN, MAX_LEN);
protected static HBaseTestingUtil TEST_UTIL;
+ protected TableName tableName;
+ protected final String allocatorType;
- @Rule
- public TableNameTestRule name = new TableNameTestRule();
+ public TestNettyRpcServer(String allocatorType) {
+ this.allocatorType = allocatorType;
+ }
- @Parameterized.Parameter
- public String allocatorType;
+ public static Stream<Arguments> parameters() {
+ return Arrays
+ .stream(
+ new Object[] { NettyRpcServer.POOLED_ALLOCATOR_TYPE,
NettyRpcServer.UNPOOLED_ALLOCATOR_TYPE,
+ NettyRpcServer.HEAP_ALLOCATOR_TYPE,
SimpleByteBufAllocator.class.getName() })
+ .map(Arguments::of);
+ }
- @Parameters
- public static Collection<Object[]> parameters() {
- return Arrays.asList(new Object[][] { {
NettyRpcServer.POOLED_ALLOCATOR_TYPE },
- { NettyRpcServer.UNPOOLED_ALLOCATOR_TYPE }, {
NettyRpcServer.HEAP_ALLOCATOR_TYPE },
- { SimpleByteBufAllocator.class.getName() } });
+ @BeforeEach
+ public void setUpTable(TestInfo testInfo) {
+ tableName = TableName.valueOf(testInfo.getTestMethod().get().getName());
Review Comment:
This test is parameterized via @HBaseParameterizedTestTemplate(name =
"{index}: allocatorType={0}"), but tableName is derived from only the method
name, so all allocatorType iterations share the same table name. If the mini
cluster/root dir is reused across iterations, this increases the chance of name
collisions and flakiness. Consider incorporating testInfo.getDisplayName()
(contains the index/param) or allocatorType into the table name, consistent
with BasicReadWriteWithDifferentConnectionRegistriesTestBase.java:111-115.
```suggestion
String sanitizedAllocatorType =
allocatorType.replaceAll("[^a-zA-Z0-9_.-]", "_");
tableName = TableName
.valueOf(testInfo.getTestMethod().get().getName() + "_" +
sanitizedAllocatorType);
```
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/mob/TestMobStoreCompaction.java:
##########
@@ -143,16 +139,17 @@ private void init(Configuration conf, long mobThreshold)
throws Exception {
compactionThreshold = conf.getInt("hbase.hstore.compactionThreshold", 3);
familyDescriptor =
ColumnFamilyDescriptorBuilder.newBuilder(COLUMN_FAMILY).setMobEnabled(true)
Review Comment:
In init(), the local variable `HBaseTestingUtil UTIL = new
HBaseTestingUtil(conf);` shadows the class-level static `UTIL`. Later code
mixes both (local UTIL for createRegionAndWAL, but class-level UTIL for
configuration elsewhere), which is easy to misuse and makes it unclear which
dataTestDir is being used. Consider removing the shadowing local variable or
renaming/storing the instance used for getDataTestDir so setup/cleanup operate
on the same paths.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/mob/TestMobCompactionWithDefaults.java:
##########
@@ -133,13 +125,14 @@ protected void htuStart() throws Exception {
protected void additonalConfigSetup() {
}
- @Before
- public void setUp() throws Exception {
+ @BeforeEach
+ public void setUp(TestInfo testInfo) throws Exception {
+ testMethodName = testInfo.getTestMethod().get().getName();
htuStart();
admin = HTU.getAdmin();
familyDescriptor =
ColumnFamilyDescriptorBuilder.newBuilder(fam).setMobEnabled(true)
.setMobThreshold(mobLen).setMaxVersions(1).build();
- tableDescriptor =
HTU.createModifyableTableDescriptor(TestMobUtils.getTableName(test))
+ tableDescriptor =
HTU.createModifyableTableDescriptor(TestMobUtils.getTableName(testMethodName))
.setColumnFamily(familyDescriptor).build();
Review Comment:
This class is parameterized with @HBaseParameterizedTestTemplate, but the
table name seed is set to the bare method name only. Each parameter iteration
will therefore use the same table/snapshot names, which can lead to collisions
across iterations (especially since the same work dir/TestData dir may be
reused between mini cluster restarts). Suggest incorporating
testInfo.getDisplayName() (which includes the parameter index/value) when
constructing testMethodName, similar to
BasicReadWriteWithDifferentConnectionRegistriesTestBase.java:111-115.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/mob/TestDefaultMobStoreFlusher.java:
##########
@@ -64,47 +59,46 @@ public class TestDefaultMobStoreFlusher {
private final static byte[] value1 = Bytes.toBytes("value1");
private final static byte[] value2 = Bytes.toBytes("value2");
- @Rule
- public TestName name = new TestName();
+ private String testMethodName;
protected Boolean useFileBasedSFT;
public TestDefaultMobStoreFlusher(Boolean useFileBasedSFT) {
this.useFileBasedSFT = useFileBasedSFT;
}
- @Parameterized.Parameters
- public static Collection<Boolean> data() {
+ public static Stream<Arguments> parameters() {
Boolean[] data = { false, true };
- return Arrays.asList(data);
+ return Arrays.asList(data).stream().map(Arguments::of);
}
- @Before
- public void setUpBefore() throws Exception {
+ @BeforeEach
+ public void setUpBefore(TestInfo testInfo) throws Exception {
+ testMethodName = testInfo.getTestMethod().get().getName();
if (useFileBasedSFT) {
TEST_UTIL.getConfiguration().set(StoreFileTrackerFactory.TRACKER_IMPL,
"org.apache.hadoop.hbase.regionserver.storefiletracker.FileBasedStoreFileTracker");
}
TEST_UTIL.startMiniCluster(1);
}
Review Comment:
Because this test is parameterized via @HBaseParameterizedTestTemplate,
using only testInfo.getTestMethod().get().getName() for testMethodName means
both parameter iterations will generate identical table names. If any state
persists across iterations (e.g., same TEST_UTIL/root dir reuse), this can make
the second iteration flaky. Consider folding in testInfo.getDisplayName() (or
the parameter value/index) into the generated name, following the pattern in
BasicReadWriteWithDifferentConnectionRegistriesTestBase.java:111-115.
--
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]
