Copilot commented on code in PR #8185:
URL: https://github.com/apache/hbase/pull/8185#discussion_r3210848490
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestCacheOnWriteInSchema.java:
##########
@@ -141,19 +131,15 @@ public TestCacheOnWriteInSchema(CacheOnWriteType cowType)
{
System.out.println(testDescription);
}
- @Parameters
- public static Collection<Object[]> getParameters() {
- List<Object[]> cowTypes = new ArrayList<>();
- for (CacheOnWriteType cowType : CacheOnWriteType.values()) {
- cowTypes.add(new Object[] { cowType });
- }
- return cowTypes;
+ public static Stream<Arguments> parameters() {
+ return Stream.of(CacheOnWriteType.values()).map(Arguments::of);
}
- @Before
- public void setUp() throws IOException {
+ @BeforeEach
+ public void setUp(TestInfo testInfo) throws IOException {
+ this.name = testInfo.getTestMethod().get().getName();
// parameterized tests add [#] suffix get rid of [ and ].
- table = Bytes.toBytes(name.getMethodName().replaceAll("[\\[\\]]", "_"));
+ table = Bytes.toBytes(name.replaceAll("[\\[\\]]", "_"));
Review Comment:
In this parameterized test class (@HBaseParameterizedTestTemplate), deriving
`name` from `testInfo.getTestMethod().get().getName()` drops the parameter
index/value, so `table` becomes identical across parameter invocations. The old
JUnit4 TestName included a `[n]` suffix (as the comment suggests) to keep
per-parameter resources distinct; without that, concurrent/parallel execution
or incomplete cleanup can cause table/WAL dir collisions. Consider
incorporating `cowType` (or the invocation index/display name) into the
generated table name, and sanitize it to a valid TableName.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDefaultMemStore.java:
##########
@@ -1004,9 +1001,9 @@ public void testShouldFlushMeta() throws Exception {
HRegion meta =
HRegion.createHRegion(RegionInfoBuilder.FIRST_META_REGIONINFO, testDir, conf,
tds.get(TableName.META_TABLE_NAME),
wFactory.getWAL(RegionInfoBuilder.FIRST_META_REGIONINFO));
// parameterized tests add [#] suffix get rid of [ and ].
- TableDescriptor desc = TableDescriptorBuilder
-
.newBuilder(TableName.valueOf(name.getMethodName().replaceAll("[\\[\\]]", "_")))
- .setColumnFamily(ColumnFamilyDescriptorBuilder.of("foo")).build();
+ TableDescriptor desc =
+
TableDescriptorBuilder.newBuilder(TableName.valueOf(name.replaceAll("[\\[\\]]",
"_")))
+ .setColumnFamily(ColumnFamilyDescriptorBuilder.of("foo")).build();
Review Comment:
This TableDescriptor/TableName is derived from `name` which is now just the
method name, so under parameterized subclasses this will no longer be unique
per parameter (the comment suggests it used to rely on `[n]`). That can make
multiple parameter invocations share the same table name and on-disk layout.
Consider basing the TableName on a parameter-aware/sanitized identifier (e.g.,
display name or explicit parameter suffix) to preserve uniqueness.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestDefaultMemStore.java:
##########
@@ -79,43 +75,45 @@
import org.apache.hbase.thirdparty.com.google.common.collect.Lists;
/** memstore test case */
-@Category({ RegionServerTests.class, MediumTests.class })
+@Tag(RegionServerTests.TAG)
+@Tag(MediumTests.TAG)
public class TestDefaultMemStore {
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestDefaultMemStore.class);
-
private static final Logger LOG =
LoggerFactory.getLogger(TestDefaultMemStore.class);
- @Rule
- public TestName name = new TestName();
+
protected AbstractMemStore memstore;
protected static final int ROW_COUNT = 10;
protected static final int SCAN_ROW_COUNT = 25000;
protected static final int QUALIFIER_COUNT = ROW_COUNT;
protected static final byte[] FAMILY = Bytes.toBytes("column");
protected MultiVersionConcurrencyControl mvcc;
protected ChunkCreator chunkCreator;
+ protected String name;
- private String getName() {
- return this.name.getMethodName();
+ @BeforeEach
+ public void setUp(TestInfo testInfo) throws Exception {
+ this.name = testInfo.getTestMethod().get().getName();
+ internalSetUp();
+ createChunkCreator();
Review Comment:
`name` is currently set to the bare Java method name via
`testInfo.getTestMethod().get().getName()`. When this base class is executed
under class-parameterized subclasses, the method name will be identical across
parameter invocations, which defeats the existing logic/comments that expect a
`[n]` suffix for uniqueness. Consider using a parameter-aware identifier (e.g.,
`TestInfo.getDisplayName()` or an injected parameter value/index) and
sanitizing it once for any later use in TableName creation.
--
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]
