Copilot commented on code in PR #8199:
URL: https://github.com/apache/hbase/pull/8199#discussion_r3202177704
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerNoMaster.java:
##########
@@ -245,12 +241,12 @@ public void testCancelOpeningWithoutZK() throws Exception
{
ProtobufUtil.buildCloseRegionRequest(getRS().getServerName(),
regionName);
try {
getRS().getRpcServices().closeRegion(null, crr);
- Assert.assertTrue(false);
+ assertTrue(false);
Review Comment:
Avoid using `assertTrue(false)`/`assertTrue(false, ...)` as a failure
mechanism; it produces less clear failures and can be optimized away in
refactors. Use `fail(...)` (already imported) to express the expected exception
path clearly.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestParallelPut.java:
##########
@@ -228,7 +221,7 @@ public void run() {
assertEquals(OperationStatusCode.SUCCESS,
ret[0].getOperationStatusCode());
assertGet(this.region, rowkey, fam1, qual1, value);
} catch (IOException e) {
- assertTrue("Thread id " + threadNumber + " operation " + i + "
failed.", false);
+ assertTrue(false, "Thread id " + threadNumber + " operation " + i +
" failed.");
Review Comment:
Using `assertTrue(false, ...)` in a catch block to indicate failure is
brittle and obscures intent. Prefer `fail(...)` (or rethrow) so the test
clearly communicates that the code path should be unreachable.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerHostname.java:
##########
@@ -179,14 +173,16 @@ public void
testConflictRegionServerHostnameConfigurationsAbortServer() throws E
} catch (Exception e) {
Throwable t1 = e.getCause();
Throwable t2 = t1.getCause();
- assertTrue(t1.getMessage() + " - " + t2.getMessage(),
-
t2.getMessage().contains(HRegionServer.UNSAFE_RS_HOSTNAME_DISABLE_MASTER_REVERSEDNS_KEY
- + " and " + DNS.UNSAFE_RS_HOSTNAME_KEY + " are mutually
exclusive"));
+ assertTrue(
+ t2.getMessage()
+
.contains(HRegionServer.UNSAFE_RS_HOSTNAME_DISABLE_MASTER_REVERSEDNS_KEY + "
and "
+ + DNS.UNSAFE_RS_HOSTNAME_KEY + " are mutually exclusive"),
+ t1.getMessage() + " - " + t2.getMessage());
return;
} finally {
TEST_UTIL.shutdownMiniCluster();
}
- assertTrue("Failed to validate against conflict hostname
configurations", false);
+ assertTrue(false, "Failed to validate against conflict hostname
configurations");
Review Comment:
This assertion uses `assertTrue(false, ...)` to mark an unreachable path.
Prefer `fail(...)` for clearer intent and better failure reporting.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionServerHostname.java:
##########
@@ -207,7 +203,7 @@ public void testRegionServerHostnameReportedToMaster()
throws Exception {
private boolean ignoreNetworkInterface(NetworkInterface networkInterface)
throws Exception {
return networkInterface == null || networkInterface.isLoopback() ||
networkInterface.isVirtual()
- || !networkInterface.isUp();
+ || networkInterface.isPointToPoint() || !networkInterface.isUp();
Review Comment:
This change adds `networkInterface.isPointToPoint()` to the ignore list,
which is a behavioral change unrelated to the JUnit4→JUnit5 migration. If this
is intentional (e.g., to avoid selecting PPP/tunnel interfaces in some CI
environments), please add a short comment explaining why; otherwise consider
reverting to keep this PR purely mechanical.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestOpenRegionFailedMemoryLeak.java:
##########
@@ -108,12 +102,12 @@ public void testOpenRegionFailedMemoryLeak() throws
Exception {
field.setAccessible(true);
BlockingQueue<Runnable> workQueue = (BlockingQueue<Runnable>)
field.get(executor);
// there are still two task not cancel, can not cause to memory lack
- Assert.assertTrue("ScheduledExecutor#workQueue should equals 2, now is
" + workQueue.size()
- + ", please check region is close", 2 == workQueue.size());
+ assertTrue(2 == workQueue.size(), "ScheduledExecutor#workQueue should
equals 2, now is "
+ + workQueue.size() + " please check region is close");
Review Comment:
Prefer `assertEquals(2, workQueue.size(), ...)` over `assertTrue(2 ==
workQueue.size(), ...)` for clearer diagnostics (expected vs actual) when the
assertion fails.
--
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]
