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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
