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/04 14:16:31 UTC
[GitHub] [iceberg] skytin1004 opened a new pull request, #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
skytin1004 opened a new pull request, #7767:
URL: https://github.com/apache/iceberg/pull/7767
I switched the files in this [catalog,encryption,inmemory,io,view] pakages 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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1216761745
##########
core/src/test/java/org/apache/iceberg/io/TestByteBufferInputStreams.java:
##########
@@ -55,18 +56,22 @@ public void testReadAll() throws Exception {
ByteBufferInputStream stream = newStream();
int bytesRead = stream.read(bytes);
- Assert.assertEquals("Should read the entire buffer", bytes.length, bytesRead);
+ Assertions.assertThat(bytesRead).as("Should read the entire buffer").isEqualTo(bytes.length);
for (int i = 0; i < bytes.length; i += 1) {
- Assert.assertEquals("Byte i should be i", i, bytes[i]);
- Assert.assertEquals("Should advance position", 35, stream.getPos());
+ Assertions.assertThat(bytes[i]).as("Byte i should be i").isEqualTo((byte) i);
Review Comment:
This casting`(byte) i` is required. This is because in JUnit 4, the `assertEquals` method performs automatic type conversion when comparing values of different types. But in AssertJ, the `assertThat` method does not perform automatic type 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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1216761745
##########
core/src/test/java/org/apache/iceberg/io/TestByteBufferInputStreams.java:
##########
@@ -55,18 +56,22 @@ public void testReadAll() throws Exception {
ByteBufferInputStream stream = newStream();
int bytesRead = stream.read(bytes);
- Assert.assertEquals("Should read the entire buffer", bytes.length, bytesRead);
+ Assertions.assertThat(bytesRead).as("Should read the entire buffer").isEqualTo(bytes.length);
for (int i = 0; i < bytes.length; i += 1) {
- Assert.assertEquals("Byte i should be i", i, bytes[i]);
- Assert.assertEquals("Should advance position", 35, stream.getPos());
+ Assertions.assertThat(bytes[i]).as("Byte i should be i").isEqualTo((byte) i);
Review Comment:
I think this casting`(byte) i` is required. This is because in JUnit 4, the `assertEquals` method performs automatic type conversion when comparing values of different types. But in AssertJ, the `assertThat` method does not perform automatic type 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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1219400741
##########
core/src/test/java/org/apache/iceberg/io/TestSingleBufferInputStream.java:
##########
@@ -40,8 +41,9 @@ protected ByteBufferInputStream newStream() {
@Override
protected void checkOriginalData() {
- Assert.assertEquals("Position should not change", 0, DATA.position());
- Assert.assertEquals("Limit should not change", DATA.array().length, DATA.limit());
+ assertThat(DATA.position()).as("Position should not change").isEqualTo(0);
+
Review Comment:
I deleted the unnecessary newline.
##########
core/src/test/java/org/apache/iceberg/io/TestMultiBufferInputStream.java:
##########
@@ -62,62 +63,66 @@ public void testSliceData() throws Exception {
buffers.add(stream.slice(bytesToSlice));
}
- Assert.assertEquals("Position should be at end", length, stream.getPos());
- Assert.assertEquals("Should produce 5 buffers", 5, buffers.size());
+ assertThat(stream.getPos()).as("Position should be at end").isEqualTo(length);
+ assertThat(buffers.size()).as("Should produce 5 buffers").isEqualTo(5);
int i = 0;
// one is a view of the first buffer because it is smaller
ByteBuffer one = buffers.get(0);
- Assert.assertSame("Should be a duplicate of the first array", one.array(), DATA.get(0).array());
- Assert.assertEquals(8, one.remaining());
- Assert.assertEquals(0, one.position());
- Assert.assertEquals(8, one.limit());
- Assert.assertEquals(9, one.capacity());
+ assertThat(DATA.get(0).array())
+ .as("Should be a duplicate of the first array")
+ .isSameAs(one.array());
+ assertThat(one.remaining()).isEqualTo(8);
+ assertThat(one.position()).isEqualTo(0);
+ assertThat(one.limit()).isEqualTo(8);
+ assertThat(one.capacity()).isEqualTo(9);
for (; i < 8; i += 1) {
- Assert.assertEquals("Should produce correct values", i, one.get());
+ assertThat(one.get()).as("Should produce correct values").isEqualTo((byte) i);
}
// two should be a copy of the next 8 bytes
ByteBuffer two = buffers.get(1);
- Assert.assertEquals(8, two.remaining());
- Assert.assertEquals(0, two.position());
- Assert.assertEquals(8, two.limit());
- Assert.assertEquals(8, two.capacity());
+ assertThat(two.remaining()).isEqualTo(8);
+ assertThat(two.position()).isEqualTo(0);
+ assertThat(two.limit()).isEqualTo(8);
+ assertThat(two.capacity()).isEqualTo(8);
for (; i < 16; i += 1) {
- Assert.assertEquals("Should produce correct values", i, two.get());
+ assertThat(two.get()).as("Should produce correct values").isEqualTo((byte) i);
}
// three is a copy of part of the 4th buffer
ByteBuffer three = buffers.get(2);
- Assert.assertSame(
- "Should be a duplicate of the fourth array", three.array(), DATA.get(3).array());
- Assert.assertEquals(8, three.remaining());
- Assert.assertEquals(3, three.position());
- Assert.assertEquals(11, three.limit());
- Assert.assertEquals(12, three.capacity());
+ assertThat(DATA.get(3).array())
+ .as("Should be a duplicate of the fourth array")
+ .isSameAs(three.array());
+ assertThat(three.remaining()).isEqualTo(8);
+ assertThat(three.position()).isEqualTo(3);
+ assertThat(three.limit()).isEqualTo(11);
+ assertThat(three.capacity()).isEqualTo(12);
for (; i < 24; i += 1) {
- Assert.assertEquals("Should produce correct values", i, three.get());
+ assertThat(three.get()).as("Should produce correct values").isEqualTo((byte) i);
}
// four should be a copy of the next 8 bytes
ByteBuffer four = buffers.get(3);
- Assert.assertEquals(8, four.remaining());
- Assert.assertEquals(0, four.position());
- Assert.assertEquals(8, four.limit());
- Assert.assertEquals(8, four.capacity());
+ assertThat(four.remaining()).isEqualTo(8);
+ assertThat(four.position()).isEqualTo(0);
+ assertThat(four.limit()).isEqualTo(8);
+ assertThat(four.capacity()).isEqualTo(8);
for (; i < 32; i += 1) {
- Assert.assertEquals("Should produce correct values", i, four.get());
+ assertThat(four.get()).as("Should produce correct values").isEqualTo((byte) i);
}
// five should be a copy of the next 8 bytes
ByteBuffer five = buffers.get(4);
- Assert.assertEquals(3, five.remaining());
- Assert.assertEquals(0, five.position());
- Assert.assertEquals(3, five.limit());
- Assert.assertEquals(3, five.capacity());
+
Review Comment:
I deleted the unnecessary newline.
--
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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#issuecomment-1584307611
Thanks for your review @nastra! I have reviewed all the code in this PR and changed the code to use the `.containsAll` method.
In some parts of the code, I changed the code to use `the containsExactlyInAnyOrderElementsOf` method.
--
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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#issuecomment-1579741659
I should have done gitpull in local after merging with Apache master branch, but I skipped the gil pull.
After that I deleted unnecessary newlines and force push.
So I Merge the 'apache:master' branch into test2.
--
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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1216761745
##########
core/src/test/java/org/apache/iceberg/io/TestByteBufferInputStreams.java:
##########
@@ -55,18 +56,22 @@ public void testReadAll() throws Exception {
ByteBufferInputStream stream = newStream();
int bytesRead = stream.read(bytes);
- Assert.assertEquals("Should read the entire buffer", bytes.length, bytesRead);
+ Assertions.assertThat(bytesRead).as("Should read the entire buffer").isEqualTo(bytes.length);
for (int i = 0; i < bytes.length; i += 1) {
- Assert.assertEquals("Byte i should be i", i, bytes[i]);
- Assert.assertEquals("Should advance position", 35, stream.getPos());
+ Assertions.assertThat(bytes[i]).as("Byte i should be i").isEqualTo((byte) i);
Review Comment:
This casting[(byte) i] is required. This is because in JUnit 4, the `assertEquals` method performs automatic type conversion when comparing values of different types. But in AssertJ, the `assertThat` method does not perform automatic type 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] nastra commented on a diff in pull request #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1219143741
##########
core/src/test/java/org/apache/iceberg/io/TestMultiBufferInputStream.java:
##########
@@ -62,62 +63,66 @@ public void testSliceData() throws Exception {
buffers.add(stream.slice(bytesToSlice));
}
- Assert.assertEquals("Position should be at end", length, stream.getPos());
- Assert.assertEquals("Should produce 5 buffers", 5, buffers.size());
+ assertThat(stream.getPos()).as("Position should be at end").isEqualTo(length);
+ assertThat(buffers.size()).as("Should produce 5 buffers").isEqualTo(5);
int i = 0;
// one is a view of the first buffer because it is smaller
ByteBuffer one = buffers.get(0);
- Assert.assertSame("Should be a duplicate of the first array", one.array(), DATA.get(0).array());
- Assert.assertEquals(8, one.remaining());
- Assert.assertEquals(0, one.position());
- Assert.assertEquals(8, one.limit());
- Assert.assertEquals(9, one.capacity());
+ assertThat(DATA.get(0).array())
+ .as("Should be a duplicate of the first array")
+ .isSameAs(one.array());
+ assertThat(one.remaining()).isEqualTo(8);
+ assertThat(one.position()).isEqualTo(0);
+ assertThat(one.limit()).isEqualTo(8);
+ assertThat(one.capacity()).isEqualTo(9);
for (; i < 8; i += 1) {
- Assert.assertEquals("Should produce correct values", i, one.get());
+ assertThat(one.get()).as("Should produce correct values").isEqualTo((byte) i);
}
// two should be a copy of the next 8 bytes
ByteBuffer two = buffers.get(1);
- Assert.assertEquals(8, two.remaining());
- Assert.assertEquals(0, two.position());
- Assert.assertEquals(8, two.limit());
- Assert.assertEquals(8, two.capacity());
+ assertThat(two.remaining()).isEqualTo(8);
+ assertThat(two.position()).isEqualTo(0);
+ assertThat(two.limit()).isEqualTo(8);
+ assertThat(two.capacity()).isEqualTo(8);
for (; i < 16; i += 1) {
- Assert.assertEquals("Should produce correct values", i, two.get());
+ assertThat(two.get()).as("Should produce correct values").isEqualTo((byte) i);
}
// three is a copy of part of the 4th buffer
ByteBuffer three = buffers.get(2);
- Assert.assertSame(
- "Should be a duplicate of the fourth array", three.array(), DATA.get(3).array());
- Assert.assertEquals(8, three.remaining());
- Assert.assertEquals(3, three.position());
- Assert.assertEquals(11, three.limit());
- Assert.assertEquals(12, three.capacity());
+ assertThat(DATA.get(3).array())
+ .as("Should be a duplicate of the fourth array")
+ .isSameAs(three.array());
+ assertThat(three.remaining()).isEqualTo(8);
+ assertThat(three.position()).isEqualTo(3);
+ assertThat(three.limit()).isEqualTo(11);
+ assertThat(three.capacity()).isEqualTo(12);
for (; i < 24; i += 1) {
- Assert.assertEquals("Should produce correct values", i, three.get());
+ assertThat(three.get()).as("Should produce correct values").isEqualTo((byte) i);
}
// four should be a copy of the next 8 bytes
ByteBuffer four = buffers.get(3);
- Assert.assertEquals(8, four.remaining());
- Assert.assertEquals(0, four.position());
- Assert.assertEquals(8, four.limit());
- Assert.assertEquals(8, four.capacity());
+ assertThat(four.remaining()).isEqualTo(8);
+ assertThat(four.position()).isEqualTo(0);
+ assertThat(four.limit()).isEqualTo(8);
+ assertThat(four.capacity()).isEqualTo(8);
for (; i < 32; i += 1) {
- Assert.assertEquals("Should produce correct values", i, four.get());
+ assertThat(four.get()).as("Should produce correct values").isEqualTo((byte) i);
}
// five should be a copy of the next 8 bytes
ByteBuffer five = buffers.get(4);
- Assert.assertEquals(3, five.remaining());
- Assert.assertEquals(0, five.position());
- Assert.assertEquals(3, five.limit());
- Assert.assertEquals(3, five.capacity());
+
Review Comment:
nit: unnecessary newline
##########
core/src/test/java/org/apache/iceberg/inmemory/TestInMemoryInputFile.java:
##########
@@ -22,16 +22,17 @@
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class TestInMemoryInputFile {
@Test
public void testReadAfterClose() throws IOException {
InMemoryInputFile inputFile =
new InMemoryInputFile("abc".getBytes(StandardCharsets.ISO_8859_1));
InputStream inputStream = inputFile.newStream();
- Assert.assertEquals('a', inputStream.read());
+ Assertions.assertThat(inputStream.read())
+ .as("Checking read value from inputStream")
Review Comment:
do we really need this description?
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -168,70 +166,71 @@ protected boolean supportsNamesWithSlashes() {
public void testCreateNamespace() {
C catalog = catalog();
- Assert.assertFalse("Namespace should not exist", catalog.namespaceExists(NS));
+ Assertions.assertThat(catalog.namespaceExists(NS)).as("Namespace should not exist").isFalse();
catalog.createNamespace(NS);
- Assert.assertTrue(
- "Catalog should have the created namespace", catalog.listNamespaces().contains(NS));
- Assert.assertTrue("Namespace should exist", catalog.namespaceExists(NS));
+ Assertions.assertThat(catalog.listNamespaces())
+ .as("Catalog should have the created namespace")
+ .contains(NS);
+ Assertions.assertThat(catalog.namespaceExists(NS)).as("Namespace should exist").isTrue();
}
@Test
public void testCreateExistingNamespace() {
C catalog = catalog();
- Assert.assertFalse("Namespace should not exist", catalog.namespaceExists(NS));
+ Assertions.assertThat(catalog.namespaceExists(NS)).as("Namespace should not exist").isFalse();
catalog.createNamespace(NS);
- Assert.assertTrue("Namespace should exist", catalog.namespaceExists(NS));
+ Assertions.assertThat(catalog.namespaceExists(NS)).as("Namespace should exist").isTrue();
Assertions.assertThatThrownBy(() -> catalog.createNamespace(NS))
.isInstanceOf(AlreadyExistsException.class)
.hasMessageContaining("Namespace already exists");
- Assert.assertTrue("Namespace should still exist", catalog.namespaceExists(NS));
+ Assertions.assertThat(catalog.namespaceExists(NS)).as("Namespace should still exist").isTrue();
}
@Test
public void testCreateNamespaceWithProperties() {
- Assume.assumeTrue(supportsNamespaceProperties());
+ Assumptions.assumeTrue(supportsNamespaceProperties());
C catalog = catalog();
- Assert.assertFalse("Namespace should not exist", catalog.namespaceExists(NS));
+ Assertions.assertThat(catalog.namespaceExists(NS)).as("Namespace should not exist").isFalse();
Map<String, String> createProps = ImmutableMap.of("prop", "val");
catalog.createNamespace(NS, createProps);
- Assert.assertTrue("Namespace should exist", catalog.namespaceExists(NS));
+
Review Comment:
unnecessary newline
##########
core/src/test/java/org/apache/iceberg/io/TestByteBufferInputStreams.java:
##########
@@ -506,7 +562,8 @@ public void testMarkDoubleReset() throws Exception {
ByteBufferInputStream stream = newStream();
stream.mark(5);
- Assert.assertEquals("Should read 5 bytes", 5, stream.read(new byte[5]));
+
Review Comment:
unnecessary newline
##########
core/src/test/java/org/apache/iceberg/io/TestSingleBufferInputStream.java:
##########
@@ -40,8 +41,9 @@ protected ByteBufferInputStream newStream() {
@Override
protected void checkOriginalData() {
- Assert.assertEquals("Position should not change", 0, DATA.position());
- Assert.assertEquals("Limit should not change", DATA.array().length, DATA.limit());
+ assertThat(DATA.position()).as("Position should not change").isEqualTo(0);
+
Review Comment:
nit: unnecessary newline
--
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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1230580254
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -614,20 +621,23 @@ public void testCompleteCreateTable() {
.create();
// validate table settings
- Assert.assertEquals(
- "Table name should report its full name", catalog.name() + "." + ident, table.name());
- Assert.assertTrue("Table should exist", catalog.tableExists(ident));
- Assert.assertEquals(
- "Schema should match expected ID assignment",
- TABLE_SCHEMA.asStruct(),
- table.schema().asStruct());
- Assert.assertNotNull("Should have a location", table.location());
- Assert.assertEquals("Should use requested partition spec", TABLE_SPEC, table.spec());
- Assert.assertEquals("Should use requested write order", TABLE_WRITE_ORDER, table.sortOrder());
- Assert.assertEquals(
- "Table properties should be a superset of the requested properties",
- properties.entrySet(),
- Sets.intersection(properties.entrySet(), table.properties().entrySet()));
+ Assertions.assertThat(table.name())
+ .as("Table name should report its full name")
+ .isEqualTo(catalog.name() + "." + ident);
+ Assertions.assertThat(catalog.tableExists(ident)).as("Table should exist").isTrue();
+ Assertions.assertThat(table.schema().asStruct())
+ .as("Schema should match expected ID assignment")
+ .isEqualTo(TABLE_SCHEMA.asStruct());
+ Assertions.assertThat(table.location()).as("Should have a location").isNotNull();
+ Assertions.assertThat(table.spec())
+ .as("Should use requested partition spec")
+ .isEqualTo(TABLE_SPEC);
+ Assertions.assertThat(table.sortOrder())
+ .as("Should use requested write order")
+ .isEqualTo(TABLE_WRITE_ORDER);
+ Assertions.assertThat(table.properties().entrySet())
+ .as("Table properties should be a superset of the requested properties")
+ .containsAll(properties.entrySet());
Review Comment:
@skytin1004 this still uses `containsAll` rather than `containsExactlyInAnyOrder`. We want to make sure that `table.properties()` doesn't contain any more elements, hence the `containsExactlyInAnyOrder` check
--
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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #7767:
URL: https://github.com/apache/iceberg/pull/7767
--
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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1223970174
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -614,20 +621,23 @@ public void testCompleteCreateTable() {
.create();
// validate table settings
- Assert.assertEquals(
- "Table name should report its full name", catalog.name() + "." + ident, table.name());
- Assert.assertTrue("Table should exist", catalog.tableExists(ident));
- Assert.assertEquals(
- "Schema should match expected ID assignment",
- TABLE_SCHEMA.asStruct(),
- table.schema().asStruct());
- Assert.assertNotNull("Should have a location", table.location());
- Assert.assertEquals("Should use requested partition spec", TABLE_SPEC, table.spec());
- Assert.assertEquals("Should use requested write order", TABLE_WRITE_ORDER, table.sortOrder());
- Assert.assertEquals(
- "Table properties should be a superset of the requested properties",
- properties.entrySet(),
- Sets.intersection(properties.entrySet(), table.properties().entrySet()));
+ Assertions.assertThat(table.name())
+ .as("Table name should report its full name")
+ .isEqualTo(catalog.name() + "." + ident);
+ Assertions.assertThat(catalog.tableExists(ident)).as("Table should exist").isTrue();
+ Assertions.assertThat(table.schema().asStruct())
+ .as("Schema should match expected ID assignment")
+ .isEqualTo(TABLE_SCHEMA.asStruct());
+ Assertions.assertThat(table.location()).as("Should have a location").isNotNull();
+ Assertions.assertThat(table.spec())
+ .as("Should use requested partition spec")
+ .isEqualTo(TABLE_SPEC);
+ Assertions.assertThat(table.sortOrder())
+ .as("Should use requested write order")
+ .isEqualTo(TABLE_WRITE_ORDER);
+ Assertions.assertThat(table.properties().entrySet())
+ .as("Table properties should be a superset of the requested properties")
+ .containsAll(properties.entrySet());
Review Comment:
@skytin1004 can you make that change please so that we can get this in?
--
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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1219395414
##########
core/src/test/java/org/apache/iceberg/inmemory/TestInMemoryInputFile.java:
##########
@@ -22,16 +22,17 @@
import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import org.assertj.core.api.Assertions;
-import org.junit.Assert;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class TestInMemoryInputFile {
@Test
public void testReadAfterClose() throws IOException {
InMemoryInputFile inputFile =
new InMemoryInputFile("abc".getBytes(StandardCharsets.ISO_8859_1));
InputStream inputStream = inputFile.newStream();
- Assert.assertEquals('a', inputStream.read());
+ Assertions.assertThat(inputStream.read())
+ .as("Checking read value from inputStream")
Review Comment:
I think it's an unnecessary description.
I deleted unnecessary description.
--
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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#issuecomment-1578519269
@nastra Thank you for your review.
Based on your review, I deleted unnecessary newlines and description.
--
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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1219413775
##########
core/src/test/java/org/apache/iceberg/io/TestByteBufferInputStreams.java:
##########
@@ -506,7 +562,8 @@ public void testMarkDoubleReset() throws Exception {
ByteBufferInputStream stream = newStream();
stream.mark(5);
- Assert.assertEquals("Should read 5 bytes", 5, stream.read(new byte[5]));
+
Review Comment:
I deleted the unnecessary newline.
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -168,70 +166,71 @@ protected boolean supportsNamesWithSlashes() {
public void testCreateNamespace() {
C catalog = catalog();
- Assert.assertFalse("Namespace should not exist", catalog.namespaceExists(NS));
+ Assertions.assertThat(catalog.namespaceExists(NS)).as("Namespace should not exist").isFalse();
catalog.createNamespace(NS);
- Assert.assertTrue(
- "Catalog should have the created namespace", catalog.listNamespaces().contains(NS));
- Assert.assertTrue("Namespace should exist", catalog.namespaceExists(NS));
+ Assertions.assertThat(catalog.listNamespaces())
+ .as("Catalog should have the created namespace")
+ .contains(NS);
+ Assertions.assertThat(catalog.namespaceExists(NS)).as("Namespace should exist").isTrue();
}
@Test
public void testCreateExistingNamespace() {
C catalog = catalog();
- Assert.assertFalse("Namespace should not exist", catalog.namespaceExists(NS));
+ Assertions.assertThat(catalog.namespaceExists(NS)).as("Namespace should not exist").isFalse();
catalog.createNamespace(NS);
- Assert.assertTrue("Namespace should exist", catalog.namespaceExists(NS));
+ Assertions.assertThat(catalog.namespaceExists(NS)).as("Namespace should exist").isTrue();
Assertions.assertThatThrownBy(() -> catalog.createNamespace(NS))
.isInstanceOf(AlreadyExistsException.class)
.hasMessageContaining("Namespace already exists");
- Assert.assertTrue("Namespace should still exist", catalog.namespaceExists(NS));
+ Assertions.assertThat(catalog.namespaceExists(NS)).as("Namespace should still exist").isTrue();
}
@Test
public void testCreateNamespaceWithProperties() {
- Assume.assumeTrue(supportsNamespaceProperties());
+ Assumptions.assumeTrue(supportsNamespaceProperties());
C catalog = catalog();
- Assert.assertFalse("Namespace should not exist", catalog.namespaceExists(NS));
+ Assertions.assertThat(catalog.namespaceExists(NS)).as("Namespace should not exist").isFalse();
Map<String, String> createProps = ImmutableMap.of("prop", "val");
catalog.createNamespace(NS, createProps);
- Assert.assertTrue("Namespace should exist", catalog.namespaceExists(NS));
+
Review Comment:
I deleted the unnecessary newline.
--
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 closed pull request #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 closed pull request #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
URL: https://github.com/apache/iceberg/pull/7767
--
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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1219465336
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -614,20 +621,23 @@ public void testCompleteCreateTable() {
.create();
// validate table settings
- Assert.assertEquals(
- "Table name should report its full name", catalog.name() + "." + ident, table.name());
- Assert.assertTrue("Table should exist", catalog.tableExists(ident));
- Assert.assertEquals(
- "Schema should match expected ID assignment",
- TABLE_SCHEMA.asStruct(),
- table.schema().asStruct());
- Assert.assertNotNull("Should have a location", table.location());
- Assert.assertEquals("Should use requested partition spec", TABLE_SPEC, table.spec());
- Assert.assertEquals("Should use requested write order", TABLE_WRITE_ORDER, table.sortOrder());
- Assert.assertEquals(
- "Table properties should be a superset of the requested properties",
- properties.entrySet(),
- Sets.intersection(properties.entrySet(), table.properties().entrySet()));
+ Assertions.assertThat(table.name())
+ .as("Table name should report its full name")
+ .isEqualTo(catalog.name() + "." + ident);
+ Assertions.assertThat(catalog.tableExists(ident)).as("Table should exist").isTrue();
+ Assertions.assertThat(table.schema().asStruct())
+ .as("Schema should match expected ID assignment")
+ .isEqualTo(TABLE_SCHEMA.asStruct());
+ Assertions.assertThat(table.location()).as("Should have a location").isNotNull();
+ Assertions.assertThat(table.spec())
+ .as("Should use requested partition spec")
+ .isEqualTo(TABLE_SPEC);
+ Assertions.assertThat(table.sortOrder())
+ .as("Should use requested write order")
+ .isEqualTo(TABLE_WRITE_ORDER);
+ Assertions.assertThat(table.properties().entrySet())
+ .as("Table properties should be a superset of the requested properties")
+ .containsAll(properties.entrySet());
Review Comment:
should that be containsExactlyInAnyOrder?
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -548,20 +556,20 @@ public void testTableNameWithDot() {
catalog.createNamespace(Namespace.of("ns"));
}
- Assert.assertFalse("Table should not exist", catalog.tableExists(ident));
+ Assertions.assertThat(catalog.tableExists(ident)).as("Table should not exist").isFalse();
catalog.buildTable(ident, SCHEMA).create();
- Assert.assertTrue("Table should exist", catalog.tableExists(ident));
+
Review Comment:
nit: unnecessary newline
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -651,24 +661,27 @@ public void testLoadTable() {
.withSortOrder(WRITE_ORDER)
.withProperties(properties)
.create();
- Assert.assertTrue("Table should exist", catalog.tableExists(ident));
+ Assertions.assertThat(catalog.tableExists(ident)).as("Table should exist").isTrue();
Table table = catalog.loadTable(ident);
// validate table settings
- Assert.assertEquals(
- "Table name should report its full name", catalog.name() + "." + ident, table.name());
- Assert.assertTrue("Table should exist", catalog.tableExists(ident));
- Assert.assertEquals(
- "Schema should match expected ID assignment",
- TABLE_SCHEMA.asStruct(),
- table.schema().asStruct());
- Assert.assertNotNull("Should have a location", table.location());
- Assert.assertEquals("Should use requested partition spec", TABLE_SPEC, table.spec());
- Assert.assertEquals("Should use requested write order", TABLE_WRITE_ORDER, table.sortOrder());
- Assert.assertEquals(
- "Table properties should be a superset of the requested properties",
- properties.entrySet(),
- Sets.intersection(properties.entrySet(), table.properties().entrySet()));
+ Assertions.assertThat(table.name())
+ .as("Table name should report its full name")
+ .isEqualTo(catalog.name() + "." + ident);
+ Assertions.assertThat(catalog.tableExists(ident)).as("Table should exist").isTrue();
+ Assertions.assertThat(table.schema().asStruct())
+ .as("Schema should match expected ID assignment")
+ .isEqualTo(TABLE_SCHEMA.asStruct());
+ Assertions.assertThat(table.location()).as("Should have a location").isNotNull();
+ Assertions.assertThat(table.spec())
+ .as("Should use requested partition spec")
+ .isEqualTo(TABLE_SPEC);
+ Assertions.assertThat(table.sortOrder())
+ .as("Should use requested write order")
+ .isEqualTo(TABLE_WRITE_ORDER);
+ Assertions.assertThat(table.properties().entrySet())
+ .as("Table properties should be a superset of the requested properties")
+ .containsAll(properties.entrySet());
Review Comment:
containsExactlyInAnyOrder?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1230580254
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -614,20 +621,23 @@ public void testCompleteCreateTable() {
.create();
// validate table settings
- Assert.assertEquals(
- "Table name should report its full name", catalog.name() + "." + ident, table.name());
- Assert.assertTrue("Table should exist", catalog.tableExists(ident));
- Assert.assertEquals(
- "Schema should match expected ID assignment",
- TABLE_SCHEMA.asStruct(),
- table.schema().asStruct());
- Assert.assertNotNull("Should have a location", table.location());
- Assert.assertEquals("Should use requested partition spec", TABLE_SPEC, table.spec());
- Assert.assertEquals("Should use requested write order", TABLE_WRITE_ORDER, table.sortOrder());
- Assert.assertEquals(
- "Table properties should be a superset of the requested properties",
- properties.entrySet(),
- Sets.intersection(properties.entrySet(), table.properties().entrySet()));
+ Assertions.assertThat(table.name())
+ .as("Table name should report its full name")
+ .isEqualTo(catalog.name() + "." + ident);
+ Assertions.assertThat(catalog.tableExists(ident)).as("Table should exist").isTrue();
+ Assertions.assertThat(table.schema().asStruct())
+ .as("Schema should match expected ID assignment")
+ .isEqualTo(TABLE_SCHEMA.asStruct());
+ Assertions.assertThat(table.location()).as("Should have a location").isNotNull();
+ Assertions.assertThat(table.spec())
+ .as("Should use requested partition spec")
+ .isEqualTo(TABLE_SPEC);
+ Assertions.assertThat(table.sortOrder())
+ .as("Should use requested write order")
+ .isEqualTo(TABLE_WRITE_ORDER);
+ Assertions.assertThat(table.properties().entrySet())
+ .as("Table properties should be a superset of the requested properties")
+ .containsAll(properties.entrySet());
Review Comment:
@skytin1004 this still uses `containsAll` rather than `containsExactlyInAnyOrder`. We want to make sure that `table.properties()` doesn't contain any more elements, hence the `containsExactlyInAnyOrder` check
--
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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1230580922
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -651,24 +661,27 @@ public void testLoadTable() {
.withSortOrder(WRITE_ORDER)
.withProperties(properties)
.create();
- Assert.assertTrue("Table should exist", catalog.tableExists(ident));
+ Assertions.assertThat(catalog.tableExists(ident)).as("Table should exist").isTrue();
Table table = catalog.loadTable(ident);
// validate table settings
- Assert.assertEquals(
- "Table name should report its full name", catalog.name() + "." + ident, table.name());
- Assert.assertTrue("Table should exist", catalog.tableExists(ident));
- Assert.assertEquals(
- "Schema should match expected ID assignment",
- TABLE_SCHEMA.asStruct(),
- table.schema().asStruct());
- Assert.assertNotNull("Should have a location", table.location());
- Assert.assertEquals("Should use requested partition spec", TABLE_SPEC, table.spec());
- Assert.assertEquals("Should use requested write order", TABLE_WRITE_ORDER, table.sortOrder());
- Assert.assertEquals(
- "Table properties should be a superset of the requested properties",
- properties.entrySet(),
- Sets.intersection(properties.entrySet(), table.properties().entrySet()));
+ Assertions.assertThat(table.name())
+ .as("Table name should report its full name")
+ .isEqualTo(catalog.name() + "." + ident);
+ Assertions.assertThat(catalog.tableExists(ident)).as("Table should exist").isTrue();
+ Assertions.assertThat(table.schema().asStruct())
+ .as("Schema should match expected ID assignment")
+ .isEqualTo(TABLE_SCHEMA.asStruct());
+ Assertions.assertThat(table.location()).as("Should have a location").isNotNull();
+ Assertions.assertThat(table.spec())
+ .as("Should use requested partition spec")
+ .isEqualTo(TABLE_SPEC);
+ Assertions.assertThat(table.sortOrder())
+ .as("Should use requested write order")
+ .isEqualTo(TABLE_WRITE_ORDER);
+ Assertions.assertThat(table.properties().entrySet())
+ .as("Table properties should be a superset of the requested properties")
+ .containsAll(properties.entrySet());
Review Comment:
this still needs to be changed
--
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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1230596958
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -2489,30 +2617,28 @@ public void tableCreationWithoutNamespace() {
private static void assertEmpty(String context, Catalog catalog, Namespace ns) {
try {
- Assert.assertEquals(context, 0, catalog.listTables(ns).size());
+ Assertions.assertThat(catalog.listTables(ns).size()).as("context").isEqualTo(0);
Review Comment:
```suggestion
Assertions.assertThat(catalog.listTables(ns)).as(context).isEmpty();
```
--
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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1230583426
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -1478,33 +1527,39 @@ public void testCompleteCreateTransaction() {
.withProperties(properties)
.createTransaction();
- Assert.assertFalse(
- "Table should not exist after createTransaction", catalog.tableExists(TABLE));
+ Assertions.assertThat(catalog.tableExists(TABLE))
+ .as("Table should not exist after createTransaction")
+ .isFalse();
create.newFastAppend().appendFile(FILE_A).commit();
- Assert.assertFalse("Table should not exist after append commit", catalog.tableExists(TABLE));
+ Assertions.assertThat(catalog.tableExists(TABLE))
+ .as("Table should not exist after append commit")
+ .isFalse();
create.commitTransaction();
- Assert.assertTrue("Table should exist after append commit", catalog.tableExists(TABLE));
- Table table = catalog.loadTable(TABLE);
+ Assertions.assertThat(catalog.tableExists(TABLE))
+ .as("Table should exist after append commit")
+ .isTrue();
- Assert.assertEquals(
- "Table schema should match the new schema",
- TABLE_SCHEMA.asStruct(),
- table.schema().asStruct());
- Assert.assertEquals(
- "Table should have create partition spec", TABLE_SPEC.fields(), table.spec().fields());
- Assert.assertEquals(
- "Table should have create sort order", TABLE_WRITE_ORDER, table.sortOrder());
- Assert.assertEquals(
- "Table properties should be a superset of the requested properties",
- properties.entrySet(),
- Sets.intersection(properties.entrySet(), table.properties().entrySet()));
+ Table table = catalog.loadTable(TABLE);
+ Assertions.assertThat(table.schema().asStruct())
+ .as("Table schema should match the new schema")
+ .isEqualTo(TABLE_SCHEMA.asStruct());
+ Assertions.assertThat(table.spec().fields())
+ .as("Table should have create partition spec")
+ .isEqualTo(TABLE_SPEC.fields());
+ Assertions.assertThat(table.sortOrder())
+ .as("Table should have create sort order")
+ .isEqualTo(TABLE_WRITE_ORDER);
+ Assertions.assertThat(table.properties().entrySet())
+ .as("Table properties should be a superset of the requested properties")
+ .containsAll(properties.entrySet());
Review Comment:
`containsExactlyInAnyOrder` should be used 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 #7767: Core: Switch tests to Junit5 in catalog,encryption,inmemory,io,view
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7767:
URL: https://github.com/apache/iceberg/pull/7767#discussion_r1230580922
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -651,24 +661,27 @@ public void testLoadTable() {
.withSortOrder(WRITE_ORDER)
.withProperties(properties)
.create();
- Assert.assertTrue("Table should exist", catalog.tableExists(ident));
+ Assertions.assertThat(catalog.tableExists(ident)).as("Table should exist").isTrue();
Table table = catalog.loadTable(ident);
// validate table settings
- Assert.assertEquals(
- "Table name should report its full name", catalog.name() + "." + ident, table.name());
- Assert.assertTrue("Table should exist", catalog.tableExists(ident));
- Assert.assertEquals(
- "Schema should match expected ID assignment",
- TABLE_SCHEMA.asStruct(),
- table.schema().asStruct());
- Assert.assertNotNull("Should have a location", table.location());
- Assert.assertEquals("Should use requested partition spec", TABLE_SPEC, table.spec());
- Assert.assertEquals("Should use requested write order", TABLE_WRITE_ORDER, table.sortOrder());
- Assert.assertEquals(
- "Table properties should be a superset of the requested properties",
- properties.entrySet(),
- Sets.intersection(properties.entrySet(), table.properties().entrySet()));
+ Assertions.assertThat(table.name())
+ .as("Table name should report its full name")
+ .isEqualTo(catalog.name() + "." + ident);
+ Assertions.assertThat(catalog.tableExists(ident)).as("Table should exist").isTrue();
+ Assertions.assertThat(table.schema().asStruct())
+ .as("Schema should match expected ID assignment")
+ .isEqualTo(TABLE_SCHEMA.asStruct());
+ Assertions.assertThat(table.location()).as("Should have a location").isNotNull();
+ Assertions.assertThat(table.spec())
+ .as("Should use requested partition spec")
+ .isEqualTo(TABLE_SPEC);
+ Assertions.assertThat(table.sortOrder())
+ .as("Should use requested write order")
+ .isEqualTo(TABLE_WRITE_ORDER);
+ Assertions.assertThat(table.properties().entrySet())
+ .as("Table properties should be a superset of the requested properties")
+ .containsAll(properties.entrySet());
Review Comment:
this still needs to be changed
--
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