You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "skytin1004 (via GitHub)" <gi...@apache.org> on 2023/06/18 08:16:49 UTC
[GitHub] [iceberg] skytin1004 opened a new pull request, #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
skytin1004 opened a new pull request, #7861:
URL: https://github.com/apache/iceberg/pull/7861
This pull request is related to the issue "Move JUnit4 tests to JUnit5 https://github.com/apache/iceberg/issues/7160".
I switched the files in [rest, hadoop] pakages to JUnit5.
All files in Core's rest package have completed the switch to JUnit5.
I have switched some Hadoop packages to JUnit5 and switched all Hadoop packages from `Assert` to `AssertJ`.
In Core's Hadoop package, `"HadoopTableTestBase"`, when switching to JUnit5, an error occurred in `"testListNamespace"` of `"TestHadoopCatalog"` where the namespace of the root directory of the` temp` folder could not be looked up during testing.
Therefore, I switched only the files that are not related to `"HadoopTableTestBase"` in the Hadoop package to JUnit5, kept the files related to `"HadoopTableTestBase"` in JUnit4, and switched all the parts that are written in `Assert` to `AssertJ`.
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238258039
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -57,7 +58,6 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.Tasks;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
import org.junit.Test;
Review Comment:
I switched to JUnit5.
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -57,7 +58,6 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.Tasks;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
import org.junit.Test;
Review Comment:
I have switched 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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238263064
##########
core/src/test/java/org/apache/iceberg/hadoop/TestStaticTable.java:
##########
@@ -24,7 +24,6 @@
import org.apache.iceberg.Table;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
import org.junit.Test;
Review Comment:
I have switched 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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238679149
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -67,55 +64,59 @@ public class TestHadoopCommits extends HadoopTableTestBase {
public void testCreateTable() throws Exception {
PartitionSpec expectedSpec = PartitionSpec.builderFor(TABLE_SCHEMA).bucket("data", 16).build();
- Assert.assertEquals(
- "Table schema should match schema with reassigned ids",
- TABLE_SCHEMA.asStruct(),
- table.schema().asStruct());
- Assert.assertEquals(
- "Table partition spec should match with reassigned ids", expectedSpec, table.spec());
+ Assertions.assertThat(table.schema().asStruct())
+ .as("Table schema should match schema with reassigned ids")
+ .isEqualTo(TABLE_SCHEMA.asStruct());
+ Assertions.assertThat(table.spec())
+ .as("Table partition spec should match with reassigned ids")
+ .isEqualTo(expectedSpec);
List<FileScanTask> tasks = Lists.newArrayList(table.newScan().planFiles());
- Assert.assertEquals("Should not create any scan tasks", 0, tasks.size());
-
- Assert.assertTrue("Table location should exist", tableDir.exists());
- Assert.assertTrue(
- "Should create metadata folder", metadataDir.exists() && metadataDir.isDirectory());
- Assert.assertTrue("Should create v1 metadata", version(1).exists() && version(1).isFile());
- Assert.assertFalse("Should not create v2 or newer versions", version(2).exists());
- Assert.assertTrue("Should create version hint file", versionHintFile.exists());
- Assert.assertEquals("Should write the current version to the hint file", 1, readVersionHint());
-
+ Assertions.assertThat(tasks).as("Should not create any scan tasks").isEmpty();
+ Assertions.assertThat(tableDir).as("Table location should exist").exists();
+ Assertions.assertThat(metadataDir.exists() && metadataDir.isDirectory())
+ .as("Should create metadata folder")
+ .isTrue();
+ Assertions.assertThat(version(1).exists() && version(1).isFile())
Review Comment:
```suggestion
Assertions.assertThat(version(1)).exists().isFile();
```
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238385936
##########
core/src/test/java/org/apache/iceberg/rest/RequestResponseTestBase.java:
##########
@@ -76,11 +75,13 @@ public void testHasOnlyKnownFields() {
try {
JsonNode node = mapper().readValue(serialize(createExampleInstance()), JsonNode.class);
for (String field : fieldsFromSpec) {
- Assert.assertTrue("Should have field: " + field, node.has(field));
+ Assertions.assertThat(node.has(field)).as("Should have field: %s", field).isTrue();
}
for (String field : ((Iterable<? extends String>) node::fieldNames)) {
- Assert.assertTrue("Should not have field: " + field, fieldsFromSpec.contains(field));
+ Assertions.assertThat(fieldsFromSpec.contains(field))
Review Comment:
I modified the code based on the method you presented.
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238264179
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -51,7 +51,6 @@
import org.apache.iceberg.transforms.Transform;
import org.apache.iceberg.transforms.Transforms;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
import org.junit.Test;
Review Comment:
I've modified that code based on your review, 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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238669008
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -335,26 +342,26 @@ public void testListNamespace() throws Exception {
List<Namespace> nsp1 = catalog.listNamespaces(Namespace.of("db"));
Set<String> tblSet = Sets.newHashSet(nsp1.stream().map(t -> t.toString()).iterator());
- Assert.assertEquals(3, tblSet.size());
- Assert.assertTrue(tblSet.contains("db.ns1"));
- Assert.assertTrue(tblSet.contains("db.ns2"));
- Assert.assertTrue(tblSet.contains("db.ns3"));
+ Assertions.assertThat(tblSet).hasSize(3);
+ Assertions.assertThat(tblSet).contains("db.ns1");
+ Assertions.assertThat(tblSet).contains("db.ns2");
+ Assertions.assertThat(tblSet).contains("db.ns3");
List<Namespace> nsp2 = catalog.listNamespaces(Namespace.of("db", "ns1"));
- Assert.assertEquals(1, nsp2.size());
- Assert.assertTrue(nsp2.get(0).toString().equals("db.ns1.ns2"));
+ Assertions.assertThat(nsp2).hasSize(1);
+ Assertions.assertThat(nsp2.get(0).toString()).isEqualTo("db.ns1.ns2");
List<Namespace> nsp3 = catalog.listNamespaces();
Set<String> tblSet2 = Sets.newHashSet(nsp3.stream().map(t -> t.toString()).iterator());
- Assert.assertEquals(2, tblSet2.size());
- Assert.assertTrue(tblSet2.contains("db"));
- Assert.assertTrue(tblSet2.contains("db2"));
+ Assertions.assertThat(tblSet2).hasSize(2);
+ Assertions.assertThat(tblSet2).contains("db");
+ Assertions.assertThat(tblSet2).contains("db2");
Review Comment:
I apologize for some things not being changed, I just went through all the code in this PR and fixed it all based on your review.
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238384474
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -105,23 +104,25 @@ public void testReplaceTxnBuilder() throws Exception {
createTxn.commitTransaction();
Table table = catalog.loadTable(tableIdent);
- Assert.assertNotNull(table.currentSnapshot());
+ Assertions.assertThat(table.currentSnapshot()).isNotNull();
Transaction replaceTxn =
catalog.buildTable(tableIdent, SCHEMA).withProperty("key2", "value2").replaceTransaction();
replaceTxn.commitTransaction();
table = catalog.loadTable(tableIdent);
- Assert.assertNull(table.currentSnapshot());
+ Assertions.assertThat(table.currentSnapshot()).isNull();
PartitionSpec v1Expected =
PartitionSpec.builderFor(table.schema())
.alwaysNull("data", "data_bucket")
.withSpecId(1)
.build();
- Assert.assertEquals("Table should have a spec with one void field", v1Expected, table.spec());
+ Assertions.assertThat(table.spec())
+ .as("Table should have a spec with one void field")
+ .isEqualTo(v1Expected);
- Assert.assertEquals("value1", table.properties().get("key1"));
- Assert.assertEquals("value2", table.properties().get("key2"));
+ Assertions.assertThat(table.properties().get("key1")).isEqualTo("value1");
Review Comment:
I modified the code based on the method you suggested.
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238392276
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -55,104 +53,103 @@ public class TestHadoopTables {
required(1, "id", Types.IntegerType.get(), "unique ID"),
required(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
- private File tableDir = null;
-
- @Before
- public void setupTableLocation() throws Exception {
- tableDir = temp.newFolder();
- }
+ @TempDir private Path tableDir;
Review Comment:
Based on your review, I've changed the code and deleted toUri() changes.
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238761291
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -92,27 +83,23 @@ public void testDropTableWithPurge() throws IOException {
.isInstanceOf(NoSuchTableException.class)
.hasMessageStartingWith("Table does not exist");
- Assert.assertEquals(0, dataDir.listFiles().length);
- Assert.assertFalse(tableDir.exists());
-
- Assert.assertFalse(TABLES.dropTable(tableDir.toURI().toString()));
+ Assertions.assertThat(Files.list(dataDir.toPath()).count()).isEqualTo(0);
+ Assertions.assertThat(tableDir).doesNotExist();
+ Assertions.assertThat(TABLES.dropTable(tableDir.toURI().toString())).isFalse();
}
@Test
public void testDropTableWithoutPurge() throws IOException {
- File dataDir = temp.newFolder();
-
createDummyTable(tableDir, dataDir);
TABLES.dropTable(tableDir.toURI().toString(), false);
Assertions.assertThatThrownBy(() -> TABLES.load(tableDir.toURI().toString()))
.isInstanceOf(NoSuchTableException.class)
.hasMessageStartingWith("Table does not exist");
- Assert.assertEquals(1, dataDir.listFiles().length);
- Assert.assertFalse(tableDir.exists());
-
- Assert.assertFalse(TABLES.dropTable(tableDir.toURI().toString()));
+ Assertions.assertThat(Files.list(dataDir.toPath()).count()).isEqualTo(1);
Review Comment:
I've modified `Files.list(dataDir.toPath()).count().isEqual` to `dataDir.listFiles()).hasSize(1)`
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238672889
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -105,23 +104,25 @@ public void testReplaceTxnBuilder() throws Exception {
createTxn.commitTransaction();
Table table = catalog.loadTable(tableIdent);
- Assert.assertNotNull(table.currentSnapshot());
+ Assertions.assertThat(table.currentSnapshot()).isNotNull();
Transaction replaceTxn =
catalog.buildTable(tableIdent, SCHEMA).withProperty("key2", "value2").replaceTransaction();
replaceTxn.commitTransaction();
table = catalog.loadTable(tableIdent);
- Assert.assertNull(table.currentSnapshot());
+ Assertions.assertThat(table.currentSnapshot()).isNull();
PartitionSpec v1Expected =
PartitionSpec.builderFor(table.schema())
.alwaysNull("data", "data_bucket")
.withSpecId(1)
.build();
- Assert.assertEquals("Table should have a spec with one void field", v1Expected, table.spec());
+ Assertions.assertThat(table.spec())
+ .as("Table should have a spec with one void field")
+ .isEqualTo(v1Expected);
- Assert.assertEquals("value1", table.properties().get("key1"));
- Assert.assertEquals("value2", table.properties().get("key2"));
+ Assertions.assertThat(table.properties()).containsEntry("key1", "value1");
+ Assertions.assertThat(table.properties()).containsEntry("key2", "value2");
Review Comment:
can be chained together
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238608682
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -404,14 +444,14 @@ public void testCanReadOldCompressedManifestFiles() throws Exception {
Table reloaded = TABLES.load(tableLocation);
List<FileScanTask> tasks = Lists.newArrayList(reloaded.newScan().planFiles());
- Assert.assertEquals("Should scan 1 files", 1, tasks.size());
+ Assertions.assertThat(tasks).as("Should scan 1 files").hasSize(1);
}
@Test
public void testConcurrentFastAppends() throws Exception {
assertTrue("Should create v1 metadata", version(1).exists() && version(1).isFile());
- File dir = temp.newFolder();
- dir.delete();
+ File dir = tableDir;
+ FileUtils.deleteDirectory(dir);
Review Comment:
`Table already exists at location: file:/tmp/junit5635809745002339218/`
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238643515
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -404,14 +444,14 @@ public void testCanReadOldCompressedManifestFiles() throws Exception {
Table reloaded = TABLES.load(tableLocation);
List<FileScanTask> tasks = Lists.newArrayList(reloaded.newScan().planFiles());
- Assert.assertEquals("Should scan 1 files", 1, tasks.size());
+ Assertions.assertThat(tasks).as("Should scan 1 files").hasSize(1);
}
@Test
public void testConcurrentFastAppends() throws Exception {
assertTrue("Should create v1 metadata", version(1).exists() && version(1).isFile());
- File dir = temp.newFolder();
- dir.delete();
+ File dir = tableDir;
+ FileUtils.deleteDirectory(dir);
Review Comment:
then it's probably better to inject a separate temp dir for this particular test using `testConcurrentFastAppends(@TempDir File dir)` as a method parameter
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238667006
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -267,13 +273,13 @@ public void testListTables() throws Exception {
List<TableIdentifier> tbls1 = catalog.listTables(Namespace.of("db"));
Set<String> tblSet = Sets.newHashSet(tbls1.stream().map(t -> t.name()).iterator());
- Assert.assertEquals(2, tblSet.size());
- Assert.assertTrue(tblSet.contains("tbl1"));
- Assert.assertTrue(tblSet.contains("tbl2"));
+ Assertions.assertThat(tblSet).hasSize(2);
Review Comment:
I went through all of my code and shortened to `Assertions.assertThat(tblSet).hasSize(...).contains(x).contains(y)` .
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238667006
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -267,13 +273,13 @@ public void testListTables() throws Exception {
List<TableIdentifier> tbls1 = catalog.listTables(Namespace.of("db"));
Set<String> tblSet = Sets.newHashSet(tbls1.stream().map(t -> t.name()).iterator());
- Assert.assertEquals(2, tblSet.size());
- Assert.assertTrue(tblSet.contains("tbl1"));
- Assert.assertTrue(tblSet.contains("tbl2"));
+ Assertions.assertThat(tblSet).hasSize(2);
Review Comment:
I went through all of my code and shortened wherever I could.
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238051299
##########
core/src/test/java/org/apache/iceberg/hadoop/HadoopTableTestBase.java:
##########
@@ -113,17 +113,17 @@ public class HadoopTableTestBase {
.withRecordCount(2) // needs at least one record or else metrics will filter it out
.build();
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir Path temp;
Review Comment:
```suggestion
@TempDir File tableDir;
```
and then we can remove all `this.tableDir = temp.toFile()` calls
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238041612
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCachingCatalog.java:
##########
@@ -291,9 +292,10 @@ public void testCacheExpirationEagerlyRemovesMetadataTables() throws IOException
Arrays.stream(metadataTables(tableIdent))
.forEach(
metadataTable ->
- Assert.assertFalse(
- "When a data table expires, its metadata tables should expire regardless of age",
- catalog.cache().asMap().containsKey(metadataTable)));
+ Assertions.assertThat(catalog.cache().asMap().containsKey(metadataTable))
Review Comment:
I modified the code based on the method you suggested. `.doesNotContainKeys(metadataTable)`
--
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
[GitHub] [iceberg] skytin1004 commented on pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#issuecomment-1602481848
Once again, thank you so much for your detailed review @nastra . I made some code changes based on your suggestions and the test issue was resolved.
And I modified the code based on your review, using
`@TempDir private File tableDir;
@TempDir private Path dataDir;`
to eliminate unnecessary `toUri()`changes.
And I modified the code based on the method `.containsEntry(...).containsEntry(..)` you suggested to
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238747028
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -92,27 +83,23 @@ public void testDropTableWithPurge() throws IOException {
.isInstanceOf(NoSuchTableException.class)
.hasMessageStartingWith("Table does not exist");
- Assert.assertEquals(0, dataDir.listFiles().length);
- Assert.assertFalse(tableDir.exists());
-
- Assert.assertFalse(TABLES.dropTable(tableDir.toURI().toString()));
+ Assertions.assertThat(Files.list(dataDir.toPath()).count()).isEqualTo(0);
Review Comment:
given that we switched back dataDir to a File, we don't need to call `Files.list(..)` here right?
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -92,27 +83,23 @@ public void testDropTableWithPurge() throws IOException {
.isInstanceOf(NoSuchTableException.class)
.hasMessageStartingWith("Table does not exist");
- Assert.assertEquals(0, dataDir.listFiles().length);
- Assert.assertFalse(tableDir.exists());
-
- Assert.assertFalse(TABLES.dropTable(tableDir.toURI().toString()));
+ Assertions.assertThat(Files.list(dataDir.toPath()).count()).isEqualTo(0);
+ Assertions.assertThat(tableDir).doesNotExist();
+ Assertions.assertThat(TABLES.dropTable(tableDir.toURI().toString())).isFalse();
}
@Test
public void testDropTableWithoutPurge() throws IOException {
- File dataDir = temp.newFolder();
-
createDummyTable(tableDir, dataDir);
TABLES.dropTable(tableDir.toURI().toString(), false);
Assertions.assertThatThrownBy(() -> TABLES.load(tableDir.toURI().toString()))
.isInstanceOf(NoSuchTableException.class)
.hasMessageStartingWith("Table does not exist");
- Assert.assertEquals(1, dataDir.listFiles().length);
- Assert.assertFalse(tableDir.exists());
-
- Assert.assertFalse(TABLES.dropTable(tableDir.toURI().toString()));
+ Assertions.assertThat(Files.list(dataDir.toPath()).count()).isEqualTo(1);
Review Comment:
same 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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238674431
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -267,13 +273,11 @@ public void testListTables() throws Exception {
List<TableIdentifier> tbls1 = catalog.listTables(Namespace.of("db"));
Set<String> tblSet = Sets.newHashSet(tbls1.stream().map(t -> t.name()).iterator());
- Assert.assertEquals(2, tblSet.size());
- Assert.assertTrue(tblSet.contains("tbl1"));
- Assert.assertTrue(tblSet.contains("tbl2"));
+ Assertions.assertThat(tblSet).hasSize(2).contains("tbl1").contains("tbl2");
List<TableIdentifier> tbls2 = catalog.listTables(Namespace.of("db", "ns1"));
- Assert.assertEquals("table identifiers", 1, tbls2.size());
- Assert.assertEquals("table name", "tbl3", tbls2.get(0).name());
+ Assertions.assertThat(tbls2).as("table identifiers").hasSize(1);
Review Comment:
nit: the `.as(..)` part here is meaningless and can be removed. Same for the `.as("table name")`
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238668695
##########
core/src/test/java/org/apache/iceberg/hadoop/HadoopFileIOTest.java:
##########
@@ -111,16 +110,16 @@ public void testDeletePrefix() {
hadoopFileIO.deletePrefix(scalePath.toUri().toString());
// Hadoop filesystem will throw if the path does not exist
- assertThrows(
- UncheckedIOException.class,
- () -> hadoopFileIO.listPrefix(scalePath.toUri().toString()).iterator());
+ Assertions.assertThatThrownBy(
+ () -> hadoopFileIO.listPrefix(scalePath.toUri().toString()).iterator())
+ .isInstanceOf(UncheckedIOException.class);
Review Comment:
can we add a `.hasMessage(..)` check here and below?
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238029445
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -55,20 +54,19 @@ public class TestHadoopTables {
required(1, "id", Types.IntegerType.get(), "unique ID"),
required(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
private File tableDir = null;
- @Before
- public void setupTableLocation() throws Exception {
- tableDir = temp.newFolder();
+ @BeforeEach
+ public void setupTableLocation(@TempDir Path temp) throws Exception {
Review Comment:
Thank you for your review. I have now fixed the code based on your review and switched the file to JUnit5. I will review all your reviews and post a commit.
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -55,20 +54,19 @@ public class TestHadoopTables {
required(1, "id", Types.IntegerType.get(), "unique ID"),
required(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
private File tableDir = null;
- @Before
- public void setupTableLocation() throws Exception {
- tableDir = temp.newFolder();
+ @BeforeEach
+ public void setupTableLocation(@TempDir Path temp) throws Exception {
Review Comment:
Thank you for your review! I have now fixed the code based on your review and switched the file to JUnit5. I will review all your reviews and post a commit.
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238264745
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCatalogUtilDropTable.java:
##########
@@ -124,8 +131,12 @@ public void shouldNotDropDataFilesIfGcNotEnabled() {
Set<String> manifestListLocations = manifestListLocations(snapshotSet);
Set<String> manifestLocations = manifestLocations(snapshotSet, table.io());
Set<String> metadataLocations = metadataLocations(tableMetadata);
- Assert.assertEquals("should have 2 manifest lists", 2, manifestListLocations.size());
- Assert.assertEquals("should have 4 metadata locations", 4, metadataLocations.size());
+ Assertions.assertThat(manifestListLocations.size())
+ .as("should have 2 manifest lists")
+ .isEqualTo(2);
+ Assertions.assertThat(metadataLocations.size())
+ .as("should have 4 metadata locations")
+ .isEqualTo(4);
Review Comment:
I've modified that code based on your review.
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238608682
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -404,14 +444,14 @@ public void testCanReadOldCompressedManifestFiles() throws Exception {
Table reloaded = TABLES.load(tableLocation);
List<FileScanTask> tasks = Lists.newArrayList(reloaded.newScan().planFiles());
- Assert.assertEquals("Should scan 1 files", 1, tasks.size());
+ Assertions.assertThat(tasks).as("Should scan 1 files").hasSize(1);
}
@Test
public void testConcurrentFastAppends() throws Exception {
assertTrue("Should create v1 metadata", version(1).exists() && version(1).isFile());
- File dir = temp.newFolder();
- dir.delete();
+ File dir = tableDir;
+ FileUtils.deleteDirectory(dir);
Review Comment:
If don't use FileUtils.deleteDirectory(dir), get an error like the one below.
`Table already exists at location: file:/tmp/junit5635809745002339218/`
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238670248
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -404,14 +444,14 @@ public void testCanReadOldCompressedManifestFiles() throws Exception {
Table reloaded = TABLES.load(tableLocation);
List<FileScanTask> tasks = Lists.newArrayList(reloaded.newScan().planFiles());
- Assert.assertEquals("Should scan 1 files", 1, tasks.size());
+ Assertions.assertThat(tasks).as("Should scan 1 files").hasSize(1);
}
@Test
public void testConcurrentFastAppends() throws Exception {
assertTrue("Should create v1 metadata", version(1).exists() && version(1).isFile());
- File dir = temp.newFolder();
- dir.delete();
+ File dir = tableDir;
+ FileUtils.deleteDirectory(dir);
Review Comment:
Thank you for the suggestion, I've modified the code based on your suggestion.
--
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
[GitHub] [iceberg] skytin1004 commented on pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#issuecomment-1603006555
Thank you @nastra, I've fixed the code based on your review.
There was a lot that needed to be fixed in my PR this time around.
I'm glad to be able to fix the problem with PR based on your detailed review.
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238672265
##########
core/src/test/java/org/apache/iceberg/hadoop/HadoopFileIOTest.java:
##########
@@ -111,16 +110,16 @@ public void testDeletePrefix() {
hadoopFileIO.deletePrefix(scalePath.toUri().toString());
// Hadoop filesystem will throw if the path does not exist
- assertThrows(
- UncheckedIOException.class,
- () -> hadoopFileIO.listPrefix(scalePath.toUri().toString()).iterator());
+ Assertions.assertThatThrownBy(
+ () -> hadoopFileIO.listPrefix(scalePath.toUri().toString()).iterator())
+ .isInstanceOf(UncheckedIOException.class);
Review Comment:
I'll add the `.hasmessage` .
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238261542
##########
core/src/test/java/org/apache/iceberg/hadoop/TestStaticTable.java:
##########
@@ -88,13 +87,13 @@ public void testHasSameProperties() {
table.newAppend().appendFile(FILE_B).commit();
table.newOverwrite().deleteFile(FILE_B).addFile(FILE_C).commit();
Table staticTable = getStaticTable();
- Assert.assertTrue("Same history?", table.history().containsAll(staticTable.history()));
- Assert.assertTrue(
- "Same snapshot?",
- table.currentSnapshot().snapshotId() == staticTable.currentSnapshot().snapshotId());
- Assert.assertTrue(
- "Same properties?",
- Maps.difference(table.properties(), staticTable.properties()).areEqual());
+ Assertions.assertThat(table.history()).as("Same history?").containsAll(staticTable.history());
+ Assertions.assertThat(table.currentSnapshot().snapshotId())
+ .as("Same snapshot?")
+ .isEqualTo(staticTable.currentSnapshot().snapshotId());
+ Assertions.assertThat(Maps.difference(table.properties(), staticTable.properties()).areEqual())
Review Comment:
I've modified that code based on what you suggested.
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238257276
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -92,15 +90,15 @@ public void testDropTableWithPurge() throws IOException {
.isInstanceOf(NoSuchTableException.class)
.hasMessageStartingWith("Table does not exist");
- Assert.assertEquals(0, dataDir.listFiles().length);
- Assert.assertFalse(tableDir.exists());
+ Assertions.assertThat(dataDir.listFiles()).hasSize(0);
+ Assertions.assertThat(tableDir).doesNotExist();
- Assert.assertFalse(TABLES.dropTable(tableDir.toURI().toString()));
+ Assertions.assertThat(TABLES.dropTable(tableDir.toURI().toString())).isFalse();
}
@Test
- public void testDropTableWithoutPurge() throws IOException {
- File dataDir = temp.newFolder();
+ public void testDropTableWithoutPurge(@TempDir Path temp) throws IOException {
Review Comment:
I replaced them all with tableDir.
--
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
[GitHub] [iceberg] skytin1004 commented on pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#issuecomment-1602302764
Thank you so much for your very detailed review @nastra .
Based on your review, I was able to change the Hadoop package to JUnit5, incorporating all the things you reviewed.
However, there is an issue. When I run the tests in TestHadoopCatalog, I get the following results
The test "testListNamespace" fails.
I investigated the problem through debug and found that the namespaces "db", "db2" in the root directory are not being looked up properly, since switching to JUnit5 In that method.
So I would like to get some Solutions from you. Thank you again for the detailed review :)
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238726805
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -57,8 +58,7 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.Tasks;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
Review Comment:
Based on your review, I've changed all the remaining JUnit4 code in this PR 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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1233601694
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCachingCatalog.java:
##########
@@ -291,9 +292,10 @@ public void testCacheExpirationEagerlyRemovesMetadataTables() throws IOException
Arrays.stream(metadataTables(tableIdent))
.forEach(
metadataTable ->
- Assert.assertFalse(
- "When a data table expires, its metadata tables should expire regardless of age",
- catalog.cache().asMap().containsKey(metadataTable)));
+ Assertions.assertThat(catalog.cache().asMap().containsKey(metadataTable))
Review Comment:
```suggestion
Assertions.assertThat(catalog.cache().asMap()).containsKey(metadataTable)
```
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCatalogUtilDropTable.java:
##########
@@ -49,8 +49,12 @@ public void dropTableDataDeletesExpectedFiles() {
Set<String> manifestLocations = manifestLocations(snapshotSet, table.io());
Set<String> dataLocations = dataLocations(snapshotSet, table.io());
Set<String> metadataLocations = metadataLocations(tableMetadata);
- Assert.assertEquals("should have 2 manifest lists", 2, manifestListLocations.size());
- Assert.assertEquals("should have 3 metadata locations", 3, metadataLocations.size());
+ Assertions.assertThat(manifestListLocations.size())
Review Comment:
```suggestion
Assertions.assertThat(manifestListLocations).hasSize(2)
```
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCatalogUtilDropTable.java:
##########
@@ -49,8 +49,12 @@ public void dropTableDataDeletesExpectedFiles() {
Set<String> manifestLocations = manifestLocations(snapshotSet, table.io());
Set<String> dataLocations = dataLocations(snapshotSet, table.io());
Set<String> metadataLocations = metadataLocations(tableMetadata);
- Assert.assertEquals("should have 2 manifest lists", 2, manifestListLocations.size());
- Assert.assertEquals("should have 3 metadata locations", 3, metadataLocations.size());
+ Assertions.assertThat(manifestListLocations.size())
+ .as("should have 2 manifest lists")
+ .isEqualTo(2);
+ Assertions.assertThat(metadataLocations.size())
Review Comment:
```suggestion
Assertions.assertThat(metadataLocations).hasSize(3)
```
##########
core/src/test/java/org/apache/iceberg/hadoop/TestTableSerialization.java:
##########
@@ -41,7 +41,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.types.Types;
-import org.junit.Assert;
+import org.assertj.core.api.Assertions;
import org.junit.Test;
Review Comment:
needs to be switched to Junit5
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -57,7 +58,6 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.Tasks;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
import org.junit.Test;
Review Comment:
needs to be switched to Junit5
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCatalogUtilDropTable.java:
##########
@@ -124,8 +131,12 @@ public void shouldNotDropDataFilesIfGcNotEnabled() {
Set<String> manifestListLocations = manifestListLocations(snapshotSet);
Set<String> manifestLocations = manifestLocations(snapshotSet, table.io());
Set<String> metadataLocations = metadataLocations(tableMetadata);
- Assert.assertEquals("should have 2 manifest lists", 2, manifestListLocations.size());
- Assert.assertEquals("should have 4 metadata locations", 4, metadataLocations.size());
+ Assertions.assertThat(manifestListLocations.size())
+ .as("should have 2 manifest lists")
+ .isEqualTo(2);
+ Assertions.assertThat(metadataLocations.size())
+ .as("should have 4 metadata locations")
+ .isEqualTo(4);
Review Comment:
same as above for both assertions
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -55,20 +54,19 @@ public class TestHadoopTables {
required(1, "id", Types.IntegerType.get(), "unique ID"),
required(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
private File tableDir = null;
Review Comment:
I think we can just add a `@TempDir` annotation to `tableDir` and remove `@TempDir Path temp` alltogether?
##########
core/src/test/java/org/apache/iceberg/hadoop/TestCachingCatalog.java:
##########
@@ -291,9 +292,10 @@ public void testCacheExpirationEagerlyRemovesMetadataTables() throws IOException
Arrays.stream(metadataTables(tableIdent))
.forEach(
metadataTable ->
- Assert.assertFalse(
- "When a data table expires, its metadata tables should expire regardless of age",
- catalog.cache().asMap().containsKey(metadataTable)));
+ Assertions.assertThat(catalog.cache().asMap().containsKey(metadataTable))
Review Comment:
we want to avoid isTrue/isFalse as much as possible and use assertions that are more descriptive
##########
core/src/test/java/org/apache/iceberg/hadoop/TestStaticTable.java:
##########
@@ -24,7 +24,6 @@
import org.apache.iceberg.Table;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
import org.junit.Test;
Review Comment:
needs to be switched to Junit5
##########
core/src/test/java/org/apache/iceberg/hadoop/TestStaticTable.java:
##########
@@ -88,13 +87,13 @@ public void testHasSameProperties() {
table.newAppend().appendFile(FILE_B).commit();
table.newOverwrite().deleteFile(FILE_B).addFile(FILE_C).commit();
Table staticTable = getStaticTable();
- Assert.assertTrue("Same history?", table.history().containsAll(staticTable.history()));
- Assert.assertTrue(
- "Same snapshot?",
- table.currentSnapshot().snapshotId() == staticTable.currentSnapshot().snapshotId());
- Assert.assertTrue(
- "Same properties?",
- Maps.difference(table.properties(), staticTable.properties()).areEqual());
+ Assertions.assertThat(table.history()).as("Same history?").containsAll(staticTable.history());
+ Assertions.assertThat(table.currentSnapshot().snapshotId())
+ .as("Same snapshot?")
+ .isEqualTo(staticTable.currentSnapshot().snapshotId());
+ Assertions.assertThat(Maps.difference(table.properties(), staticTable.properties()).areEqual())
Review Comment:
can we express that with an AssertJ assertion rather than using isTrue?
##########
core/src/test/java/org/apache/iceberg/rest/RequestResponseTestBase.java:
##########
@@ -76,11 +75,13 @@ public void testHasOnlyKnownFields() {
try {
JsonNode node = mapper().readValue(serialize(createExampleInstance()), JsonNode.class);
for (String field : fieldsFromSpec) {
- Assert.assertTrue("Should have field: " + field, node.has(field));
+ Assertions.assertThat(node.has(field)).as("Should have field: %s", field).isTrue();
}
for (String field : ((Iterable<? extends String>) node::fieldNames)) {
- Assert.assertTrue("Should not have field: " + field, fieldsFromSpec.contains(field));
+ Assertions.assertThat(fieldsFromSpec.contains(field))
Review Comment:
should be `Assertions.assertThat(fieldsFromSpec).contains(field);`
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -51,7 +51,6 @@
import org.apache.iceberg.transforms.Transform;
import org.apache.iceberg.transforms.Transforms;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
import org.junit.Test;
Review Comment:
we should also switch this to JUnit5.
For switching `HadoopTableTestBase` you'd need to switch from `@Rule public TemporaryFolder temp = new TemporaryFolder();` to `@TempDir File temp` (similarly to how it's done in other test classes)
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238749539
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -92,27 +83,23 @@ public void testDropTableWithPurge() throws IOException {
.isInstanceOf(NoSuchTableException.class)
.hasMessageStartingWith("Table does not exist");
- Assert.assertEquals(0, dataDir.listFiles().length);
- Assert.assertFalse(tableDir.exists());
-
- Assert.assertFalse(TABLES.dropTable(tableDir.toURI().toString()));
+ Assertions.assertThat(Files.list(dataDir.toPath()).count()).isEqualTo(0);
Review Comment:
Your review is correct. I'll modify the code now.
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238479969
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -267,13 +273,13 @@ public void testListTables() throws Exception {
List<TableIdentifier> tbls1 = catalog.listTables(Namespace.of("db"));
Set<String> tblSet = Sets.newHashSet(tbls1.stream().map(t -> t.name()).iterator());
- Assert.assertEquals(2, tblSet.size());
- Assert.assertTrue(tblSet.contains("tbl1"));
- Assert.assertTrue(tblSet.contains("tbl2"));
+ Assertions.assertThat(tblSet).hasSize(2);
Review Comment:
nit: I think this can be shortened to `Assertions.assertThat(tblSet).hasSize(2).contains(x).contains(y)`
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -335,26 +342,26 @@ public void testListNamespace() throws Exception {
List<Namespace> nsp1 = catalog.listNamespaces(Namespace.of("db"));
Set<String> tblSet = Sets.newHashSet(nsp1.stream().map(t -> t.toString()).iterator());
- Assert.assertEquals(3, tblSet.size());
- Assert.assertTrue(tblSet.contains("db.ns1"));
- Assert.assertTrue(tblSet.contains("db.ns2"));
- Assert.assertTrue(tblSet.contains("db.ns3"));
+ Assertions.assertThat(tblSet).hasSize(3);
+ Assertions.assertThat(tblSet).contains("db.ns1");
+ Assertions.assertThat(tblSet).contains("db.ns2");
+ Assertions.assertThat(tblSet).contains("db.ns3");
List<Namespace> nsp2 = catalog.listNamespaces(Namespace.of("db", "ns1"));
- Assert.assertEquals(1, nsp2.size());
- Assert.assertTrue(nsp2.get(0).toString().equals("db.ns1.ns2"));
+ Assertions.assertThat(nsp2).hasSize(1);
+ Assertions.assertThat(nsp2.get(0).toString()).isEqualTo("db.ns1.ns2");
List<Namespace> nsp3 = catalog.listNamespaces();
Set<String> tblSet2 = Sets.newHashSet(nsp3.stream().map(t -> t.toString()).iterator());
- Assert.assertEquals(2, tblSet2.size());
- Assert.assertTrue(tblSet2.contains("db"));
- Assert.assertTrue(tblSet2.contains("db2"));
+ Assertions.assertThat(tblSet2).hasSize(2);
+ Assertions.assertThat(tblSet2).contains("db");
+ Assertions.assertThat(tblSet2).contains("db2");
Review Comment:
same as above (please also adjust the other places being touched by this PR)
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -55,104 +53,101 @@ public class TestHadoopTables {
required(1, "id", Types.IntegerType.get(), "unique ID"),
required(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
- private File tableDir = null;
-
- @Before
- public void setupTableLocation() throws Exception {
- tableDir = temp.newFolder();
- }
+ @TempDir private File tableDir;
+ @TempDir private Path dataDir;
@Test
public void testTableExists() {
- Assert.assertFalse(TABLES.exists(tableDir.toURI().toString()));
Review Comment:
sorry for not being precise around the toUri changes. What I meant in my previous comment is that if we make `tableDir` a `File`, then we don't need any `toURI()` -> `toUri()` changes.
That being said, I don't see any particular reason to remove these `toURI()` calls
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -404,14 +444,14 @@ public void testCanReadOldCompressedManifestFiles() throws Exception {
Table reloaded = TABLES.load(tableLocation);
List<FileScanTask> tasks = Lists.newArrayList(reloaded.newScan().planFiles());
- Assert.assertEquals("Should scan 1 files", 1, tasks.size());
+ Assertions.assertThat(tasks).as("Should scan 1 files").hasSize(1);
}
@Test
public void testConcurrentFastAppends() throws Exception {
assertTrue("Should create v1 metadata", version(1).exists() && version(1).isFile());
- File dir = temp.newFolder();
- dir.delete();
+ File dir = tableDir;
+ FileUtils.deleteDirectory(dir);
Review Comment:
not sure I understand why we need to delete this here?
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -67,55 +67,69 @@ public class TestHadoopCommits extends HadoopTableTestBase {
public void testCreateTable() throws Exception {
PartitionSpec expectedSpec = PartitionSpec.builderFor(TABLE_SCHEMA).bucket("data", 16).build();
- Assert.assertEquals(
- "Table schema should match schema with reassigned ids",
- TABLE_SCHEMA.asStruct(),
- table.schema().asStruct());
- Assert.assertEquals(
- "Table partition spec should match with reassigned ids", expectedSpec, table.spec());
+ Assertions.assertThat(table.schema().asStruct())
+ .as("Table schema should match schema with reassigned ids")
+ .isEqualTo(TABLE_SCHEMA.asStruct());
+ Assertions.assertThat(table.spec())
+ .as("Table partition spec should match with reassigned ids")
+ .isEqualTo(expectedSpec);
List<FileScanTask> tasks = Lists.newArrayList(table.newScan().planFiles());
- Assert.assertEquals("Should not create any scan tasks", 0, tasks.size());
-
- Assert.assertTrue("Table location should exist", tableDir.exists());
- Assert.assertTrue(
- "Should create metadata folder", metadataDir.exists() && metadataDir.isDirectory());
- Assert.assertTrue("Should create v1 metadata", version(1).exists() && version(1).isFile());
- Assert.assertFalse("Should not create v2 or newer versions", version(2).exists());
- Assert.assertTrue("Should create version hint file", versionHintFile.exists());
- Assert.assertEquals("Should write the current version to the hint file", 1, readVersionHint());
-
+ Assertions.assertThat(tasks).as("Should not create any scan tasks").isEmpty();
+ Assertions.assertThat(tableDir.exists()).as("Table location should exist").isTrue();
Review Comment:
```suggestion
Assertions.assertThat(tableDir).as("Table location should exist").exists();
```
same for the check against `metadataDir` and all other places part of this PR
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -335,26 +342,26 @@ public void testListNamespace() throws Exception {
List<Namespace> nsp1 = catalog.listNamespaces(Namespace.of("db"));
Set<String> tblSet = Sets.newHashSet(nsp1.stream().map(t -> t.toString()).iterator());
- Assert.assertEquals(3, tblSet.size());
- Assert.assertTrue(tblSet.contains("db.ns1"));
- Assert.assertTrue(tblSet.contains("db.ns2"));
- Assert.assertTrue(tblSet.contains("db.ns3"));
+ Assertions.assertThat(tblSet).hasSize(3);
+ Assertions.assertThat(tblSet).contains("db.ns1");
+ Assertions.assertThat(tblSet).contains("db.ns2");
+ Assertions.assertThat(tblSet).contains("db.ns3");
Review Comment:
same as above
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -57,8 +58,7 @@
import org.apache.iceberg.types.Types;
import org.apache.iceberg.util.Tasks;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
Review Comment:
this class is still using
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
and those should all be switched to assertJ
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238728332
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -55,104 +53,101 @@ public class TestHadoopTables {
required(1, "id", Types.IntegerType.get(), "unique ID"),
required(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
- private File tableDir = null;
-
- @Before
- public void setupTableLocation() throws Exception {
- tableDir = temp.newFolder();
- }
+ @TempDir private File tableDir;
+ @TempDir private Path dataDir;
@Test
public void testTableExists() {
- Assert.assertFalse(TABLES.exists(tableDir.toURI().toString()));
Review Comment:
Based on your review, I've restored the `toURI()` calls to its original state. It looks like I would misunderstood.
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238758022
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -69,10 +68,10 @@ public void testCreateTableBuilder() throws Exception {
.withProperties(ImmutableMap.of("key2", "value2"))
.create();
- Assert.assertEquals(TABLE_SCHEMA.toString(), table.schema().toString());
- Assert.assertEquals(1, table.spec().fields().size());
- Assert.assertEquals("value1", table.properties().get("key1"));
- Assert.assertEquals("value2", table.properties().get("key2"));
+ Assertions.assertThat(table.schema().toString()).isEqualTo(TABLE_SCHEMA.toString());
+ Assertions.assertThat(table.spec().fields()).hasSize(1);
+ Assertions.assertThat(table.properties()).containsEntry("key1", "value1");
+ Assertions.assertThat(table.properties()).containsEntry("key2", "value2");
Review Comment:
Based on your review, I've modified the code to be more concise.
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -105,23 +104,25 @@ public void testReplaceTxnBuilder() throws Exception {
createTxn.commitTransaction();
Table table = catalog.loadTable(tableIdent);
- Assert.assertNotNull(table.currentSnapshot());
+ Assertions.assertThat(table.currentSnapshot()).isNotNull();
Transaction replaceTxn =
catalog.buildTable(tableIdent, SCHEMA).withProperty("key2", "value2").replaceTransaction();
replaceTxn.commitTransaction();
table = catalog.loadTable(tableIdent);
- Assert.assertNull(table.currentSnapshot());
+ Assertions.assertThat(table.currentSnapshot()).isNull();
PartitionSpec v1Expected =
PartitionSpec.builderFor(table.schema())
.alwaysNull("data", "data_bucket")
.withSpecId(1)
.build();
- Assert.assertEquals("Table should have a spec with one void field", v1Expected, table.spec());
+ Assertions.assertThat(table.spec())
+ .as("Table should have a spec with one void field")
+ .isEqualTo(v1Expected);
- Assert.assertEquals("value1", table.properties().get("key1"));
- Assert.assertEquals("value2", table.properties().get("key2"));
+ Assertions.assertThat(table.properties()).containsEntry("key1", "value1");
+ Assertions.assertThat(table.properties()).containsEntry("key2", "value2");
Review Comment:
Based on your review, I've modified the code to be more concise.
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238759449
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -530,13 +533,14 @@ public void testTableName() throws Exception {
catalog.buildTable(tableIdent, SCHEMA).withPartitionSpec(SPEC).create();
Table table = catalog.loadTable(tableIdent);
- Assert.assertEquals("Name must match", "hadoop.db.ns1.ns2.tbl", table.name());
+ Assertions.assertThat(table.name()).as("Name must match").isEqualTo("hadoop.db.ns1.ns2.tbl");
Review Comment:
Based on your review, I've removed unnecessary descriptions.
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -267,13 +273,11 @@ public void testListTables() throws Exception {
List<TableIdentifier> tbls1 = catalog.listTables(Namespace.of("db"));
Set<String> tblSet = Sets.newHashSet(tbls1.stream().map(t -> t.name()).iterator());
- Assert.assertEquals(2, tblSet.size());
- Assert.assertTrue(tblSet.contains("tbl1"));
- Assert.assertTrue(tblSet.contains("tbl2"));
+ Assertions.assertThat(tblSet).hasSize(2).contains("tbl1").contains("tbl2");
List<TableIdentifier> tbls2 = catalog.listTables(Namespace.of("db", "ns1"));
- Assert.assertEquals("table identifiers", 1, tbls2.size());
- Assert.assertEquals("table name", "tbl3", tbls2.get(0).name());
+ Assertions.assertThat(tbls2).as("table identifiers").hasSize(1);
Review Comment:
Based on your review, I've removed unnecessary descriptions.
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238278926
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -105,23 +104,25 @@ public void testReplaceTxnBuilder() throws Exception {
createTxn.commitTransaction();
Table table = catalog.loadTable(tableIdent);
- Assert.assertNotNull(table.currentSnapshot());
+ Assertions.assertThat(table.currentSnapshot()).isNotNull();
Transaction replaceTxn =
catalog.buildTable(tableIdent, SCHEMA).withProperty("key2", "value2").replaceTransaction();
replaceTxn.commitTransaction();
table = catalog.loadTable(tableIdent);
- Assert.assertNull(table.currentSnapshot());
+ Assertions.assertThat(table.currentSnapshot()).isNull();
PartitionSpec v1Expected =
PartitionSpec.builderFor(table.schema())
.alwaysNull("data", "data_bucket")
.withSpecId(1)
.build();
- Assert.assertEquals("Table should have a spec with one void field", v1Expected, table.spec());
+ Assertions.assertThat(table.spec())
+ .as("Table should have a spec with one void field")
+ .isEqualTo(v1Expected);
- Assert.assertEquals("value1", table.properties().get("key1"));
- Assert.assertEquals("value2", table.properties().get("key2"));
+ Assertions.assertThat(table.properties().get("key1")).isEqualTo("value1");
Review Comment:
nit: better to use `.containsEntry(...).containsEntry(..)`
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -55,104 +53,103 @@ public class TestHadoopTables {
required(1, "id", Types.IntegerType.get(), "unique ID"),
required(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
- private File tableDir = null;
-
- @Before
- public void setupTableLocation() throws Exception {
- tableDir = temp.newFolder();
- }
+ @TempDir private Path tableDir;
Review Comment:
please change this to
```
@TempDir private File tableDir;
@TempDir private Path dataDir;
```
Then the `toUri()` changes aren't required
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -55,104 +53,103 @@ public class TestHadoopTables {
required(1, "id", Types.IntegerType.get(), "unique ID"),
required(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
- private File tableDir = null;
-
- @Before
- public void setupTableLocation() throws Exception {
- tableDir = temp.newFolder();
- }
+ @TempDir private Path tableDir;
@Test
public void testTableExists() {
- Assert.assertFalse(TABLES.exists(tableDir.toURI().toString()));
+ Assertions.assertThat(TABLES.exists(tableDir.toUri().toString())).isFalse();
PartitionSpec spec = PartitionSpec.builderFor(SCHEMA).bucket("data", 16).build();
- TABLES.create(SCHEMA, spec, tableDir.toURI().toString());
- Assert.assertTrue(TABLES.exists(tableDir.toURI().toString()));
+ TABLES.create(SCHEMA, spec, tableDir.toUri().toString());
+ Assertions.assertThat(TABLES.exists(tableDir.toUri().toString())).isTrue();
}
@Test
public void testDropTable() {
- TABLES.create(SCHEMA, tableDir.toURI().toString());
- TABLES.dropTable(tableDir.toURI().toString());
+ TABLES.create(SCHEMA, tableDir.toUri().toString());
+ TABLES.dropTable(tableDir.toUri().toString());
- Assertions.assertThatThrownBy(() -> TABLES.load(tableDir.toURI().toString()))
+ Assertions.assertThatThrownBy(() -> TABLES.load(tableDir.toUri().toString()))
.isInstanceOf(NoSuchTableException.class)
.hasMessageStartingWith("Table does not exist");
}
@Test
public void testDropTableWithPurge() throws IOException {
- File dataDir = temp.newFolder();
- createDummyTable(tableDir, dataDir);
+ createDummyTable(tableDir.toFile(), dataDir.toFile());
- TABLES.dropTable(tableDir.toURI().toString(), true);
- Assertions.assertThatThrownBy(() -> TABLES.load(tableDir.toURI().toString()))
+ TABLES.dropTable(tableDir.toUri().toString(), true);
+ Assertions.assertThatThrownBy(() -> TABLES.load(tableDir.toUri().toString()))
.isInstanceOf(NoSuchTableException.class)
.hasMessageStartingWith("Table does not exist");
- Assert.assertEquals(0, dataDir.listFiles().length);
- Assert.assertFalse(tableDir.exists());
+ Assertions.assertThat(Files.list(dataDir)).hasSize(0);
+ Assertions.assertThat(tableDir).doesNotExist();
- Assert.assertFalse(TABLES.dropTable(tableDir.toURI().toString()));
+ Assertions.assertThat(TABLES.dropTable(tableDir.toUri().toString())).isFalse();
}
+ @TempDir private Path dataDir;
Review Comment:
this should be moved above
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238260821
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -55,20 +54,19 @@ public class TestHadoopTables {
required(1, "id", Types.IntegerType.get(), "unique ID"),
required(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
private File tableDir = null;
Review Comment:
I have switched to @TempDir tableDir
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238761291
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -92,27 +83,23 @@ public void testDropTableWithPurge() throws IOException {
.isInstanceOf(NoSuchTableException.class)
.hasMessageStartingWith("Table does not exist");
- Assert.assertEquals(0, dataDir.listFiles().length);
- Assert.assertFalse(tableDir.exists());
-
- Assert.assertFalse(TABLES.dropTable(tableDir.toURI().toString()));
+ Assertions.assertThat(Files.list(dataDir.toPath()).count()).isEqualTo(0);
+ Assertions.assertThat(tableDir).doesNotExist();
+ Assertions.assertThat(TABLES.dropTable(tableDir.toURI().toString())).isFalse();
}
@Test
public void testDropTableWithoutPurge() throws IOException {
- File dataDir = temp.newFolder();
-
createDummyTable(tableDir, dataDir);
TABLES.dropTable(tableDir.toURI().toString(), false);
Assertions.assertThatThrownBy(() -> TABLES.load(tableDir.toURI().toString()))
.isInstanceOf(NoSuchTableException.class)
.hasMessageStartingWith("Table does not exist");
- Assert.assertEquals(1, dataDir.listFiles().length);
- Assert.assertFalse(tableDir.exists());
-
- Assert.assertFalse(TABLES.dropTable(tableDir.toURI().toString()));
+ Assertions.assertThat(Files.list(dataDir.toPath()).count()).isEqualTo(1);
Review Comment:
I've modified `Files.list(dataDir.toPath()).count()` to
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238678729
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -67,55 +64,59 @@ public class TestHadoopCommits extends HadoopTableTestBase {
public void testCreateTable() throws Exception {
PartitionSpec expectedSpec = PartitionSpec.builderFor(TABLE_SCHEMA).bucket("data", 16).build();
- Assert.assertEquals(
- "Table schema should match schema with reassigned ids",
- TABLE_SCHEMA.asStruct(),
- table.schema().asStruct());
- Assert.assertEquals(
- "Table partition spec should match with reassigned ids", expectedSpec, table.spec());
+ Assertions.assertThat(table.schema().asStruct())
+ .as("Table schema should match schema with reassigned ids")
+ .isEqualTo(TABLE_SCHEMA.asStruct());
+ Assertions.assertThat(table.spec())
+ .as("Table partition spec should match with reassigned ids")
+ .isEqualTo(expectedSpec);
List<FileScanTask> tasks = Lists.newArrayList(table.newScan().planFiles());
- Assert.assertEquals("Should not create any scan tasks", 0, tasks.size());
-
- Assert.assertTrue("Table location should exist", tableDir.exists());
- Assert.assertTrue(
- "Should create metadata folder", metadataDir.exists() && metadataDir.isDirectory());
- Assert.assertTrue("Should create v1 metadata", version(1).exists() && version(1).isFile());
- Assert.assertFalse("Should not create v2 or newer versions", version(2).exists());
- Assert.assertTrue("Should create version hint file", versionHintFile.exists());
- Assert.assertEquals("Should write the current version to the hint file", 1, readVersionHint());
-
+ Assertions.assertThat(tasks).as("Should not create any scan tasks").isEmpty();
+ Assertions.assertThat(tableDir).as("Table location should exist").exists();
+ Assertions.assertThat(metadataDir.exists() && metadataDir.isDirectory())
Review Comment:
Assertions.assertThat(metadataDir).exists().isDirectory()
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238679587
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -67,55 +64,59 @@ public class TestHadoopCommits extends HadoopTableTestBase {
public void testCreateTable() throws Exception {
PartitionSpec expectedSpec = PartitionSpec.builderFor(TABLE_SCHEMA).bucket("data", 16).build();
- Assert.assertEquals(
- "Table schema should match schema with reassigned ids",
- TABLE_SCHEMA.asStruct(),
- table.schema().asStruct());
- Assert.assertEquals(
- "Table partition spec should match with reassigned ids", expectedSpec, table.spec());
+ Assertions.assertThat(table.schema().asStruct())
+ .as("Table schema should match schema with reassigned ids")
+ .isEqualTo(TABLE_SCHEMA.asStruct());
+ Assertions.assertThat(table.spec())
+ .as("Table partition spec should match with reassigned ids")
+ .isEqualTo(expectedSpec);
List<FileScanTask> tasks = Lists.newArrayList(table.newScan().planFiles());
- Assert.assertEquals("Should not create any scan tasks", 0, tasks.size());
-
- Assert.assertTrue("Table location should exist", tableDir.exists());
- Assert.assertTrue(
- "Should create metadata folder", metadataDir.exists() && metadataDir.isDirectory());
- Assert.assertTrue("Should create v1 metadata", version(1).exists() && version(1).isFile());
- Assert.assertFalse("Should not create v2 or newer versions", version(2).exists());
- Assert.assertTrue("Should create version hint file", versionHintFile.exists());
- Assert.assertEquals("Should write the current version to the hint file", 1, readVersionHint());
-
+ Assertions.assertThat(tasks).as("Should not create any scan tasks").isEmpty();
+ Assertions.assertThat(tableDir).as("Table location should exist").exists();
+ Assertions.assertThat(metadataDir.exists() && metadataDir.isDirectory())
+ .as("Should create metadata folder")
+ .isTrue();
+ Assertions.assertThat(version(1).exists() && version(1).isFile())
+ .as("Should create v1 metadata")
+ .isTrue();
+ Assertions.assertThat(version(2).exists())
Review Comment:
```suggestion
Assertions.assertThat(version(2)).exists()
```
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238676367
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -530,13 +533,14 @@ public void testTableName() throws Exception {
catalog.buildTable(tableIdent, SCHEMA).withPartitionSpec(SPEC).create();
Table table = catalog.loadTable(tableIdent);
- Assert.assertEquals("Name must match", "hadoop.db.ns1.ns2.tbl", table.name());
+ Assertions.assertThat(table.name()).as("Name must match").isEqualTo("hadoop.db.ns1.ns2.tbl");
Review Comment:
nit: `.as("Name must match")` is meaningless here and further below, so 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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238018928
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -55,20 +54,19 @@ public class TestHadoopTables {
required(1, "id", Types.IntegerType.get(), "unique ID"),
required(2, "data", Types.StringType.get()));
- @Rule public TemporaryFolder temp = new TemporaryFolder();
private File tableDir = null;
- @Before
- public void setupTableLocation() throws Exception {
- tableDir = temp.newFolder();
+ @BeforeEach
+ public void setupTableLocation(@TempDir Path temp) throws Exception {
Review Comment:
the entire `setupTableLocation()` can be removed I believe
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -92,15 +90,15 @@ public void testDropTableWithPurge() throws IOException {
.isInstanceOf(NoSuchTableException.class)
.hasMessageStartingWith("Table does not exist");
- Assert.assertEquals(0, dataDir.listFiles().length);
- Assert.assertFalse(tableDir.exists());
+ Assertions.assertThat(dataDir.listFiles()).hasSize(0);
+ Assertions.assertThat(tableDir).doesNotExist();
- Assert.assertFalse(TABLES.dropTable(tableDir.toURI().toString()));
+ Assertions.assertThat(TABLES.dropTable(tableDir.toURI().toString())).isFalse();
}
@Test
- public void testDropTableWithoutPurge() throws IOException {
- File dataDir = temp.newFolder();
+ public void testDropTableWithoutPurge(@TempDir Path temp) throws IOException {
Review Comment:
why do we need the `@TempDir Path temp` parameter here? We should be able to use `tableDir`
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238732362
##########
core/src/test/java/org/apache/iceberg/hadoop/HadoopFileIOTest.java:
##########
@@ -111,16 +110,16 @@ public void testDeletePrefix() {
hadoopFileIO.deletePrefix(scalePath.toUri().toString());
// Hadoop filesystem will throw if the path does not exist
- assertThrows(
- UncheckedIOException.class,
- () -> hadoopFileIO.listPrefix(scalePath.toUri().toString()).iterator());
+ Assertions.assertThatThrownBy(
+ () -> hadoopFileIO.listPrefix(scalePath.toUri().toString()).iterator())
+ .isInstanceOf(UncheckedIOException.class);
Review Comment:
Based on your review, I added the `.hasMessage(...)` in `Assertions.assertThatThrownBy` .
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238672434
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -69,10 +68,10 @@ public void testCreateTableBuilder() throws Exception {
.withProperties(ImmutableMap.of("key2", "value2"))
.create();
- Assert.assertEquals(TABLE_SCHEMA.toString(), table.schema().toString());
- Assert.assertEquals(1, table.spec().fields().size());
- Assert.assertEquals("value1", table.properties().get("key1"));
- Assert.assertEquals("value2", table.properties().get("key2"));
+ Assertions.assertThat(table.schema().toString()).isEqualTo(TABLE_SCHEMA.toString());
+ Assertions.assertThat(table.spec().fields()).hasSize(1);
+ Assertions.assertThat(table.properties()).containsEntry("key1", "value1");
+ Assertions.assertThat(table.properties()).containsEntry("key2", "value2");
Review Comment:
can be chained together
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238671605
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -83,36 +76,32 @@ public void testDropTable() {
@Test
public void testDropTableWithPurge() throws IOException {
- File dataDir = temp.newFolder();
- createDummyTable(tableDir, dataDir);
+ createDummyTable(tableDir, dataDir.toFile());
Review Comment:
shall we change dataDir to `File` instead of `Path` so that we don't have to do Path->File conversion?
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238665419
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -69,10 +68,10 @@ public void testCreateTableBuilder() throws Exception {
.withProperties(ImmutableMap.of("key2", "value2"))
.create();
- Assert.assertEquals(TABLE_SCHEMA.toString(), table.schema().toString());
- Assert.assertEquals(1, table.spec().fields().size());
- Assert.assertEquals("value1", table.properties().get("key1"));
- Assert.assertEquals("value2", table.properties().get("key2"));
+ Assertions.assertThat(table.schema().toString()).isEqualTo(TABLE_SCHEMA.toString());
+ Assertions.assertThat(table.spec().fields()).hasSize(1);
+ Assertions.assertThat(table.properties().get("key1")).isEqualTo("value1");
Review Comment:
I double-checked and changed everything that could be changed to .containsEntry.
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238736242
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopTables.java:
##########
@@ -83,36 +76,32 @@ public void testDropTable() {
@Test
public void testDropTableWithPurge() throws IOException {
- File dataDir = temp.newFolder();
- createDummyTable(tableDir, dataDir);
+ createDummyTable(tableDir, dataDir.toFile());
Review Comment:
I've changed dataDir to File instead of Path
##########
core/src/test/java/org/apache/iceberg/hadoop/HadoopFileIOTest.java:
##########
@@ -111,16 +110,16 @@ public void testDeletePrefix() {
hadoopFileIO.deletePrefix(scalePath.toUri().toString());
// Hadoop filesystem will throw if the path does not exist
- assertThrows(
- UncheckedIOException.class,
- () -> hadoopFileIO.listPrefix(scalePath.toUri().toString()).iterator());
+ Assertions.assertThatThrownBy(
+ () -> hadoopFileIO.listPrefix(scalePath.toUri().toString()).iterator())
+ .isInstanceOf(UncheckedIOException.class);
Review Comment:
Based on your review, I've added the `.hasMessage(...)` in `Assertions.assertThatThrownBy` .
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238732362
##########
core/src/test/java/org/apache/iceberg/hadoop/HadoopFileIOTest.java:
##########
@@ -111,16 +110,16 @@ public void testDeletePrefix() {
hadoopFileIO.deletePrefix(scalePath.toUri().toString());
// Hadoop filesystem will throw if the path does not exist
- assertThrows(
- UncheckedIOException.class,
- () -> hadoopFileIO.listPrefix(scalePath.toUri().toString()).iterator());
+ Assertions.assertThatThrownBy(
+ () -> hadoopFileIO.listPrefix(scalePath.toUri().toString()).iterator())
+ .isInstanceOf(UncheckedIOException.class);
Review Comment:
Based on your review, I added the hasMessage in `Assertions.assertThatThrownBy` .
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238746729
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCommits.java:
##########
@@ -67,55 +64,59 @@ public class TestHadoopCommits extends HadoopTableTestBase {
public void testCreateTable() throws Exception {
PartitionSpec expectedSpec = PartitionSpec.builderFor(TABLE_SCHEMA).bucket("data", 16).build();
- Assert.assertEquals(
- "Table schema should match schema with reassigned ids",
- TABLE_SCHEMA.asStruct(),
- table.schema().asStruct());
- Assert.assertEquals(
- "Table partition spec should match with reassigned ids", expectedSpec, table.spec());
+ Assertions.assertThat(table.schema().asStruct())
+ .as("Table schema should match schema with reassigned ids")
+ .isEqualTo(TABLE_SCHEMA.asStruct());
+ Assertions.assertThat(table.spec())
+ .as("Table partition spec should match with reassigned ids")
+ .isEqualTo(expectedSpec);
List<FileScanTask> tasks = Lists.newArrayList(table.newScan().planFiles());
- Assert.assertEquals("Should not create any scan tasks", 0, tasks.size());
-
- Assert.assertTrue("Table location should exist", tableDir.exists());
- Assert.assertTrue(
- "Should create metadata folder", metadataDir.exists() && metadataDir.isDirectory());
- Assert.assertTrue("Should create v1 metadata", version(1).exists() && version(1).isFile());
- Assert.assertFalse("Should not create v2 or newer versions", version(2).exists());
- Assert.assertTrue("Should create version hint file", versionHintFile.exists());
- Assert.assertEquals("Should write the current version to the hint file", 1, readVersionHint());
-
+ Assertions.assertThat(tasks).as("Should not create any scan tasks").isEmpty();
+ Assertions.assertThat(tableDir).as("Table location should exist").exists();
+ Assertions.assertThat(metadataDir.exists() && metadataDir.isDirectory())
Review Comment:
I've modified the code based on your review in this PR
--
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
[GitHub] [iceberg] nastra merged pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #7861:
URL: https://github.com/apache/iceberg/pull/7861
--
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
[GitHub] [iceberg] skytin1004 commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238253720
##########
core/src/test/java/org/apache/iceberg/hadoop/HadoopTableTestBase.java:
##########
@@ -113,17 +113,17 @@ public class HadoopTableTestBase {
.withRecordCount(2) // needs at least one record or else metrics will filter it out
.build();
- @Rule public TemporaryFolder temp = new TemporaryFolder();
+ @TempDir Path temp;
Review Comment:
Thank you for the suggestion. I've modified the code based on your suggestions.
--
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
[GitHub] [iceberg] nastra commented on a diff in pull request #7861: Core: Switch tests to Junit5 in rest, hadoop pakages
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7861:
URL: https://github.com/apache/iceberg/pull/7861#discussion_r1238478352
##########
core/src/test/java/org/apache/iceberg/hadoop/TestHadoopCatalog.java:
##########
@@ -69,10 +68,10 @@ public void testCreateTableBuilder() throws Exception {
.withProperties(ImmutableMap.of("key2", "value2"))
.create();
- Assert.assertEquals(TABLE_SCHEMA.toString(), table.schema().toString());
- Assert.assertEquals(1, table.spec().fields().size());
- Assert.assertEquals("value1", table.properties().get("key1"));
- Assert.assertEquals("value2", table.properties().get("key2"));
+ Assertions.assertThat(table.schema().toString()).isEqualTo(TABLE_SCHEMA.toString());
+ Assertions.assertThat(table.spec().fields()).hasSize(1);
+ Assertions.assertThat(table.properties().get("key1")).isEqualTo("value1");
Review Comment:
those should all be `.containsEntry` (please also double-check other places that are being modified as part of this PR so that we use the same mechanism everywhere)
--
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