Copilot commented on code in PR #8119:
URL: https://github.com/apache/hbase/pull/8119#discussion_r3130132170
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestPostIncrementAndAppendBeforeWAL.java:
##########
@@ -73,15 +70,11 @@
* change the cells which will be applied to memstore and WAL. So add unit
test for the case which
* change the cell's column family and tags.
*/
-@Category({ CoprocessorTests.class, MediumTests.class })
[email protected](CoprocessorTests.TAG)
[email protected](MediumTests.TAG)
Review Comment:
These tags use fully-qualified annotation names while the rest of the
migrated tests use imported `@Tag`. For consistency and readability, import
`org.junit.jupiter.api.Tag` and use `@Tag(...)` here as well.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/backup/TestHFileArchiving.java:
##########
@@ -476,22 +474,22 @@ public void testArchiveRegionsWhenPermissionDenied()
throws Exception {
UserGroupInformation.createUserForTesting("foo1234", new String[] {
"group1" });
try {
- ugi.doAs((PrivilegedExceptionAction<Void>) () -> {
- FileSystem fs = UTIL.getTestFileSystem();
- HFileArchiver.archiveRegions(UTIL.getConfiguration(), fs, rootDir,
tableDir, regionDirList);
- return null;
- });
- } catch (IOException e) {
+ IOException e =
+ assertThrows(IOException.class, () ->
ugi.doAs((PrivilegedExceptionAction<Void>) () -> {
+ FileSystem fs = UTIL.getTestFileSystem();
+ HFileArchiver.archiveRegions(UTIL.getConfiguration(), fs, rootDir,
tableDir,
+ regionDirList);
+ return null;
+ }));
Review Comment:
This assertion assumes the thrown `IOException` always has a non-null cause
with a non-null message. To avoid brittle failures (NPE masking the real
exception), assert against the exception message itself (and/or use JUnit
assertions that include the original exception), or assert the cause/message
presence before accessing it.
```suggestion
}));
assertNotNull(e.getCause(), "Expected IOException to have a cause");
assertNotNull(e.getCause().getMessage(), "Expected IOException cause
to have a message");
```
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestCoprocessorHost.java:
##########
@@ -54,7 +49,7 @@ private static class TestAbortable implements Abortable {
@Override
public void abort(String why, Throwable e) {
this.aborted = true;
- Assert.fail(e.getMessage());
+ fail(e.getMessage());
Review Comment:
This drops the throwable context and can throw a secondary NPE if `e` is
null. Prefer failing with both `why` and the throwable (e.g., using JUnit's
overload that accepts a `Throwable`) so the original stack trace is preserved
and the failure message remains informative.
```suggestion
fail(why, e);
```
--
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]
