Copilot commented on code in PR #7999:
URL: https://github.com/apache/hbase/pull/7999#discussion_r3005146571
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestSuperUserQuotaPermissions.java:
##########
@@ -96,16 +87,17 @@ public static void setupMiniCluster() throws Exception {
TEST_UTIL.startMiniCluster(1);
}
- @AfterClass
+ @AfterAll
public static void tearDown() throws Exception {
TEST_UTIL.shutdownMiniCluster();
}
- @Before
- public void removeAllQuotas() throws Exception {
+ @BeforeEach
+ public void removeAllQuotas(TestInfo testInfo) throws Exception {
final Connection conn = TEST_UTIL.getConnection();
if (helper == null) {
- helper = new SpaceQuotaHelperForTests(TEST_UTIL, testName, COUNTER);
+ helper = new SpaceQuotaHelperForTests(TEST_UTIL,
+ () -> testInfo.getTestMethod().get().getName(), COUNTER);
}
Review Comment:
`SpaceQuotaHelperForTests` is only initialized once (when `helper == null`),
but the `Supplier<String>` you pass captures the *current* `testInfo` instance.
On subsequent tests, the supplier will still return the first test method name,
which can cause table/file name collisions and make later tests operate on the
wrong resources. Consider either re-initializing `helper` in every
`@BeforeEach`, or store the current method name in a field updated per-test and
pass a supplier that reads that field (e.g., set `this.testName = ...` then
pass `() -> this.testName`).
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestQuotaObserverChoreWithMiniCluster.java:
##########
@@ -92,16 +82,17 @@ public static void setUp() throws Exception {
TEST_UTIL.startMiniCluster(1);
}
- @AfterClass
+ @AfterAll
public static void tearDown() throws Exception {
TEST_UTIL.shutdownMiniCluster();
}
- @Before
- public void removeAllQuotas() throws Exception {
+ @BeforeEach
+ public void removeAllQuotas(TestInfo testInfo) throws Exception {
final Connection conn = TEST_UTIL.getConnection();
if (helper == null) {
- helper = new SpaceQuotaHelperForTests(TEST_UTIL, testName, COUNTER);
+ helper = new SpaceQuotaHelperForTests(TEST_UTIL,
+ () -> testInfo.getTestMethod().get().getName(), COUNTER);
}
Review Comment:
`helper` is cached after the first test run, but the `Supplier<String>` you
pass closes over that first `testInfo` parameter. This means `helper` will keep
generating names based on the first test method for all subsequent tests, which
can lead to resource/name collisions and flaky behavior. Recreate `helper`
every `@BeforeEach`, or update a per-test field (e.g., `this.testName`) and use
a supplier that reads that field.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/quotas/TestMasterQuotasObserver.java:
##########
@@ -38,47 +37,39 @@
import org.apache.hadoop.hbase.master.MasterCoprocessorHost;
import org.apache.hadoop.hbase.testclassification.MediumTests;
import org.apache.hadoop.hbase.util.Bytes;
-import org.junit.AfterClass;
-import org.junit.Before;
-import org.junit.BeforeClass;
-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.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInfo;
/**
* Test class for {@link MasterQuotasObserver}.
*/
-@Category(MediumTests.class)
+@Tag(MediumTests.TAG)
public class TestMasterQuotasObserver {
- @ClassRule
- public static final HBaseClassTestRule CLASS_RULE =
- HBaseClassTestRule.forClass(TestMasterQuotasObserver.class);
-
private static final HBaseTestingUtil TEST_UTIL = new HBaseTestingUtil();
private static SpaceQuotaHelperForTests helper;
- @Rule
- public TestName testName = new TestName();
-
- @BeforeClass
+ @BeforeAll
public static void setUp() throws Exception {
Configuration conf = TEST_UTIL.getConfiguration();
conf.setBoolean(QuotaUtil.QUOTA_CONF_KEY, true);
TEST_UTIL.startMiniCluster(1);
}
- @AfterClass
+ @AfterAll
public static void tearDown() throws Exception {
TEST_UTIL.shutdownMiniCluster();
}
- @Before
- public void removeAllQuotas() throws Exception {
+ @BeforeEach
+ public void removeAllQuotas(TestInfo testInfo) throws Exception {
if (helper == null) {
- helper = new SpaceQuotaHelperForTests(TEST_UTIL, testName, new
AtomicLong());
+ helper = new SpaceQuotaHelperForTests(TEST_UTIL,
+ () -> testInfo.getTestMethod().get().getName(), new AtomicLong());
}
Review Comment:
`helper` is `static` and only initialized once, but the supplied test-name
lambda captures the `testInfo` from the first `@BeforeEach` invocation. Later
tests will still use the first test method name when generating tables/paths,
which can cause collisions and incorrect cleanup. Prefer initializing `helper`
per-test, or store the current method name in a mutable field updated each
`@BeforeEach` and pass `() -> field` to the helper.
--
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]
