ebyhr commented on code in PR #12853:
URL: https://github.com/apache/iceberg/pull/12853#discussion_r2051808966
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/TaskCheckHelper.java:
##########
@@ -45,61 +47,58 @@ public static void assertEquals(FileScanTask expected,
FileScanTask actual) {
assertEquals(expected.file(), actual.file());
// PartitionSpec implements its own equals method
- Assert.assertEquals("PartitionSpec doesn't match", expected.spec(),
actual.spec());
+ assertThat(actual.spec()).as("PartitionSpec doesn't
match").isEqualTo(expected.spec());
- Assert.assertEquals("starting position doesn't match", expected.start(),
actual.start());
+ assertThat(actual.start()).as("starting position doesn't
match").isEqualTo(expected.start());
- Assert.assertEquals(
- "the number of bytes to scan doesn't match", expected.start(),
actual.start());
+ assertThat(actual.start())
+ .as("the number of bytes to scan doesn't match")
+ .isEqualTo(expected.start());
// simplify comparison on residual expression via comparing toString
- Assert.assertEquals(
- "Residual expression doesn't match",
- expected.residual().toString(),
- actual.residual().toString());
+ assertThat(actual.residual())
+ .asString()
+ .as("Residual expression doesn't match")
+ .isEqualTo(expected.residual().toString());
}
public static void assertEquals(DataFile expected, DataFile actual) {
- Assert.assertEquals(
- "Should match the serialized record path", expected.location(),
actual.location());
- Assert.assertEquals(
- "Should match the serialized record format", expected.format(),
actual.format());
- Assert.assertEquals(
- "Should match the serialized record partition",
- expected.partition().get(0, Object.class),
- actual.partition().get(0, Object.class));
- Assert.assertEquals(
- "Should match the serialized record count", expected.recordCount(),
actual.recordCount());
- Assert.assertEquals(
- "Should match the serialized record size",
- expected.fileSizeInBytes(),
- actual.fileSizeInBytes());
- Assert.assertEquals(
- "Should match the serialized record value counts",
- expected.valueCounts(),
- actual.valueCounts());
- Assert.assertEquals(
- "Should match the serialized record null value counts",
- expected.nullValueCounts(),
- actual.nullValueCounts());
- Assert.assertEquals(
- "Should match the serialized record lower bounds",
- expected.lowerBounds(),
- actual.lowerBounds());
- Assert.assertEquals(
- "Should match the serialized record upper bounds",
- expected.upperBounds(),
- actual.upperBounds());
- Assert.assertEquals(
- "Should match the serialized record key metadata",
- expected.keyMetadata(),
- actual.keyMetadata());
- Assert.assertEquals(
- "Should match the serialized record offsets",
- expected.splitOffsets(),
- actual.splitOffsets());
- Assert.assertEquals(
- "Should match the serialized record offsets", expected.keyMetadata(),
actual.keyMetadata());
+ assertThat(actual.location())
+ .as("Should match the serialized record path")
+ .isEqualTo(expected.location());
+ assertThat(actual.format())
+ .as("Should match the serialized record format")
+ .isEqualTo(expected.format());
+ assertThat(actual.partition().get(0, Object.class))
+ .as("Should match the serialized record partition")
+ .isEqualTo(expected.partition().get(0, Object.class));
+ assertThat(actual.recordCount())
+ .as("Should match the serialized record count")
+ .isEqualTo(expected.recordCount());
+ assertThat(actual.fileSizeInBytes())
+ .as("Should match the serialized record size")
+ .isEqualTo(expected.fileSizeInBytes());
+ assertThat(actual.valueCounts())
+ .as("Should match the serialized record value counts")
+ .isEqualTo(expected.valueCounts());
+ assertThat(actual.nullValueCounts())
+ .as("Should match the serialized record null value counts")
+ .isEqualTo(expected.nullValueCounts());
+ assertThat(actual.lowerBounds())
+ .as("Should match the serialized record lower bounds")
+ .isEqualTo(expected.lowerBounds());
+ assertThat(actual.upperBounds())
+ .as("Should match the serialized record upper bounds")
+ .isEqualTo(expected.upperBounds());
+ assertThat(actual.keyMetadata())
+ .as("Should match the serialized record key metadata")
+ .isEqualTo(expected.keyMetadata());
+ assertThat(actual.splitOffsets())
+ .as("Should match the serialized record offsets")
+ .isEqualTo(expected.splitOffsets());
+ assertThat(actual.keyMetadata())
+ .as("Should match the serialized record offsets")
+ .isEqualTo(expected.keyMetadata());
Review Comment:
The `keyMetadata()` method is already verified at L93-95. It's a
pre-existing mistake, though.
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/actions/TestCreateActions.java:
##########
@@ -521,17 +535,13 @@ public void testProperties() throws Exception {
expectedProps.put("dogs", "sundance");
for (Map.Entry<String, String> entry : expectedProps.entrySet()) {
- Assert.assertTrue(
- "Created table missing property " + entry.getKey(),
- table.properties().containsKey(entry.getKey()));
- Assert.assertEquals(
- "Property value is not the expected value",
- entry.getValue(),
- table.properties().get(entry.getKey()));
+ assertThat(table.properties())
Review Comment:
nit: The outer `for` loop looks redundant. We could use
`containsAllEntriesOf` instead.
##########
spark/v3.4/spark/src/test/java/org/apache/iceberg/spark/actions/TestCreateActions.java:
##########
@@ -547,28 +557,24 @@ public void testSparkTableReservedProperties() throws
Exception {
String[] keys = {"provider", "format", "current-snapshot-id", "location",
"sort-order"};
for (String entry : keys) {
- Assert.assertTrue(
- "Created table missing reserved property " + entry,
- table.properties().containsKey(entry));
+ assertThat(table.properties())
+ .as("Created table missing reserved property " + entry)
+ .containsKey(entry);
Review Comment:
nit: We could use `containsKeys` instead:
```java
assertThat(table.properties())
.as("Created table missing reserved properties")
.containsKeys(keys);
```
Also, `as` method accepts string format style:
```java
.as("Created table missing reserved property %s", entry)
```
--
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: issues-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
