rdblue commented on code in PR #18403:
URL: https://github.com/apache/iceberg/pull/18403#discussion_r4212876909
##########
core/src/test/java/org/apache/iceberg/TestV4ManifestReader.java:
##########
@@ -18,6 +18,12 @@
*/
package org.apache.iceberg;
+import static org.apache.iceberg.V4TestHelpers.ADDED_TRACKING;
Review Comment:
My main concern is that we don't use these static imported values in
assertions. If these constants are used as defaults for test values, that's
great as long as we validate against the test values. But not if we are making
assertions about individual values because that exposes the constants outside
of `V4TestHelpers`.
For instance, I would not want to see this:
```java
TrackedFile file = dataFile("s3://bucket/path.parquet");
TrackedFile actual = readOne(...);
assertThat(actual.snapshotId()).isEqualTo(42L); // WHERE DID 42 COME FROM??
```
On the other hand, this is fine:
```java
TrackedFile file = dataFile("s3://bucket/path.parquet");
TrackedFile actual = readOne(...);
assertThat(actual).usingComparator(FILE_COMPARATOR).isEqualTo(file);
```
--
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]
