Re: [PR] HBASE-30066 Upgrade hbase-server to use junit5 Part9 [hbase]
liuxiaocs7 merged PR #8135: URL: https://github.com/apache/hbase/pull/8135 -- 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]
Re: [PR] HBASE-30066 Upgrade hbase-server to use junit5 Part9 [hbase]
liuxiaocs7 merged PR #8119: URL: https://github.com/apache/hbase/pull/8119 -- 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]
Re: [PR] HBASE-30066 Upgrade hbase-server to use junit5 Part9 [hbase]
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) () -> {
-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) () -> {
+ 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]
Re: [PR] HBASE-30066 Upgrade hbase-server to use junit5 Part9 [hbase]
liuxiaocs7 merged PR #8073: URL: https://github.com/apache/hbase/pull/8073 -- 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]
Re: [PR] HBASE-30066 Upgrade hbase-server to use junit5 Part9 [hbase]
liuxiaocs7 commented on PR #8042: URL: https://github.com/apache/hbase/pull/8042#issuecomment-4231895763 I have checked that the number of unit tests was consistent before and after, and that the unit tests were run in the same way, so let me merge it first! -- 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]
Re: [PR] HBASE-30066 Upgrade hbase-server to use junit5 Part9 [hbase]
liuxiaocs7 merged PR #8042: URL: https://github.com/apache/hbase/pull/8042 -- 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]
Re: [PR] HBASE-30066 Upgrade hbase-server to use junit5 Part9 [hbase]
Copilot commented on code in PR #8042:
URL: https://github.com/apache/hbase/pull/8042#discussion_r3055394648
##
hbase-server/src/test/java/org/apache/hadoop/hbase/fs/TestBlockReorderMultiBlocks.java:
##
@@ -252,11 +246,11 @@ private void testFromDFS(DistributedFileSystem dfs,
String src, int repCount, St
final long max = EnvironmentEdgeManager.currentTime() + 1;
boolean done;
do {
-Assert.assertTrue("Can't get enouth replica",
EnvironmentEdgeManager.currentTime() < max);
+assertTrue(EnvironmentEdgeManager.currentTime() < max, "Can't get
enouth replica");
Review Comment:
Typo in assertion message: "enouth" should be "enough".
##
hbase-server/src/test/java/org/apache/hadoop/hbase/coprocessor/TestRegionObserverInterface.java:
##
@@ -696,8 +695,8 @@ public void testCompactionOverride() throws Exception {
assertNotNull(r);
assertFalse(r.isEmpty());
byte[] iBytes = Bytes.toBytes(i);
-assertArrayEquals("Row should be " + i, r.getRow(), iBytes);
-assertArrayEquals("Value should be " + i, r.getValue(A, A), iBytes);
+assertArrayEquals(r.getRow(), iBytes, "Row should be " + i);
+assertArrayEquals(r.getValue(A, A), iBytes, "Value should be " + i);
Review Comment:
`assertArrayEquals` uses the JUnit 5 signature `assertArrayEquals(expected,
actual, ...)`. The current argument order is reversed, which makes diffs in
assertion failures confusing. Swap arguments so `iBytes` is expected and the
value from `Result` is actual.
##
hbase-server/src/test/java/org/apache/hadoop/hbase/backup/example/TestZooKeeperTableArchiveClient.java:
##
@@ -167,20 +162,20 @@ public void testArchivingEnableDisable() throws Exception
{
// 1. turn on hfile backups
LOG.debug("Starting archiving");
archivingClient.enableHFileBackupAsync(TABLE_NAME);
-assertTrue("Archving didn't get turned on",
archivingClient.getArchivingEnabled(TABLE_NAME));
+assertTrue(archivingClient.getArchivingEnabled(TABLE_NAME), "Archving
didn't get turned on");
// 2. Turn off archiving and make sure its off
archivingClient.disableHFileBackup();
-assertFalse("Archving didn't get turned off.",
archivingClient.getArchivingEnabled(TABLE_NAME));
+assertFalse(archivingClient.getArchivingEnabled(TABLE_NAME), "Archving
didn't get turned off.");
Review Comment:
Typo in assertion messages: "Archving" should be "Archiving".
##
hbase-server/src/test/java/org/apache/hadoop/hbase/snapshot/TestSnapshotWhenChoreCleaning.java:
##
@@ -156,10 +147,10 @@ public void testSnapshotWhenSnapshotHFileCleanerRunning()
throws Exception {
}
TEST_UTIL.getAdmin().snapshot("snapshotName_prev", TABLE_NAME);
-
Assert.assertEquals(Lists.newArrayList(cleaner.getDeletableFiles(files)).size(),
0);
+assertEquals(Lists.newArrayList(cleaner.getDeletableFiles(files)).size(),
0);
TEST_UTIL.getAdmin().deleteSnapshot("snapshotName_prev");
cleaner.getFileCacheForTesting().triggerCacheRefreshForTesting();
-
Assert.assertEquals(Lists.newArrayList(cleaner.getDeletableFiles(files)).size(),
100);
+assertEquals(Lists.newArrayList(cleaner.getDeletableFiles(files)).size(),
100);
Review Comment:
The `assertEquals` calls have arguments reversed for JUnit 5
(`assertEquals(expected, actual, ...)`). Using the current order makes failure
output misleading. Swap the arguments so the expected values are `0` and `100`
and the actual value is the computed size.
##
hbase-server/src/test/java/org/apache/hadoop/hbase/backup/example/TestZooKeeperTableArchiveClient.java:
##
@@ -363,7 +358,7 @@ private List
turnOnArchiving(String tableName, HFileCl
// turn on hfile retention
LOG.debug("Starting archiving for table:" + tableName);
archivingClient.enableHFileBackupAsync(Bytes.toBytes(tableName));
-assertTrue("Archving didn't get turned on",
archivingClient.getArchivingEnabled(tableName));
+assertTrue(archivingClient.getArchivingEnabled(tableName), "Archving
didn't get turned on");
Review Comment:
Typo in assertion message: "Archving" should be "Archiving".
```suggestion
assertTrue(archivingClient.getArchivingEnabled(tableName), "Archiving
didn't get turned on");
```
##
hbase-server/src/test/java/org/apache/hadoop/hbase/backup/example/TestZooKeeperTableArchiveClient.java:
##
@@ -167,20 +162,20 @@ public void testArchivingEnableDisable() throws Exception
{
// 1. turn on hfile backups
LOG.debug("Starting archiving");
archivingClient.enableHFileBackupAsync(TABLE_NAME);
-assertTrue("Archving didn't get turned on",
archivingClient.getArchivingEnabled(TABLE_NAME));
+assertTrue(archivingClient.getArchivingEnabled(TABLE_NAME), "Archving
didn't get turned on");
// 2. Turn off archiving and make sure its off
archivingClient.disableHFileBack
