You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "tomtongue (via GitHub)" <gi...@apache.org> on 2024/03/22 17:08:34 UTC
[PR] [WIP] Migrate other files in Core to JUnit5 [iceberg]
tomtongue opened a new pull request, #10027:
URL: https://github.com/apache/iceberg/pull/10027
Migrate the following test classes in iceberg-core to JUnit 5 and AssertJ style for https://github.com/apache/iceberg/issues/9085.
## Current Progress
- [x] `TestMetricsModes.java`
- [x] `TestMicroBatchBuilder.java`
- [x] `TestRemoveSnapshots.java`
- [x] `TestRewriteManifests.java`
- [ ] `TestRowDelta.java` (WIP)
- [x] `TestSequenceNumberForV2Table.java`
- [x] `TestSingleValueParser.java`
- [x] `TestSortOrder.java`
- [x] `TestSortOrderParser.java`
- [x] `TestSplitPlanning.java`
- [x] `TestV1ToV2RowDeltaDelete.java`
- [ ] `TestWapWorkflow.java`
- [x] `V2TableTestBase.java`
Refactoring (related to https://github.com/apache/iceberg/pull/10014#discussion_r1534263933):
- [x] `TestSchemaAndMappingUpdate.java`
- [x] `TestSchemaUpdate`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#discussion_r1537319084
##########
core/src/test/java/org/apache/iceberg/TestSplitPlanning.java:
##########
@@ -33,40 +38,34 @@
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.Types;
-import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public class TestSplitPlanning extends TableTestBase {
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestSplitPlanning extends TestBase {
private static final Configuration CONF = new Configuration();
private static final HadoopTables TABLES = new HadoopTables(CONF);
private static final Schema SCHEMA =
new Schema(
optional(1, "id", Types.IntegerType.get()), optional(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
- private Table table = null;
+ // @Rule public TemporaryFolder temp = new TemporaryFolder();
Review Comment:
Thanks. Remove this comment.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#discussion_r1537425697
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1102,19 +1050,17 @@ public void testExpireWithDefaultSnapshotAge() {
// rely solely on default configs
removeSnapshots(table).deleteWith(deletedFiles::add).commit();
- Assert.assertEquals(
- "Should not change current snapshot", thirdSnapshot, table.currentSnapshot());
- Assert.assertEquals("Should keep 1 snapshot", 1, Iterables.size(table.snapshots()));
- Assert.assertEquals(
- "Should remove expired manifest lists",
- Sets.newHashSet(
- firstSnapshot.manifestListLocation(), secondSnapshot.manifestListLocation()),
- deletedFiles);
+ assertThat(table.currentSnapshot()).isEqualTo(thirdSnapshot);
+ assertThat(table.snapshots()).hasSize(1);
+ assertThat(deletedFiles)
+ .isEqualTo(
Review Comment:
Okay, thank you!
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#discussion_r1537319084
##########
core/src/test/java/org/apache/iceberg/TestSplitPlanning.java:
##########
@@ -33,40 +38,34 @@
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.Types;
-import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public class TestSplitPlanning extends TableTestBase {
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestSplitPlanning extends TestBase {
private static final Configuration CONF = new Configuration();
private static final HadoopTables TABLES = new HadoopTables(CONF);
private static final Schema SCHEMA =
new Schema(
optional(1, "id", Types.IntegerType.get()), optional(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
- private Table table = null;
+ // @Rule public TemporaryFolder temp = new TemporaryFolder();
Review Comment:
Oops, thanks. Remove this comment.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#discussion_r1537193892
##########
core/src/test/java/org/apache/iceberg/TestSplitPlanning.java:
##########
@@ -33,40 +38,34 @@
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.Types;
-import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public class TestSplitPlanning extends TableTestBase {
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestSplitPlanning extends TestBase {
private static final Configuration CONF = new Configuration();
private static final HadoopTables TABLES = new HadoopTables(CONF);
private static final Schema SCHEMA =
new Schema(
optional(1, "id", Types.IntegerType.get()), optional(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
- private Table table = null;
+ // @Rule public TemporaryFolder temp = new TemporaryFolder();
Review Comment:
can be removed
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#discussion_r1537367647
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -202,39 +182,32 @@ public void testExpireOlderThanWithDeleteInMergedManifests() {
removeSnapshots(table).expireOlderThan(tAfterCommits).deleteWith(deletedFiles::add).commit();
- Assert.assertEquals(
- "Expire should not change current snapshot",
- snapshotId,
- table.currentSnapshot().snapshotId());
- Assert.assertNull(
- "Expire should remove the oldest snapshot", table.snapshot(firstSnapshot.snapshotId()));
- Assert.assertNull(
- "Expire should remove the second oldest snapshot",
- table.snapshot(secondSnapshot.snapshotId()));
-
- Assert.assertEquals(
- "Should remove expired manifest lists and deleted data file",
- Sets.newHashSet(
- firstSnapshot.manifestListLocation(), // snapshot expired
- firstSnapshot
- .allManifests(table.io())
- .get(0)
- .path(), // manifest was rewritten for delete
- secondSnapshot.manifestListLocation(), // snapshot expired
- FILE_A.path()), // deleted
- deletedFiles);
+ assertThat(table.currentSnapshot().snapshotId()).isEqualTo(snapshotId);
+ assertThat(table.snapshot(firstSnapshot.snapshotId())).isNull();
+ assertThat(table.snapshot(secondSnapshot.snapshotId())).isNull();
+
+ assertThat(deletedFiles)
+ .as("Should remove expired manifest lists and deleted data file")
+ .isEqualTo(
+ Sets.newHashSet(
+ firstSnapshot.manifestListLocation(), // snapshot expired
+ firstSnapshot
+ .allManifests(table.io())
+ .get(0)
+ .path(), // manifest was rewritten for delete
+ secondSnapshot.manifestListLocation(), // snapshot expired
+ FILE_A.path()));
Review Comment:
original comment is missing here: `// deleted`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#issuecomment-2017729873
> > @nastra Could you review this PR?
> > Just let you know, the following two classes in `org/apache/iceberg` will be migrated:
> >
> > * `TestWapWorkflow`
> > * `TestMetrics` (this class is related files in `iceberg-data`)
>
> You mean migrated in a separate PR? Also I see a few more files in `core` that need to be migrated:
I will submit a PR for the two classes `TestWapWorkflow` and `TestMetrics`. These two classes are remaining classes in `org/apache/iceberg`.
There are also some files to be migrated in some sub-packages (like `actions`, `avro` etc.) in `org/apache/iceberg`. I will check which file in the sub-packages should be migrated to JUnit5.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#discussion_r1537416747
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1102,19 +1050,17 @@ public void testExpireWithDefaultSnapshotAge() {
// rely solely on default configs
removeSnapshots(table).deleteWith(deletedFiles::add).commit();
- Assert.assertEquals(
- "Should not change current snapshot", thirdSnapshot, table.currentSnapshot());
- Assert.assertEquals("Should keep 1 snapshot", 1, Iterables.size(table.snapshots()));
- Assert.assertEquals(
- "Should remove expired manifest lists",
- Sets.newHashSet(
- firstSnapshot.manifestListLocation(), secondSnapshot.manifestListLocation()),
- deletedFiles);
+ assertThat(table.currentSnapshot()).isEqualTo(thirdSnapshot);
+ assertThat(table.snapshots()).hasSize(1);
+ assertThat(deletedFiles)
+ .isEqualTo(
Review Comment:
The order of `firstSnapshot.manifestLocation` and `secondSnapshot.manifestLocation` depends on the hashset values, so the order is not always the same. So, this part needs to use `isEqualTo`, or `containsExactlyInAnyOrder`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #10027:
URL: https://github.com/apache/iceberg/pull/10027
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#issuecomment-2017686753
> @nastra Could you review this PR?
>
> Just let you know, the following two classes in `org/apache/iceberg` will be migrated:
>
> * `TestWapWorkflow`
> * `TestMetrics` (this class is related files in `iceberg-data`)
You mean migrated in a separate PR? Also I see a few more files in `core` that need to be migrated:
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#discussion_r1537366557
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -136,43 +123,37 @@ public void testExpireOlderThanWithDelete() {
removeSnapshots(table).expireOlderThan(tAfterCommits).deleteWith(deletedFiles::add).commit();
- Assert.assertEquals(
- "Expire should not change current snapshot",
- snapshotId,
- table.currentSnapshot().snapshotId());
- Assert.assertNull(
- "Expire should remove the oldest snapshot", table.snapshot(firstSnapshot.snapshotId()));
- Assert.assertNull(
- "Expire should remove the second oldest snapshot",
- table.snapshot(secondSnapshot.snapshotId()));
-
- Assert.assertEquals(
- "Should remove expired manifest lists and deleted data file",
- Sets.newHashSet(
- firstSnapshot.manifestListLocation(), // snapshot expired
- firstSnapshot
- .allManifests(table.io())
- .get(0)
- .path(), // manifest was rewritten for delete
- secondSnapshot.manifestListLocation(), // snapshot expired
- secondSnapshot
- .allManifests(table.io())
- .get(0)
- .path(), // manifest contained only deletes, was dropped
- FILE_A.path()), // deleted
- deletedFiles);
+ assertThat(table.currentSnapshot().snapshotId()).isEqualTo(snapshotId);
+ assertThat(table.snapshot(firstSnapshot.snapshotId())).isNull();
+ assertThat(table.snapshot(secondSnapshot.snapshotId())).isNull();
+
+ assertThat(deletedFiles)
+ .as("Should remove expired manifest lists and deleted data file")
+ .isEqualTo(
+ Sets.newHashSet(
+ firstSnapshot.manifestListLocation(), // snapshot expired
+ firstSnapshot
+ .allManifests(table.io())
+ .get(0)
+ .path(), // manifest was rewritten for delete
+ secondSnapshot.manifestListLocation(), // snapshot expired
+ secondSnapshot
+ .allManifests(table.io())
+ .get(0)
+ .path(), // manifest contained only deletes, was dropped
+ FILE_A.path().toString() // deleted
Review Comment:
why is `toString()` needed here?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#discussion_r1537370506
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1102,19 +1050,17 @@ public void testExpireWithDefaultSnapshotAge() {
// rely solely on default configs
removeSnapshots(table).deleteWith(deletedFiles::add).commit();
- Assert.assertEquals(
- "Should not change current snapshot", thirdSnapshot, table.currentSnapshot());
- Assert.assertEquals("Should keep 1 snapshot", 1, Iterables.size(table.snapshots()));
- Assert.assertEquals(
- "Should remove expired manifest lists",
- Sets.newHashSet(
- firstSnapshot.manifestListLocation(), secondSnapshot.manifestListLocation()),
- deletedFiles);
+ assertThat(table.currentSnapshot()).isEqualTo(thirdSnapshot);
+ assertThat(table.snapshots()).hasSize(1);
+ assertThat(deletedFiles)
+ .isEqualTo(
Review Comment:
`.containsExactly()`
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#discussion_r1537424515
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1102,19 +1050,17 @@ public void testExpireWithDefaultSnapshotAge() {
// rely solely on default configs
removeSnapshots(table).deleteWith(deletedFiles::add).commit();
- Assert.assertEquals(
- "Should not change current snapshot", thirdSnapshot, table.currentSnapshot());
- Assert.assertEquals("Should keep 1 snapshot", 1, Iterables.size(table.snapshots()));
- Assert.assertEquals(
- "Should remove expired manifest lists",
- Sets.newHashSet(
- firstSnapshot.manifestListLocation(), secondSnapshot.manifestListLocation()),
- deletedFiles);
+ assertThat(table.currentSnapshot()).isEqualTo(thirdSnapshot);
+ assertThat(table.snapshots()).hasSize(1);
+ assertThat(deletedFiles)
+ .isEqualTo(
Review Comment:
let's use `containsExactlyInAnyOrder` then
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#issuecomment-2017151496
@nastra Could you review this PR?
The following two classes in `org/apache/iceberg` need to be migrated for now:
* `TestWapWorkflow`
* `TestMetrics` (this class is related files in `iceberg-data`)
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#discussion_r1537402730
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -136,43 +123,37 @@ public void testExpireOlderThanWithDelete() {
removeSnapshots(table).expireOlderThan(tAfterCommits).deleteWith(deletedFiles::add).commit();
- Assert.assertEquals(
- "Expire should not change current snapshot",
- snapshotId,
- table.currentSnapshot().snapshotId());
- Assert.assertNull(
- "Expire should remove the oldest snapshot", table.snapshot(firstSnapshot.snapshotId()));
- Assert.assertNull(
- "Expire should remove the second oldest snapshot",
- table.snapshot(secondSnapshot.snapshotId()));
-
- Assert.assertEquals(
- "Should remove expired manifest lists and deleted data file",
- Sets.newHashSet(
- firstSnapshot.manifestListLocation(), // snapshot expired
- firstSnapshot
- .allManifests(table.io())
- .get(0)
- .path(), // manifest was rewritten for delete
- secondSnapshot.manifestListLocation(), // snapshot expired
- secondSnapshot
- .allManifests(table.io())
- .get(0)
- .path(), // manifest contained only deletes, was dropped
- FILE_A.path()), // deleted
- deletedFiles);
+ assertThat(table.currentSnapshot().snapshotId()).isEqualTo(snapshotId);
+ assertThat(table.snapshot(firstSnapshot.snapshotId())).isNull();
+ assertThat(table.snapshot(secondSnapshot.snapshotId())).isNull();
+
+ assertThat(deletedFiles)
+ .as("Should remove expired manifest lists and deleted data file")
+ .isEqualTo(
+ Sets.newHashSet(
+ firstSnapshot.manifestListLocation(), // snapshot expired
+ firstSnapshot
+ .allManifests(table.io())
+ .get(0)
+ .path(), // manifest was rewritten for delete
+ secondSnapshot.manifestListLocation(), // snapshot expired
+ secondSnapshot
+ .allManifests(table.io())
+ .get(0)
+ .path(), // manifest contained only deletes, was dropped
+ FILE_A.path().toString() // deleted
Review Comment:
This is not needed. Remove `toString()` here.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#discussion_r1537416747
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -1102,19 +1050,17 @@ public void testExpireWithDefaultSnapshotAge() {
// rely solely on default configs
removeSnapshots(table).deleteWith(deletedFiles::add).commit();
- Assert.assertEquals(
- "Should not change current snapshot", thirdSnapshot, table.currentSnapshot());
- Assert.assertEquals("Should keep 1 snapshot", 1, Iterables.size(table.snapshots()));
- Assert.assertEquals(
- "Should remove expired manifest lists",
- Sets.newHashSet(
- firstSnapshot.manifestListLocation(), secondSnapshot.manifestListLocation()),
- deletedFiles);
+ assertThat(table.currentSnapshot()).isEqualTo(thirdSnapshot);
+ assertThat(table.snapshots()).hasSize(1);
+ assertThat(deletedFiles)
+ .isEqualTo(
Review Comment:
The order of `firstSnapshot.manifestLocation` and `secondSnapshot.manifestLocation` depends on the hashset values, so the order is not always the same. So, I believe this part needs to use `isEqualTo`, or `containsExactlyInAnyOrder`.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "tomtongue (via GitHub)" <gi...@apache.org>.
tomtongue commented on code in PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#discussion_r1537319084
##########
core/src/test/java/org/apache/iceberg/TestSplitPlanning.java:
##########
@@ -33,40 +38,34 @@
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.Types;
-import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public class TestSplitPlanning extends TableTestBase {
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestSplitPlanning extends TestBase {
private static final Configuration CONF = new Configuration();
private static final HadoopTables TABLES = new HadoopTables(CONF);
private static final Schema SCHEMA =
new Schema(
optional(1, "id", Types.IntegerType.get()), optional(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
- private Table table = null;
+ // @Rule public TemporaryFolder temp = new TemporaryFolder();
Review Comment:
Opps, thanks. Remove this comment-out.
##########
core/src/test/java/org/apache/iceberg/TestSplitPlanning.java:
##########
@@ -33,40 +38,34 @@
import org.apache.iceberg.relocated.com.google.common.collect.Iterables;
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.types.Types;
-import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.TemporaryFolder;
-import org.junit.runner.RunWith;
-import org.junit.runners.Parameterized;
-
-@RunWith(Parameterized.class)
-public class TestSplitPlanning extends TableTestBase {
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.TestTemplate;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.junit.jupiter.api.io.TempDir;
+
+@ExtendWith(ParameterizedTestExtension.class)
+public class TestSplitPlanning extends TestBase {
private static final Configuration CONF = new Configuration();
private static final HadoopTables TABLES = new HadoopTables(CONF);
private static final Schema SCHEMA =
new Schema(
optional(1, "id", Types.IntegerType.get()), optional(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
- private Table table = null;
+ // @Rule public TemporaryFolder temp = new TemporaryFolder();
Review Comment:
Oops, thanks. Remove this comment-out.
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Migrate other files in Core to JUnit5 [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #10027:
URL: https://github.com/apache/iceberg/pull/10027#discussion_r1537368676
##########
core/src/test/java/org/apache/iceberg/TestRemoveSnapshots.java:
##########
@@ -257,32 +229,30 @@ public void testExpireOlderThanWithRollback() {
removeSnapshots(table).expireOlderThan(tAfterCommits).deleteWith(deletedFiles::add).commit();
- Assert.assertEquals(
- "Expire should not change current snapshot",
- snapshotId,
- table.currentSnapshot().snapshotId());
- Assert.assertNotNull(
- "Expire should keep the oldest snapshot, current",
- table.snapshot(firstSnapshot.snapshotId()));
- Assert.assertNull(
- "Expire should remove the orphaned snapshot", table.snapshot(secondSnapshot.snapshotId()));
-
- Assert.assertEquals(
- "Should remove expired manifest lists and reverted appended data file",
- Sets.newHashSet(
- secondSnapshot.manifestListLocation(), // snapshot expired
- Iterables.getOnlyElement(secondSnapshotManifests)
- .path()), // manifest is no longer referenced
- deletedFiles);
+ assertThat(table.currentSnapshot().snapshotId()).isEqualTo(snapshotId);
+ assertThat(table.snapshot(firstSnapshot.snapshotId()))
+ .as("Expire should keep the oldest snapshot, current")
+ .isNotNull();
+ assertThat(table.snapshot(secondSnapshot.snapshotId()))
+ .as("Expire should remove the orphaned snapshot")
+ .isNull();
+
+ assertThat(deletedFiles)
+ .as("Should remove expired manifest lists and reverted appended data file")
+ .isEqualTo(
+ Sets.newHashSet(
+ secondSnapshot.manifestListLocation(), // snapshot expired
+ Iterables.getOnlyElement(secondSnapshotManifests)
Review Comment:
can we avoid using `Iterables` here?
--
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-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org