Migrate Snapshot files in Core to JUnit5 by tomtongue · Pull Request #9892 · apache/iceberg

tomtongue · 2024-03-07T11:36:50Z

Migrate the following "Read" in iceberg-core to JUnit 5 for #9085.

Current Progress

Snapshots:

The following classes are for Iceberg procedures:

TestSnapshotManager
TestSnapshotRefParser

tomtongue · 2024-03-08T10:50:20Z

@nastra Update the snapshot files. Could you review this PR?

nastra · 2024-03-08T11:45:16Z

core/src/test/java/org/apache/iceberg/LocalTableOperations.java

        name -> {
          try {
-            return temp.newFile(name).getAbsolutePath();
+            return java.nio.file.Files.createTempDirectory(temp, "junit")


shouldn't this create a temp file with the given name rather than a temp directory?

thanks. yes, that should create a temp file. Will fix.

nastra · 2024-03-08T11:45:34Z

core/src/test/java/org/apache/iceberg/TestSnapshotJson.java


 public class TestSnapshotJson {
-  @Rule public TemporaryFolder temp = new TemporaryFolder();
+  @TempDir private static Path temp;


Suggested change

@TempDir private static Path temp;

@TempDir private Path temp;

nastra · 2024-03-08T11:46:12Z

core/src/test/java/org/apache/iceberg/TestSnapshotJson.java


-  public TableOperations ops = new LocalTableOperations(temp);
+  public TableOperations ops =
+      new LocalTableOperations(java.nio.file.Files.createTempDirectory(temp, "junit"));


why does this create a new temp directory? I think it should just pass temp

nastra · 2024-03-08T11:46:27Z

core/src/test/java/org/apache/iceberg/TestSnapshotJson.java

+  public TableOperations ops =
+      new LocalTableOperations(java.nio.file.Files.createTempDirectory(temp, "junit"));
+
+  public TestSnapshotJson() throws IOException {}


why is the constructor here?

It doesn't need. As the above comment, I previously set createTempDirectory for LocalTableOperations, then the createTempDirectory needs the IOException handling. But as your feedback, temp should be set for LocalTableOperations and this IOException is not needed. Remove it in the next commit.

nastra · 2024-03-08T11:48:21Z

core/src/test/java/org/apache/iceberg/TestSnapshotJson.java

  private String createManifestListWithManifestFiles(long snapshotId, Long parentSnapshotId)
      throws IOException {
-    File manifestList = temp.newFile("manifests" + UUID.randomUUID());
+    File manifestList = java.nio.file.Files.createTempDirectory(temp, "junit").toFile();


this should create a new temp file instead of a temp directory

nastra · 2024-03-08T11:49:24Z

core/src/test/java/org/apache/iceberg/TestSnapshotJson.java

+    assertThat(snapshot.snapshotId()).isEqualTo(expected.snapshotId());
+    assertThat(snapshot.allManifests(ops.io())).hasSize(2);
+
+    for (int i = 0; i < snapshot.allManifests(ops.io()).size(); i++) {


are these new checks? can you elaborate why those are being added?

@nastra I tried to check each element in the manifest list, but it's a bit redundant. I believe it's enough to check with isEqualTo between two lists. Let me revert this part and other part like this.

nastra · 2024-03-08T11:51:45Z

core/src/test/java/org/apache/iceberg/TestTableMetadata.java

          .build();

-  @Rule public TemporaryFolder temp = new TemporaryFolder();
+  @TempDir protected static Path temp;


Suggested change

@TempDir protected static Path temp;

@TempDir private Path temp;

nastra · 2024-03-08T11:52:07Z

core/src/test/java/org/apache/iceberg/TestTableMetadata.java

  private String createManifestListWithManifestFile(
      long snapshotId, Long parentSnapshotId, String manifestFile) throws IOException {
-    File manifestList = temp.newFile("manifests" + UUID.randomUUID());
+    File manifestList = java.nio.file.Files.createTempDirectory(temp, "junit").toFile();


same as above, shouldn't this create a temp file with the given name rather than a temp folder?

yes it should be a temp file. Will fix this part.

nastra · 2024-03-08T11:53:43Z

core/src/test/java/org/apache/iceberg/TestSnapshotManager.java

-        expectedBranch != null
-            && expectedBranch.equals(SnapshotRef.branchBuilder(snapshotId).build()));
+    assertThat(expectedBranch).isNotNull();
+    assertThat(expectedBranch).isEqualTo(SnapshotRef.branchBuilder(snapshotId).build());


Suggested change

assertThat(expectedBranch).isEqualTo(SnapshotRef.branchBuilder(snapshotId).build());

assertThat(expectedBranch).isNotNull().isEqualTo(SnapshotRef.branchBuilder(snapshotId).build());

nastra · 2024-03-08T11:53:56Z

core/src/test/java/org/apache/iceberg/TestSnapshotManager.java

-    Assert.assertTrue(
-        expectedBranch != null
-            && expectedBranch.equals(SnapshotRef.branchBuilder(snapshotId).build()));
+    assertThat(expectedBranch).isNotNull();


Suggested change

assertThat(expectedBranch).isNotNull();

nastra · 2024-03-08T11:54:14Z

core/src/test/java/org/apache/iceberg/TestSnapshotManager.java

-    Assertions.assertThat(actualBranch).isNotNull();
-    Assertions.assertThat(actualBranch).isEqualTo(SnapshotRef.branchBuilder(snapshotId).build());
+    assertThat(actualBranch).isNotNull();
+    assertThat(actualBranch).isEqualTo(SnapshotRef.branchBuilder(snapshotId).build());


both checks can be combined

sure, thank you.

nastra · 2024-03-08T11:54:35Z

core/src/test/java/org/apache/iceberg/TestSnapshotManager.java

-    Assert.assertTrue(
-        expectedTag != null && expectedTag.equals(SnapshotRef.tagBuilder(snapshotId).build()));
+    assertThat(expectedTag).isNotNull();
+    assertThat(expectedTag).isEqualTo(SnapshotRef.tagBuilder(snapshotId).build());


same as above

nastra · 2024-03-08T11:56:46Z

core/src/test/java/org/apache/iceberg/TestSnapshotManager.java

-        "Table should be on version 2 after creating manageSnapshots", 2, (int) version());
+    assertThat(readMetadata())
+        .as("Base metadata should not change when manageSnapshots is created")
+        .isEqualTo(base);


this should be isSameAs() as this check actually verifies that both objects are referentially equal. Same for the other places that previously used assertSame()

Okay sure. Thanks for the advice.

tomtongue · 2024-03-08T23:57:30Z

@nastra Thanks for the review. Reflected your comments. Could you review the new one?

nastra · 2024-03-11T08:22:46Z

core/src/test/java/org/apache/iceberg/TestSnapshotJson.java

  private String createManifestListWithManifestFiles(long snapshotId, Long parentSnapshotId)
      throws IOException {
-    File manifestList = temp.newFile("manifests" + UUID.randomUUID());
+    File manifestList = File.createTempFile("junit", null, temp.toFile());


Suggested change

File manifestList = File.createTempFile("junit", null, temp.toFile());

File manifestList = File.createTempFile("manifests", null, temp.toFile());

nastra · 2024-03-11T08:23:01Z

core/src/test/java/org/apache/iceberg/TestTableMetadata.java

  private String createManifestListWithManifestFile(
      long snapshotId, Long parentSnapshotId, String manifestFile) throws IOException {
-    File manifestList = temp.newFile("manifests" + UUID.randomUUID());
+    File manifestList = File.createTempFile("junit", null, temp.toFile());


Suggested change

File manifestList = File.createTempFile("junit", null, temp.toFile());

File manifestList = File.createTempFile("manifests", null, temp.toFile());

nastra · 2024-03-11T08:24:41Z

core/src/test/java/org/apache/iceberg/TestSnapshotManager.java

-        "Table should be on version 2 after creating transaction", 2, (int) version());
+    assertThat(readMetadata())
+        .as("Base metadata should not change when transaction is created")
+        .isEqualTo(base);


should be isSameAs()

nastra · 2024-03-11T08:25:23Z

core/src/test/java/org/apache/iceberg/TestSnapshotManager.java

    txn.commitTransaction();

-    Assert.assertEquals(snapshotAfterFirstAppend, readMetadata().currentSnapshot());
+    assertThat(readMetadata().currentSnapshot()).isSameAs(snapshotAfterFirstAppend);


Suggested change

assertThat(readMetadata().currentSnapshot()).isSameAs(snapshotAfterFirstAppend);

assertThat(readMetadata().currentSnapshot()).isEqualTo(snapshotAfterFirstAppend);

nastra · 2024-03-11T08:25:41Z

core/src/test/java/org/apache/iceberg/TestSnapshotManager.java

-    Assert.assertEquals(snapshotAfterFirstAppend, readMetadata().currentSnapshot());
-    Assert.assertEquals(
-        "Table should be on version 3 after invoking rollbackTo", 3, (int) version());
+    assertThat(readMetadata().currentSnapshot()).isSameAs(snapshotAfterFirstAppend);


Suggested change

assertThat(readMetadata().currentSnapshot()).isSameAs(snapshotAfterFirstAppend);

assertThat(readMetadata().currentSnapshot()).isEqualTo(snapshotAfterFirstAppend);

tomtongue · 2024-03-11T12:42:32Z

Thanks for the review @nastra

Migrate Snapshot files in Core to JUnit5

e820821

github-actions bot added the core label Mar 7, 2024

Add other Snapshot and related classes

e08c52f

tomtongue changed the title ~~[WIP] Migrate Snapshot files in Core to JUnit5~~ Migrate Snapshot files in Core to JUnit5 Mar 8, 2024

Fix npe for temp dir

57e9ca1

nastra reviewed Mar 8, 2024

View reviewed changes

Reflect the comments

eaaf0f8

tomtongue force-pushed the mig-junit5-iceberg-core-snapshot branch from cc0a63e to eaaf0f8 Compare March 8, 2024 16:43

nastra reviewed Mar 11, 2024

View reviewed changes

Update temp file prefix name and metadata snapshot tests

1c68f26

nastra approved these changes Mar 11, 2024

View reviewed changes

nastra merged commit 5ce5c78 into apache:main Mar 11, 2024

tomtongue deleted the mig-junit5-iceberg-core-snapshot branch March 11, 2024 14:57

bk-mz mentioned this pull request Jun 7, 2024

Improve MetricsReporter loading with class loader fallback bk-mz/iceberg#1

Closed

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

Core: Migrate tests to JUnit5 (apache#9892)

2a9a630

	@TempDir private static Path temp;
	@TempDir private Path temp;

	@TempDir protected static Path temp;
	@TempDir private Path temp;

	assertThat(expectedBranch).isEqualTo(SnapshotRef.branchBuilder(snapshotId).build());
	assertThat(expectedBranch).isNotNull().isEqualTo(SnapshotRef.branchBuilder(snapshotId).build());

	File manifestList = File.createTempFile("junit", null, temp.toFile());
	File manifestList = File.createTempFile("manifests", null, temp.toFile());

	assertThat(readMetadata().currentSnapshot()).isSameAs(snapshotAfterFirstAppend);
	assertThat(readMetadata().currentSnapshot()).isEqualTo(snapshotAfterFirstAppend);

Conversation

tomtongue commented Mar 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current Progress

Uh oh!

tomtongue commented Mar 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue Mar 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue commented Mar 8, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomtongue commented Mar 11, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tomtongue commented Mar 7, 2024 •

edited

Loading

tomtongue Mar 8, 2024 •

edited

Loading

tomtongue Mar 8, 2024 •

edited

Loading