Copilot commented on code in PR #8112:
URL: https://github.com/apache/hbase/pull/8112#discussion_r3124801270
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicas.java:
##########
@@ -213,7 +211,7 @@ private void assertGet(Region region, int value, boolean
expect) throws IOExcept
Get get = new Get(row);
Result result = region.get(get);
if (expect) {
- Assert.assertArrayEquals(row, result.getValue(f, null));
+ assertArrayEquals(row, result.getValue(f, null));
} else {
result.isEmpty();
}
Review Comment:
In the `expect == false` branch, the test calls `result.isEmpty()` but does
not assert the outcome. This means the helper will silently pass even if the
result is not empty. Use an assertion (for example,
assertTrue(result.isEmpty()) or an equivalent check) to validate the expected
empty result.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/TestRegionReplicas.java:
##########
@@ -228,7 +226,7 @@ private void assertGetRpc(HRegionInfo info, int value,
boolean expect)
ClientProtos.GetResponse getResp = getRS().getRSRpcServices().get(null,
getReq);
Result result = ProtobufUtil.toResult(getResp.getResult());
if (expect) {
- Assert.assertArrayEquals(row, result.getValue(f, null));
+ assertArrayEquals(row, result.getValue(f, null));
} else {
result.isEmpty();
}
Review Comment:
In the `expect == false` branch, the test calls `result.isEmpty()` but does
not assert the returned value. This can let incorrect results slip through
undetected. Please add an assertion that verifies the result is empty when
`expect` is false.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestExceptionInAssignRegion.java:
##########
@@ -98,14 +95,14 @@ public void testExceptionInAssignRegion() {
long prodId = procedureExecutor.submitProcedure(assignRegionProcedure);
ProcedureTestingUtility.waitProcedure(procedureExecutor, prodId);
- Assert.assertEquals("Should be two RS since other is aborted", 2,
- UTIL.getMiniHBaseCluster().getLiveRegionServerThreads().size());
- Assert.assertNull("RIT Map doesn't have correct value",
-
getRegionServer(0).getRegionsInTransitionInRS().get(hri.getEncodedNameAsBytes()));
- Assert.assertNull("RIT Map doesn't have correct value",
-
getRegionServer(1).getRegionsInTransitionInRS().get(hri.getEncodedNameAsBytes()));
- Assert.assertNull("RIT Map doesn't have correct value",
-
getRegionServer(2).getRegionsInTransitionInRS().get(hri.getEncodedNameAsBytes()));
+
assertEquals(UTIL.getMiniHBaseCluster().getLiveRegionServerThreads().size(), 2,
Review Comment:
JUnit Jupiter's assertEquals signature is assertEquals(expected, actual,
message). This assertion currently passes the cluster size as the expected
value and `2` as the actual value, which will invert expected/actual in
assertion failures. Swap the first two arguments so the expected value is `2`
and the actual value is the computed size.
```suggestion
assertEquals(2,
UTIL.getMiniHBaseCluster().getLiveRegionServerThreads().size(),
```
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestModifyTableWhileMerging.java:
##########
@@ -98,8 +94,8 @@ public void test() throws Exception {
long proc = executor.submitProcedure(mergeTableRegionsProcedure);
UTIL.waitFor(3000000, () ->
UTIL.getMiniHBaseCluster().getMaster().getMasterProcedureExecutor()
.isFinished(procModify));
- Assert.assertEquals("Modify Table procedure should success!",
- ProcedureProtos.ProcedureState.SUCCESS, modifyTableProcedure.getState());
+ assertEquals(modifyTableProcedure.getState(),
ProcedureProtos.ProcedureState.SUCCESS,
Review Comment:
JUnit Jupiter's assertEquals signature is assertEquals(expected, actual,
message). The current call passes `modifyTableProcedure.getState()` as the
expected value and `SUCCESS` as the actual value, which swaps expected/actual
in failure output. Please flip the first two arguments so expected is
`ProcedureState.SUCCESS` and actual is `modifyTableProcedure.getState()`.
```suggestion
assertEquals(ProcedureProtos.ProcedureState.SUCCESS,
modifyTableProcedure.getState(),
```
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestRogueRSAssignment.java:
##########
@@ -158,10 +153,10 @@ public void testReportRSWithWrongRegion() throws
Exception {
// sending fake request to master
// TODO: replace YouAreDeadException with appropriate exception as and
when necessary
- exception.expect(ServiceException.class);
- exception.expectCause(isA(YouAreDeadException.class));
- RegionServerStatusProtos.RegionServerReportResponse response =
- master.getMasterRpcServices().regionServerReport(null, request.build());
+ ServiceException exception = assertThrows(ServiceException.class,
+ () -> master.getMasterRpcServices().regionServerReport(null,
request.build()));
+ org.hamcrest.MatcherAssert.assertThat((YouAreDeadException)
exception.getCause(),
+ isA(YouAreDeadException.class));
Review Comment:
The assertion casts `exception.getCause()` to `YouAreDeadException` before
verifying its type. If the cause is null or a different type, this will throw
(ClassCastException/NPE) and hide the original failure. Assert on
`exception.getCause()` directly (and/or assertNotNull) without casting, then
check it isA(YouAreDeadException.class).
--
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]
