Copilot commented on code in PR #8008:
URL: https://github.com/apache/hbase/pull/8008#discussion_r3014319943
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/filter/FilterTestingCluster.java:
##########
@@ -94,21 +89,20 @@ private static void initialize(Configuration conf) {
try {
admin = TEST_UTIL.getAdmin();
} catch (MasterNotRunningException e) {
- assertNull("Master is not running", e);
+ assertNull(e, "Master is not running");
} catch (ZooKeeperConnectionException e) {
- assertNull("Cannot connect to ZooKeeper", e);
+ assertNull(e, "Cannot connect to ZooKeeper");
} catch (IOException e) {
- assertNull("IOException", e);
+ assertNull(e, "IOException");
}
}
- @BeforeClass
public static void setUp() throws Exception {
TEST_UTIL.startMiniCluster(1);
initialize(TEST_UTIL.getConfiguration());
}
Review Comment:
FilterTestingCluster#setUp is no longer annotated with @BeforeAll, but
tearDown is @AfterAll. This means subclasses must remember to call
FilterTestingCluster.setUp() manually; forgetting will leave admin/null cluster
state and cause confusing failures. Consider annotating setUp with `@BeforeAll`
(static) to restore the prior JUnit4 behavior, and then remove the
now-redundant explicit setUp calls in subclasses (e.g.,
TestScanRowPrefix/TestFilterWithScanLimits) to avoid double-starting the mini
cluster.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/filter/TestQualifierFilterWithEmptyQualifier.java:
##########
@@ -147,12 +138,12 @@ private void verifyScanNoEarlyOut(Scan s, long
expectedRows, long expectedKeys)
if (results.isEmpty()) {
break;
}
- assertTrue("Scanned too many rows! Only expected " + expectedRows
- + " total but already scanned " + (i + 1), expectedRows > i);
- assertEquals("Expected " + expectedKeys + " keys per row but " +
"returned " + results.size(),
- expectedKeys, results.size());
+ assertTrue(expectedRows > i, "Scanned too many rows! Only expected " +
expectedRows
+ + " total but already scanned " + (i + 1));
+ assertEquals(expectedKeys, results.size(),
+ "Expected " + expectedKeys + " keys per row but " + "returned " +
results.size());
results.clear();
}
- assertEquals("Expected " + expectedRows + " rows but scanned " + i + "
rows", expectedRows, i);
+ assertEquals(i, expectedRows, "Expected " + expectedRows + " rows but
scanned " + i + " rows");
Review Comment:
In JUnit Jupiter, assertEquals expects (expected, actual, message). Here the
arguments are reversed (i vs expectedRows), which doesn't change pass/fail but
will swap expected/actual in failure output and make debugging harder. Please
switch to assertEquals(expectedRows, i, ...).
```suggestion
assertEquals(expectedRows, i, "Expected " + expectedRows + " rows but
scanned " + i + " rows");
```
--
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]
