Copilot commented on code in PR #8118:
URL: https://github.com/apache/hbase/pull/8118#discussion_r3129837387
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestCloneSnapshotProcedureFileBasedSFT.java:
##########
@@ -20,24 +20,14 @@
import static
org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.TRACKER_IMPL;
import static
org.apache.hadoop.hbase.regionserver.storefiletracker.StoreFileTrackerFactory.Trackers.FILE;
-import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.testclassification.MasterTests;
import org.apache.hadoop.hbase.testclassification.MediumTests;
-import org.junit.BeforeClass;
-import org.junit.ClassRule;
-import org.junit.experimental.categories.Category;
+import org.junit.jupiter.api.Tag;
-@Category({ MasterTests.class, MediumTests.class })
+@Tag(MasterTests.TAG)
+@Tag(MediumTests.TAG)
public class TestCloneSnapshotProcedureFileBasedSFT extends
TestCloneSnapshotProcedure {
-
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestCloneSnapshotProcedureFileBasedSFT.class);
-
- @BeforeClass
- public static void setupCluster() throws Exception {
+ static {
UTIL.getConfiguration().set(TRACKER_IMPL, FILE.name());
-
UTIL.getConfiguration().setInt(MasterProcedureConstants.MASTER_PROCEDURE_THREADS,
1);
- UTIL.startMiniCluster(1);
}
Review Comment:
This static initializer mutates `TestTableDDLProcedureBase.UTIL`'s shared
`Configuration` (`TRACKER_IMPL`) but never restores the previous value. Since
`UTIL` is a static field on the base class and is shared across many procedure
tests, this can leak configuration into later tests and cause order-dependent
failures. Consider capturing the prior value and restoring it in an `@AfterAll`
hook (or otherwise isolating the configuration change).
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestServerRemoteProcedure.java:
##########
@@ -47,37 +48,38 @@
import org.apache.hadoop.hbase.testclassification.MasterTests;
import org.apache.hadoop.hbase.testclassification.MediumTests;
import org.apache.hadoop.hbase.util.Bytes;
-import org.junit.After;
-import org.junit.Assert;
-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.rules.TestName;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import
org.apache.hbase.thirdparty.com.google.common.util.concurrent.ThreadFactoryBuilder;
import org.apache.hadoop.hbase.shaded.protobuf.generated.AdminProtos;
-@Category({ MasterTests.class, MediumTests.class })
+@Tag(MasterTests.TAG)
+@Tag(MediumTests.TAG)
public class TestServerRemoteProcedure {
private static final Logger LOG =
LoggerFactory.getLogger(TestServerRemoteProcedure.class);
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestServerRemoteProcedure.class);
- @Rule
- public TestName name = new TestName();
+
private HBaseTestingUtility util;
private MockRSProcedureDispatcher rsDispatcher;
private MockMasterServices master;
private AssignmentManager am;
// Simple executor to run some simple tasks.
private ScheduledExecutorService executor;
- @Before
+ private String testMethodName;
+
+ @BeforeEach
+ public void setTestMethod(TestInfo testInfo) {
+ testMethodName = testInfo.getTestMethod().get().getName();
+ }
+
+ @BeforeEach
Review Comment:
`testMethodName` is captured from `TestInfo` but not referenced anywhere in
this class. Consider removing this unused field and the associated
`@BeforeEach` method to keep the test setup minimal.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestReloadQuotasProcedure.java:
##########
@@ -17,55 +17,52 @@
*/
package org.apache.hadoop.hbase.master.procedure;
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertThrows;
-import static org.junit.Assert.assertTrue;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
import java.io.IOException;
import java.util.Optional;
import java.util.concurrent.Future;
-import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.HBaseTestingUtility;
import org.apache.hadoop.hbase.ServerName;
import org.apache.hadoop.hbase.master.assignment.MockMasterServices;
import org.apache.hadoop.hbase.procedure2.RemoteProcedureDispatcher;
import org.apache.hadoop.hbase.testclassification.MasterTests;
import org.apache.hadoop.hbase.testclassification.MediumTests;
-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.rules.TestName;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
-@Category({ MasterTests.class, MediumTests.class })
+@Tag(MasterTests.TAG)
+@Tag(MediumTests.TAG)
public class TestReloadQuotasProcedure {
private static final Logger LOG =
LoggerFactory.getLogger(TestReloadQuotasProcedure.class);
+ private String testMethodName;
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestReloadQuotasProcedure.class);
-
- @Rule
- public TestName name = new TestName();
+ @BeforeEach
+ public void setTestMethod(TestInfo testInfo) {
+ testMethodName = testInfo.getTestMethod().get().getName();
+ }
Review Comment:
`testMethodName` is set in `@BeforeEach` but never used in this test class.
Consider removing the field and the extra `@BeforeEach` method (or using it for
per-test naming) to avoid dead code.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestSnapshotProcedure.java:
##########
@@ -58,14 +55,10 @@
import
org.apache.hadoop.hbase.shaded.protobuf.generated.MasterProcedureProtos.SnapshotState;
import org.apache.hadoop.hbase.shaded.protobuf.generated.SnapshotProtos;
-@Category({ MasterTests.class, MediumTests.class })
+@Tag(MasterTests.TAG)
public class TestSnapshotProcedure {
Review Comment:
This class previously covered both Master and Medium test classifications,
but now only has `@Tag(MasterTests.TAG)`. This makes it inconsistent with other
snapshot procedure tests (for example `TestSnapshotProcedureWithLockTimeout`
uses both `@Tag(MasterTests.TAG)` and `@Tag(MediumTests.TAG)`). Consider adding
back `@Tag(MediumTests.TAG)` (and the corresponding import) to preserve test
categorization.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestWALProcedureStoreOnHDFS.java:
##########
@@ -109,7 +105,7 @@ public void tearDown() throws Exception {
}
Review Comment:
`tearDown()` is not annotated with `@AfterEach` (and is not called
manually), so the MiniDFS/MiniCluster started in `setupDFS()` will never be
shut down between tests. This can leak DataNodes/threads and make subsequent
tests in this class (or later classes) flaky. Consider adding `@AfterEach`
and/or ensuring shutdown happens via `try/finally` in each test that calls
`setupDFS()`.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestReopenTableRegionsProcedureSpecificRegions.java:
##########
@@ -407,8 +402,8 @@ public void testEmptyTableWithNoRegions() throws Exception {
long procId = getProcExec().submitProcedure(proc);
ProcedureTestingUtility.waitProcedure(getProcExec(), procId);
- assertFalse("Procedure should complete successfully even with no regions",
proc.isFailed());
- assertEquals("Should handle empty table gracefully", regionCount,
proc.getRegionsReopened());
+ assertFalse(proc.isFailed(), "Procedure should complete successfully even
with no regions");
+ assertEquals(proc.getRegionsReopened(), regionCount, "Should handle empty
table gracefully");
Review Comment:
`assertEquals` arguments are reversed here (`expected` should be
`regionCount`, `actual` should be `proc.getRegionsReopened()`). The assertion
will still pass/fail the same way, but failure output will be misleading.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/procedure/TestSCPWithoutMetaWithoutZKCoordinated.java:
##########
@@ -18,20 +18,13 @@
package org.apache.hadoop.hbase.master.procedure;
import org.apache.hadoop.conf.Configuration;
-import org.apache.hadoop.hbase.HBaseClassTestRule;
import org.apache.hadoop.hbase.HConstants;
import org.apache.hadoop.hbase.testclassification.MasterTests;
-import org.apache.hadoop.hbase.testclassification.MediumTests;
-import org.junit.ClassRule;
-import org.junit.experimental.categories.Category;
+import org.junit.jupiter.api.Tag;
-@Category({ MasterTests.class, MediumTests.class })
+@Tag(MasterTests.TAG)
public class TestSCPWithoutMetaWithoutZKCoordinated extends TestSCPWithoutMeta
{
Review Comment:
This test variant no longer has a size classification tag (it only has
`@Tag(MasterTests.TAG)`). Other SCP "without ZK coordinated" variants in this
package keep the size tag (e.g. `TestSCPWithoutZKCoordinated` has
`@Tag(LargeTests.TAG)`). Consider adding the appropriate size tag here (likely
`@Tag(LargeTests.TAG)`) to avoid changing which test suites include it.
--
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]
