nastra commented on code in PR #9241:
URL: https://github.com/apache/iceberg/pull/9241#discussion_r1420174672
##########
mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java:
##########
@@ -176,8 +178,10 @@ public void testCreateDropTableToCatalog() throws
IOException {
HadoopCatalog catalog = new CustomHadoopCatalog(conf, warehouseLocation);
Table table = catalog.loadTable(identifier);
- Assert.assertEquals(SchemaParser.toJson(SCHEMA),
SchemaParser.toJson(table.schema()));
- Assert.assertEquals(PartitionSpecParser.toJson(SPEC),
PartitionSpecParser.toJson(table.spec()));
+ Assertions.assertThat(SchemaParser.toJson(table.schema()))
+ .isEqualTo(SchemaParser.toJson(SCHEMA));
+ Assertions.assertThat(PartitionSpecParser.toJson(table.spec()))
Review Comment:
please use the statically imported `assertThat()` method here and everywhere
else
##########
mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java:
##########
@@ -54,9 +53,9 @@ public class TestCatalogs {
private Configuration conf;
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir public Path temp;
Review Comment:
```suggestion
@TempDir private Path temp;
```
##########
mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java:
##########
@@ -198,11 +202,11 @@ public void testCreateDropTableToCatalog() throws
IOException {
public void testLoadCatalogDefault() {
String catalogName = "barCatalog";
Optional<Catalog> defaultCatalog = Catalogs.loadCatalog(conf, catalogName);
- Assert.assertTrue(defaultCatalog.isPresent());
+ Assertions.assertThat(defaultCatalog.isPresent()).isTrue();
Review Comment:
```suggestion
assertThat(defaultCatalog).isPresent();
```
##########
mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java:
##########
@@ -212,11 +216,11 @@ public void testLoadCatalogHive() {
InputFormatConfig.catalogPropertyConfigKey(catalogName,
CatalogUtil.ICEBERG_CATALOG_TYPE),
CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE);
Optional<Catalog> hiveCatalog = Catalogs.loadCatalog(conf, catalogName);
- Assert.assertTrue(hiveCatalog.isPresent());
+ Assertions.assertThat(hiveCatalog.isPresent()).isTrue();
Review Comment:
same as above
##########
mr/src/test/java/org/apache/iceberg/mr/hive/HiveIcebergTestUtils.java:
##########
@@ -218,13 +218,12 @@ public static void assertEquals(Record expected, Record
actual) {
for (int i = 0; i < expected.size(); ++i) {
if (expected.get(i) instanceof OffsetDateTime) {
// For OffsetDateTime we just compare the actual instant
- Assert.assertEquals(
- ((OffsetDateTime) expected.get(i)).toInstant(),
- ((OffsetDateTime) actual.get(i)).toInstant());
+ Assertions.assertThat(((OffsetDateTime) actual.get(i)).toInstant())
+ .isEqualTo(((OffsetDateTime) expected.get(i)).toInstant());
} else if (expected.get(i) instanceof byte[]) {
- Assert.assertArrayEquals((byte[]) expected.get(i), (byte[])
actual.get(i));
+ Assertions.assertThat((byte[]) actual.get(i)).isEqualTo((byte[])
expected.get(i));
Review Comment:
we can remove this line and `else if (expected.get(i) instanceof byte[]) {`
because AssertJ can properly handle byte array comparisons
##########
mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java:
##########
@@ -230,13 +234,13 @@ public void testLoadCatalogHadoop() {
catalogName, CatalogProperties.WAREHOUSE_LOCATION),
"/tmp/mylocation");
Optional<Catalog> hadoopCatalog = Catalogs.loadCatalog(conf, catalogName);
- Assert.assertTrue(hadoopCatalog.isPresent());
+ Assertions.assertThat(hadoopCatalog.isPresent()).isTrue();
Review Comment:
same as above
##########
mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java:
##########
@@ -250,16 +254,18 @@ public void testLoadCatalogCustom() {
catalogName, CatalogProperties.WAREHOUSE_LOCATION),
"/tmp/mylocation");
Optional<Catalog> customHadoopCatalog = Catalogs.loadCatalog(conf,
catalogName);
- Assert.assertTrue(customHadoopCatalog.isPresent());
+ Assertions.assertThat(customHadoopCatalog.isPresent()).isTrue();
Assertions.assertThat(customHadoopCatalog.get()).isInstanceOf(CustomHadoopCatalog.class);
Properties properties = new Properties();
properties.put(InputFormatConfig.CATALOG_NAME, catalogName);
- Assert.assertFalse(Catalogs.hiveCatalog(conf, properties));
+ Assertions.assertThat(Catalogs.hiveCatalog(conf, properties)).isFalse();
}
@Test
public void testLoadCatalogLocation() {
- Assert.assertFalse(Catalogs.loadCatalog(conf,
Catalogs.ICEBERG_HADOOP_TABLE_NAME).isPresent());
+ Assertions.assertThat(
+ Catalogs.loadCatalog(conf,
Catalogs.ICEBERG_HADOOP_TABLE_NAME).isPresent())
Review Comment:
same as above
##########
mr/src/test/java/org/apache/iceberg/mr/hive/HiveIcebergTestUtils.java:
##########
@@ -288,9 +287,10 @@ public static void validateFiles(Table table,
Configuration conf, JobID jobId, i
.filter(path -> !path.getFileName().toString().startsWith("."))
.collect(Collectors.toList());
- Assert.assertEquals(dataFileNum, dataFiles.size());
- Assert.assertFalse(
- new
File(HiveIcebergOutputCommitter.generateJobLocation(table.location(), conf,
jobId))
- .exists());
+ Assertions.assertThat(dataFiles.size()).isEqualTo(dataFileNum);
Review Comment:
```suggestion
assertThat(dataFiles).hasSize(dataFileNum);
```
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergFilterFactory.java:
##########
@@ -82,10 +80,12 @@ public void testNotEqualsOperand() {
UnboundPredicate childExpressionActual = (UnboundPredicate) actual.child();
UnboundPredicate childExpressionExpected = Expressions.equal("salary",
3000L);
- assertEquals(actual.op(), expected.op());
- assertEquals(actual.child().op(), expected.child().op());
- assertEquals(childExpressionActual.ref().name(),
childExpressionExpected.ref().name());
- assertEquals(childExpressionActual.literal(),
childExpressionExpected.literal());
+ Assertions.assertThat(expected.op()).isEqualTo(actual.op());
Review Comment:
it's ok to statically import `assertThat()` here and in the other files
##########
mr/src/test/java/org/apache/iceberg/mr/TestCatalogs.java:
##########
@@ -212,11 +216,11 @@ public void testLoadCatalogHive() {
InputFormatConfig.catalogPropertyConfigKey(catalogName,
CatalogUtil.ICEBERG_CATALOG_TYPE),
CatalogUtil.ICEBERG_CATALOG_TYPE_HIVE);
Optional<Catalog> hiveCatalog = Catalogs.loadCatalog(conf, catalogName);
- Assert.assertTrue(hiveCatalog.isPresent());
+ Assertions.assertThat(hiveCatalog.isPresent()).isTrue();
Review Comment:
please also update all the other places that are similar to this
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergOutputCommitter.java:
##########
@@ -201,22 +203,22 @@ public void writerIsClosedAfterTaskCommitFailure() throws
IOException {
.when(failingCommitter)
.commitTask(argumentCaptor.capture());
- Table table = table(temp.getRoot().getPath(), false);
+ Table table = table(temp.toFile().getPath(), false);
JobConf conf = jobConf(table, 1);
Assertions.assertThatThrownBy(
() -> writeRecords(table.name(), 1, 0, true, false, conf,
failingCommitter))
.isInstanceOf(RuntimeException.class)
.hasMessage(exceptionMessage);
- Assert.assertEquals(1, argumentCaptor.getAllValues().size());
+ Assertions.assertThat(argumentCaptor.getAllValues().size()).isEqualTo(1);
Review Comment:
same as previously mentioned about size assertions: `hasSize(xyz)` is better
here
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestDeserializer.java:
##########
@@ -35,9 +35,9 @@
import org.apache.iceberg.hive.HiveVersion;
import org.apache.iceberg.mr.hive.serde.objectinspector.IcebergObjectInspector;
import org.apache.iceberg.types.Types;
-import org.junit.Assert;
-import org.junit.Assume;
-import org.junit.Test;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Assumptions;
Review Comment:
please use AssertJ assumptions
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergSerDe.java:
##########
@@ -34,22 +35,21 @@
import org.apache.iceberg.mr.hive.serde.objectinspector.IcebergObjectInspector;
import org.apache.iceberg.mr.mapred.Container;
import org.apache.iceberg.types.Types;
-import org.junit.Assert;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
public class TestHiveIcebergSerDe {
private static final Schema schema =
new Schema(required(1, "string_field", Types.StringType.get()));
- @Rule public TemporaryFolder tmp = new TemporaryFolder();
+ @TempDir public Path tmp;
Review Comment:
```suggestion
@TempDir private Path tmp;
```
##########
mr/src/test/java/org/apache/iceberg/mr/hive/HiveIcebergTestUtils.java:
##########
@@ -265,7 +264,7 @@ public static void validateData(List<Record> expected,
List<Record> actual, int
sortedExpected.sort(Comparator.comparingLong(record -> (Long)
record.get(sortBy)));
sortedActual.sort(Comparator.comparingLong(record -> (Long)
record.get(sortBy)));
- Assert.assertEquals(sortedExpected.size(), sortedActual.size());
+
Assertions.assertThat(sortedActual.size()).isEqualTo(sortedExpected.size());
Review Comment:
```suggestion
assertThat(sortedActual).hasSameSizeAs(sortedExpected);
```
##########
mr/src/test/java/org/apache/iceberg/mr/hive/HiveIcebergTestUtils.java:
##########
@@ -63,7 +63,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.ByteBuffers;
-import org.junit.Assert;
+import org.assertj.core.api.Assertions;
Review Comment:
it's ok to statically import `assertThat()` and use it
##########
mr/src/test/java/org/apache/iceberg/mr/hive/HiveIcebergTestUtils.java:
##########
@@ -288,9 +287,10 @@ public static void validateFiles(Table table,
Configuration conf, JobID jobId, i
.filter(path -> !path.getFileName().toString().startsWith("."))
.collect(Collectors.toList());
- Assert.assertEquals(dataFileNum, dataFiles.size());
- Assert.assertFalse(
- new
File(HiveIcebergOutputCommitter.generateJobLocation(table.location(), conf,
jobId))
- .exists());
+ Assertions.assertThat(dataFiles.size()).isEqualTo(dataFileNum);
+ Assertions.assertThat(
Review Comment:
```
assertThat(
new
File(HiveIcebergOutputCommitter.generateJobLocation(table.location(), conf,
jobId)))
.doesNotExist();
```
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergOutputCommitter.java:
##########
@@ -80,7 +79,7 @@ public class TestHiveIcebergOutputCommitter {
private static final PartitionSpec PARTITIONED_SPEC =
PartitionSpec.builderFor(CUSTOMER_SCHEMA).bucket("customer_id",
3).build();
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir public Path temp;
Review Comment:
```suggestion
@TempDir private Path temp;
```
##########
mr/src/test/java/org/apache/iceberg/mr/hive/TestHiveIcebergSerDe.java:
##########
@@ -34,22 +35,21 @@
import org.apache.iceberg.mr.hive.serde.objectinspector.IcebergObjectInspector;
import org.apache.iceberg.mr.mapred.Container;
import org.apache.iceberg.types.Types;
-import org.junit.Assert;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
+import org.assertj.core.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
public class TestHiveIcebergSerDe {
private static final Schema schema =
new Schema(required(1, "string_field", Types.StringType.get()));
- @Rule public TemporaryFolder tmp = new TemporaryFolder();
+ @TempDir public Path tmp;
Review Comment:
please also update this in all the other files being touched by this PR
##########
mr/src/test/java/org/apache/iceberg/mr/hive/HiveIcebergTestUtils.java:
##########
@@ -265,7 +264,7 @@ public static void validateData(List<Record> expected,
List<Record> actual, int
sortedExpected.sort(Comparator.comparingLong(record -> (Long)
record.get(sortBy)));
sortedActual.sort(Comparator.comparingLong(record -> (Long)
record.get(sortBy)));
- Assert.assertEquals(sortedExpected.size(), sortedActual.size());
+
Assertions.assertThat(sortedActual.size()).isEqualTo(sortedExpected.size());
Review Comment:
the reason for this is that the content of both collections will be printed
when the assertion ever fails, which makes debugging much easier. Please also
update all the other affected places
--
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]
