Copilot commented on code in PR #7996:
URL: https://github.com/apache/hbase/pull/7996#discussion_r3026150155
##########
hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestImportTsv.java:
##########
@@ -453,8 +439,8 @@ protected static Tool doMROnTableTest(HBaseTestingUtility
util, TableName table,
&& "true".equalsIgnoreCase(args.get(ImportTsv.DRY_RUN_CONF_KEY));
if (args.containsKey(ImportTsv.BULK_OUTPUT_CONF_KEY)) {
if (isDryRun) {
- assertFalse(String.format("Dry run mode, %s should not have been
created.",
- ImportTsv.BULK_OUTPUT_CONF_KEY), fs.exists(new
Path(ImportTsv.BULK_OUTPUT_CONF_KEY)));
+ assertFalse(fs.exists(new Path(ImportTsv.BULK_OUTPUT_CONF_KEY)),
String.format(
+ "Dry run mode, %s should not have been created.",
ImportTsv.BULK_OUTPUT_CONF_KEY));
} else {
Review Comment:
In the dry-run + bulk-output branch, the existence check uses new
Path(ImportTsv.BULK_OUTPUT_CONF_KEY), which is the configuration key name (e.g.
"importtsv.bulk.output"), not the bulk output path value. This makes the
assertion ineffective (it will not actually verify that the bulk output
directory was not created). Use the configured output path
(args.get(ImportTsv.BULK_OUTPUT_CONF_KEY)) for the Path being checked.
##########
hbase-mapreduce/pom.xml:
##########
@@ -215,11 +215,6 @@
<artifactId>junit-jupiter-params</artifactId>
<scope>test</scope>
</dependency>
- <dependency>
- <groupId>org.junit.vintage</groupId>
- <artifactId>junit-vintage-engine</artifactId>
- <scope>test</scope>
- </dependency>
<dependency>
Review Comment:
The junit-vintage-engine dependency was removed, but there are still
JUnit4-based tests under hbase-mapreduce/src/test/java (e.g.,
TestPerformanceEvaluation, TestCellBasedHFileOutputFormat2,
TestCellBasedImportExport2, TestCellBasedWALPlayer2). Without the vintage
engine these tests will no longer be discovered/executed by Surefire, reducing
test coverage for this module. Either migrate the remaining JUnit4 tests to
JUnit5 in this PR, or keep junit-vintage-engine until the migration is complete.
```suggestion
<dependency>
<groupId>org.junit.vintage</groupId>
<artifactId>junit-vintage-engine</artifactId>
<scope>test</scope>
</dependency>
<dependency>
```
##########
hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat2.java:
##########
@@ -691,10 +684,10 @@ private void doIncrementalLoadTest(boolean
shouldChangeRegions, boolean shouldKe
}
}
}
- assertEquals("Column family not found in FS.", FAMILIES.length, dir);
+ assertEquals(FAMILIES.length, dir, "Column family not found in FS.");
}
if (writeMultipleTables) {
- assertEquals("Dir for all input tables not created", numTableDirs,
allTables.size());
+ assertEquals(numTableDirs, allTables.size(), "Dir for all input tables
not created");
Review Comment:
assertEquals parameters are reversed here: expected should be
allTables.size() (one dir per table) and actual should be numTableDirs. As
written, failure output will be misleading.
```suggestion
assertEquals(allTables.size(), numTableDirs, "Dir for all input tables
not created");
```
##########
hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestHFileOutputFormat2.java:
##########
@@ -650,9 +643,9 @@ private void doIncrementalLoadTest(boolean
shouldChangeRegions, boolean shouldKe
Table table = util.createTable(tableName, FAMILIES, splitKeys);
RegionLocator r = util.getConnection().getRegionLocator(tableName);
- assertEquals("Should start with empty table", 0, util.countRows(table));
+ assertEquals(0, util.countRows(table), "Should start with empty table");
int numRegions = r.getStartKeys().length;
- assertEquals("Should make " + regionNum + " regions", numRegions,
regionNum);
+ assertEquals(numRegions, regionNum, "Should make " + regionNum + "
regions");
Review Comment:
assertEquals parameters are reversed here: the expected number of regions is
regionNum, while numRegions is the actual value read from the locator. Swapping
them will produce correct failure diagnostics if this assertion fails.
```suggestion
assertEquals(regionNum, numRegions, "Should make " + regionNum + "
regions");
```
--
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]
