nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1233601694
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCachingCatalog.java:
##########
@@ -291,9 +292,10 @@ public void
testCacheExpirationEagerlyRemovesMetadataTables() throws IOException
Arrays.stream(metadataTables(tableIdent))
.forEach(
metadataTable ->
- Assert.assertFalse(
- "When a data table expires, its metadata tables should
expire regardless of age",
- catalog.cache().asMap().containsKey(metadataTable)));
+
Assertions.assertThat(catalog.cache().asMap().containsKey(metadataTable))
Review Comment:
```suggestion
Assertions.assertThat(catalog.cache().asMap()).containsKey(metadataTable)
```
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCatalogUtilDropTable.java:
##########
@@ -49,8 +49,12 @@ public void dropTableDataDeletesExpectedFiles() {
Set<String> manifestLocations = manifestLocations(snapshotSet, table.io());
Set<String> dataLocations = dataLocations(snapshotSet, table.io());
Set<String> metadataLocations = metadataLocations(tableMetadata);
- Assert.assertEquals("should have 2 manifest lists", 2,
manifestListLocations.size());
- Assert.assertEquals("should have 3 metadata locations", 3,
metadataLocations.size());
+ Assertions.assertThat(manifestListLocations.size())
Review Comment:
```suggestion
Assertions.assertThat(manifestListLocations).hasSize(2)
```
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCatalogUtilDropTable.java:
##########
@@ -49,8 +49,12 @@ public void dropTableDataDeletesExpectedFiles() {
Set<String> manifestLocations = manifestLocations(snapshotSet, table.io());
Set<String> dataLocations = dataLocations(snapshotSet, table.io());
Set<String> metadataLocations = metadataLocations(tableMetadata);
- Assert.assertEquals("should have 2 manifest lists", 2,
manifestListLocations.size());
- Assert.assertEquals("should have 3 metadata locations", 3,
metadataLocations.size());
+ Assertions.assertThat(manifestListLocations.size())
+ .as("should have 2 manifest lists")
+ .isEqualTo(2);
+ Assertions.assertThat(metadataLocations.size())
Review Comment:
```suggestion
Assertions.assertThat(metadataLocations).hasSize(3)
```
##########
core/src/test/java/org/apache/iceberg/hadoop/TestTableSerialization.java:
##########
@@ -41,7 +41,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.types.Types;
-import org.junit.Assert;
+import org.assertj.core.api.Assertions;
import org.junit.Test;
Review Comment:
needs to be switched to Junit5
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -57,7 +58,6 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.Tasks;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
import org.junit.Test;
Review Comment:
needs to be switched to Junit5
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCatalogUtilDropTable.java:
##########
@@ -124,8 +131,12 @@ public void shouldNotDropDataFilesIfGcNotEnabled() {
Set<String> manifestListLocations = manifestListLocations(snapshotSet);
Set<String> manifestLocations = manifestLocations(snapshotSet, table.io());
Set<String> metadataLocations = metadataLocations(tableMetadata);
- Assert.assertEquals("should have 2 manifest lists", 2,
manifestListLocations.size());
- Assert.assertEquals("should have 4 metadata locations", 4,
metadataLocations.size());
+ Assertions.assertThat(manifestListLocations.size())
+ .as("should have 2 manifest lists")
+ .isEqualTo(2);
+ Assertions.assertThat(metadataLocations.size())
+ .as("should have 4 metadata locations")
+ .isEqualTo(4);
Review Comment:
same as above for both assertions
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -55,20 +54,19 @@ public class TestHadoopTables {
required(1, "id", Types.IntegerType.get(), "unique ID"),
required(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
private File tableDir = null;
Review Comment:
I think we can just add a `@TempDir` annotation to `tableDir` and remove
`@TempDir Path temp` alltogether?
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCachingCatalog.java:
##########
@@ -291,9 +292,10 @@ public void
testCacheExpirationEagerlyRemovesMetadataTables() throws IOException
Arrays.stream(metadataTables(tableIdent))
.forEach(
metadataTable ->
- Assert.assertFalse(
- "When a data table expires, its metadata tables should
expire regardless of age",
- catalog.cache().asMap().containsKey(metadataTable)));
+
Assertions.assertThat(catalog.cache().asMap().containsKey(metadataTable))
Review Comment:
we want to avoid isTrue/isFalse as much as possible and use assertions that
are more descriptive
##########
core/src/test/java/org/apache/iceberg/hadoop/TestStaticTable.java:
##########
@@ -24,7 +24,6 @@
import org.apache.iceberg.Table;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
import org.junit.Test;
Review Comment:
needs to be switched to Junit5
##########
core/src/test/java/org/apache/iceberg/hadoop/TestStaticTable.java:
##########
@@ -88,13 +87,13 @@ public void testHasSameProperties() {
table.newAppend().appendFile(FILE_B).commit();
table.newOverwrite().deleteFile(FILE_B).addFile(FILE_C).commit();
Table staticTable = getStaticTable();
- Assert.assertTrue("Same history?",
table.history().containsAll(staticTable.history()));
- Assert.assertTrue(
- "Same snapshot?",
- table.currentSnapshot().snapshotId() ==
staticTable.currentSnapshot().snapshotId());
- Assert.assertTrue(
- "Same properties?",
- Maps.difference(table.properties(),
staticTable.properties()).areEqual());
+ Assertions.assertThat(table.history()).as("Same
history?").containsAll(staticTable.history());
+ Assertions.assertThat(table.currentSnapshot().snapshotId())
+ .as("Same snapshot?")
+ .isEqualTo(staticTable.currentSnapshot().snapshotId());
+ Assertions.assertThat(Maps.difference(table.properties(),
staticTable.properties()).areEqual())
Review Comment:
can we express that with an AssertJ assertion rather than using isTrue?
##########
core/src/test/java/org/apache/iceberg/rest/RequestResponseTestBase.java:
##########
@@ -76,11 +75,13 @@ public void testHasOnlyKnownFields() {
try {
JsonNode node = mapper().readValue(serialize(createExampleInstance()),
JsonNode.class);
for (String field : fieldsFromSpec) {
- Assert.assertTrue("Should have field: " + field, node.has(field));
+ Assertions.assertThat(node.has(field)).as("Should have field: %s",
field).isTrue();
}
for (String field : ((Iterable<? extends String>) node::fieldNames)) {
- Assert.assertTrue("Should not have field: " + field,
fieldsFromSpec.contains(field));
+ Assertions.assertThat(fieldsFromSpec.contains(field))
Review Comment:
should be `Assertions.assertThat(fieldsFromSpec).contains(field);`
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -51,7 +51,6 @@
import org.apache.iceberg.transforms.Transform;
import org.apache.iceberg.transforms.Transforms;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
import org.junit.Test;
Review Comment:
we should also switch this to JUnit5.
For switching `HadoopTableTestBase` you'd need to switch from `@Rule public
TemporaryFolder temp = new TemporaryFolder();` to `@TempDir File temp`
(similarly to how it's done in other test classes)
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
