You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "DaVincii (via GitHub)" <gi...@apache.org> on 2023/06/23 22:49:41 UTC
[GitHub] [iceberg] DaVincii opened a new pull request, #7895: Core: Migrating Util module to Junit5
DaVincii opened a new pull request, #7895:
URL: https://github.com/apache/iceberg/pull/7895
(no comment)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241789521
##########
core/src/test/java/org/apache/iceberg/util/TestReachableFileUtil.java:
##########
@@ -87,7 +83,7 @@ public void testManifestListLocations() {
table.newAppend().appendFile(FILE_B).commit();
List<String> manifestListPaths = ReachableFileUtil.manifestListLocations(table);
- Assert.assertEquals(manifestListPaths.size(), 2);
+ assertThat(manifestListPaths.size()).isEqualTo(2);
Review Comment:
```suggestion
assertThat(manifestListPaths).hasSize(2);
```
--
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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241799556
##########
core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java:
##########
@@ -120,29 +120,28 @@ public void testRemove() {
Map<StructLike, String> map = StructLikeMap.create(STRUCT_TYPE);
map.put(record, "1-aaa");
- Assert.assertEquals(1, map.size());
- Assert.assertEquals("1-aaa", map.get(record));
- Assert.assertEquals("1-aaa", map.remove(record));
- Assert.assertEquals(0, map.size());
+ assertThat(map.size()).isOne();
+ assertThat(map).containsEntry(record, "1-aaa");
Review Comment:
`assertThat(map).hasSize(1).containsEntry(...)` and then you can remove the check 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] nastra commented on a diff in pull request #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241794004
##########
core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java:
##########
@@ -43,28 +43,29 @@ public void testSingleRecord() {
Record record1 = gRecord.copy(ImmutableMap.of("id", 1, "data", "aaa"));
Map<StructLike, String> map = StructLikeMap.create(STRUCT_TYPE);
- Assert.assertEquals(0, map.size());
+ assertThat(map).isEmpty();
map.put(record1, "1-aaa");
- Assert.assertEquals(1, map.size());
- Assert.assertFalse(map.isEmpty());
- Assert.assertTrue(map.containsKey(record1));
- Assert.assertTrue(map.containsValue("1-aaa"));
- Assert.assertEquals("1-aaa", map.get(record1));
+ assertThat(map.size()).isOne();
+ assertThat(map)
Review Comment:
no need to check the same thing multiple times:
```
assertThat(map)
.hasSize(1)
.containsEntry(record1, "1-aaa")
```
--
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 #7895: Core: Migrating Util module to Junit5
Posted by "skytin1004 (via GitHub)" <gi...@apache.org>.
skytin1004 commented on PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#issuecomment-1621212973
Hi @DaVincii, @nastra , It seems like this PR hasn't gotten any work done in days. there's also a branch conflict issue. If you're too busy to work on this, would you mind if I took over? I will fix the code based on the review and PR it again as a co-author.
--
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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241797499
##########
core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java:
##########
@@ -43,28 +43,29 @@ public void testSingleRecord() {
Record record1 = gRecord.copy(ImmutableMap.of("id", 1, "data", "aaa"));
Map<StructLike, String> map = StructLikeMap.create(STRUCT_TYPE);
- Assert.assertEquals(0, map.size());
+ assertThat(map).isEmpty();
map.put(record1, "1-aaa");
- Assert.assertEquals(1, map.size());
- Assert.assertFalse(map.isEmpty());
- Assert.assertTrue(map.containsKey(record1));
- Assert.assertTrue(map.containsValue("1-aaa"));
- Assert.assertEquals("1-aaa", map.get(record1));
+ assertThat(map.size()).isOne();
+ assertThat(map)
+ .isNotEmpty()
+ .containsKey(record1)
+ .containsValue("1-aaa")
+ .containsEntry(record1, "1-aaa");
Set<StructLike> keySet = map.keySet();
- Assert.assertEquals(1, keySet.size());
- Assert.assertTrue(keySet.contains(record1));
+ assertThat(keySet.size()).isOne();
+ assertThat(keySet).contains(record1);
Collection<String> values = map.values();
- Assert.assertEquals(1, values.size());
- Assert.assertEquals("1-aaa", values.toArray(new String[0])[0]);
+ assertThat(values.size()).isOne();
+ assertThat(values.toArray(new String[0])[0]).isEqualTo("1-aaa");
Set<Map.Entry<StructLike, String>> entrySet = map.entrySet();
- Assert.assertEquals(1, entrySet.size());
+ assertThat(entrySet.size()).isOne();
Review Comment:
hasSize
--
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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241802681
##########
core/src/test/java/org/apache/iceberg/util/TestTableScanUtil.java:
##########
@@ -125,8 +123,7 @@ public void testTaskGroupPlanning() {
CloseableIterable<ScanTaskGroup<ParentTask>> taskGroups =
TableScanUtil.planTaskGroups(CloseableIterable.withNoopClose(tasks), 128, 10, 4);
-
- Assert.assertEquals("Must have 3 task groups", 3, Iterables.size(taskGroups));
+ Assertions.assertThat(taskGroups).as("Must have 3 task groups").hasSize(3);
Review Comment:
why `Assertions.assertThat` here when you're using `assertThat` in all the other places in this file?
--
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] DaVincii commented on pull request #7895: Core: Migrating Util module to Junit5
Posted by "DaVincii (via GitHub)" <gi...@apache.org>.
DaVincii commented on PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#issuecomment-1621666228
> Hi @DaVincii, @nastra , It seems like this PR hasn't gotten any work done in days. there's also a branch conflict issue. If you're too busy to work on this, would you mind if I took over? I will fix the code based on the review and PR it again as a co-author.
@skytin1004 Got busy with some office work, now I will have some more time, will be updating it in next few days.
--
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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1254176431
##########
core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java:
##########
@@ -18,247 +18,202 @@
*/
package org.apache.iceberg.util;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
import com.fasterxml.jackson.core.JsonProcessingException;
import java.nio.ByteBuffer;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
-import org.assertj.core.api.Assertions;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class TestJsonUtil {
@Test
public void get() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.get("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.get("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing field: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.get("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.get("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing field: x");
- Assertions.assertThat(JsonUtil.get("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")).asText())
+ assertThat(JsonUtil.get("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")).asText())
.isEqualTo("23");
}
@Test
public void getInt() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing int: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: null");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: \"23\"");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
+ assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: 23.0");
- Assertions.assertThat(JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
- .isEqualTo(23);
+ assertThat(JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": 23}"))).isEqualTo(23);
}
@Test
public void getIntOrNull() throws JsonProcessingException {
- Assertions.assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
- Assertions.assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
- .isEqualTo(23);
- Assertions.assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
- .isNull();
+ assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}"))).isEqualTo(23);
+ assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}"))).isNull();
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: \"23\"");
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: 23.0");
}
@Test
public void getLong() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing long: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: null");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: \"23\"");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
+ assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: 23.0");
- Assertions.assertThat(JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
- .isEqualTo(23);
+ assertThat(JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": 23}"))).isEqualTo(23);
}
@Test
public void getLongOrNull() throws JsonProcessingException {
- Assertions.assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
- Assertions.assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
+ assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
.isEqualTo(23);
- Assertions.assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
- .isNull();
+ assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}"))).isNull();
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: \"23\"");
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: 23.0");
}
@Test
public void getString() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing string: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a string value: x: null");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
+ assertThatThrownBy(() -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a string value: x: 23");
- Assertions.assertThat(JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThat(JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isEqualTo("23");
}
@Test
public void getStringOrNull() throws JsonProcessingException {
- Assertions.assertThat(JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
- Assertions.assertThat(
- JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThat(JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isEqualTo("23");
- Assertions.assertThat(
- JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
- .isNull();
+ assertThat(JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}"))).isNull();
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a string value: x: 23");
}
@Test
public void getByteBufferOrNull() throws JsonProcessingException {
- Assertions.assertThat(JsonUtil.getByteBufferOrNull("x", JsonUtil.mapper().readTree("{}")))
- .isNull();
- Assertions.assertThat(
- JsonUtil.getByteBufferOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThat(JsonUtil.getByteBufferOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getByteBufferOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isNull();
byte[] bytes = new byte[] {1, 2, 3, 4};
String base16Str = BaseEncoding.base16().encode(bytes);
String json = String.format("{\"x\": \"%s\"}", base16Str);
ByteBuffer byteBuffer = JsonUtil.getByteBufferOrNull("x", JsonUtil.mapper().readTree(json));
- Assertions.assertThat(byteBuffer.array()).isEqualTo(bytes);
+ assertThat(byteBuffer.array()).isEqualTo(bytes);
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getByteBufferOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse byte buffer from non-text value: x: 23");
}
@Test
public void getBool() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing boolean: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a boolean value: x: null");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThatThrownBy(() -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a boolean value: x: \"23\"");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": \"true\"}")))
+ assertThatThrownBy(() -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": \"true\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a boolean value: x: \"true\"");
- Assertions.assertThat(JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": true}")))
- .isTrue();
- Assertions.assertThat(JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": false}")))
- .isFalse();
- }
-
- @Test
- public void getIntArrayOrNull() throws JsonProcessingException {
Review Comment:
this also needs to be reverted. In fact, can you please revert all changes to this file. I believe the only change that this file requires is switching from `org.junit.Test` to `org.junit.jupiter.api.Test` (everything else just adds too much visual noise
##########
core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java:
##########
@@ -18,247 +18,202 @@
*/
package org.apache.iceberg.util;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
import com.fasterxml.jackson.core.JsonProcessingException;
import java.nio.ByteBuffer;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
-import org.assertj.core.api.Assertions;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class TestJsonUtil {
@Test
public void get() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.get("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.get("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing field: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.get("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.get("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing field: x");
- Assertions.assertThat(JsonUtil.get("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")).asText())
+ assertThat(JsonUtil.get("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")).asText())
.isEqualTo("23");
}
@Test
public void getInt() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing int: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: null");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: \"23\"");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
+ assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: 23.0");
- Assertions.assertThat(JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
- .isEqualTo(23);
+ assertThat(JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": 23}"))).isEqualTo(23);
}
@Test
public void getIntOrNull() throws JsonProcessingException {
- Assertions.assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
- Assertions.assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
- .isEqualTo(23);
- Assertions.assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
- .isNull();
+ assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}"))).isEqualTo(23);
+ assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}"))).isNull();
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: \"23\"");
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: 23.0");
}
@Test
public void getLong() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing long: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: null");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: \"23\"");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
+ assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: 23.0");
- Assertions.assertThat(JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
- .isEqualTo(23);
+ assertThat(JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": 23}"))).isEqualTo(23);
}
@Test
public void getLongOrNull() throws JsonProcessingException {
- Assertions.assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
- Assertions.assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
+ assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
.isEqualTo(23);
- Assertions.assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
- .isNull();
+ assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}"))).isNull();
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: \"23\"");
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: 23.0");
}
@Test
public void getString() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing string: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a string value: x: null");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
+ assertThatThrownBy(() -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a string value: x: 23");
- Assertions.assertThat(JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThat(JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isEqualTo("23");
}
@Test
public void getStringOrNull() throws JsonProcessingException {
- Assertions.assertThat(JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
- Assertions.assertThat(
- JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThat(JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isEqualTo("23");
- Assertions.assertThat(
- JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
- .isNull();
+ assertThat(JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}"))).isNull();
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a string value: x: 23");
}
@Test
public void getByteBufferOrNull() throws JsonProcessingException {
- Assertions.assertThat(JsonUtil.getByteBufferOrNull("x", JsonUtil.mapper().readTree("{}")))
- .isNull();
- Assertions.assertThat(
- JsonUtil.getByteBufferOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThat(JsonUtil.getByteBufferOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getByteBufferOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isNull();
byte[] bytes = new byte[] {1, 2, 3, 4};
String base16Str = BaseEncoding.base16().encode(bytes);
String json = String.format("{\"x\": \"%s\"}", base16Str);
ByteBuffer byteBuffer = JsonUtil.getByteBufferOrNull("x", JsonUtil.mapper().readTree(json));
- Assertions.assertThat(byteBuffer.array()).isEqualTo(bytes);
+ assertThat(byteBuffer.array()).isEqualTo(bytes);
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getByteBufferOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse byte buffer from non-text value: x: 23");
}
@Test
public void getBool() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing boolean: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a boolean value: x: null");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThatThrownBy(() -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a boolean value: x: \"23\"");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": \"true\"}")))
+ assertThatThrownBy(() -> JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": \"true\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a boolean value: x: \"true\"");
- Assertions.assertThat(JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": true}")))
- .isTrue();
- Assertions.assertThat(JsonUtil.getBool("x", JsonUtil.mapper().readTree("{\"x\": false}")))
- .isFalse();
- }
-
- @Test
- public void getIntArrayOrNull() throws JsonProcessingException {
Review Comment:
this also needs to be reverted. In fact, can you please revert all changes to this file. I believe the only change that this file requires is switching from `org.junit.Test` to `org.junit.jupiter.api.Test` (everything else just adds too much visual noise)
--
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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra merged PR #7895:
URL: https://github.com/apache/iceberg/pull/7895
--
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] DaVincii commented on pull request #7895: Core: Migrating Util module to Junit5
Posted by "DaVincii (via GitHub)" <gi...@apache.org>.
DaVincii commented on PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#issuecomment-1623184502
> there are tests being removed in `TestJsonUtil` (most likely from recent merge conflicts) that need to be restored
ah my bad, added back the test.
--
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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241784570
##########
core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java:
##########
@@ -18,203 +18,182 @@
*/
package org.apache.iceberg.util;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.AssertionsForClassTypes.assertThatThrownBy;
Review Comment:
that's the wrong import. it should be `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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241800252
##########
core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java:
##########
@@ -120,29 +120,28 @@ public void testRemove() {
Map<StructLike, String> map = StructLikeMap.create(STRUCT_TYPE);
map.put(record, "1-aaa");
- Assert.assertEquals(1, map.size());
- Assert.assertEquals("1-aaa", map.get(record));
- Assert.assertEquals("1-aaa", map.remove(record));
- Assert.assertEquals(0, map.size());
+ assertThat(map.size()).isOne();
+ assertThat(map).containsEntry(record, "1-aaa");
+ assertThat(map.remove(record)).isEqualTo("1-aaa");
+ assertThat(map).isEmpty();
map.put(record, "1-aaa");
- Assert.assertEquals("1-aaa", map.get(record));
+ assertThat(map).containsEntry(record, "1-aaa");
}
@Test
public void testNullKeys() {
Map<StructLike, String> map = StructLikeMap.create(STRUCT_TYPE);
- Assert.assertFalse(map.containsKey(null));
+ assertThat(map).doesNotContainKey(null);
map.put(null, "aaa");
- Assert.assertTrue(map.containsKey(null));
- Assert.assertEquals("aaa", map.get(null));
+ assertThat(map).containsKey(null).containsEntry(null, "aaa");
Review Comment:
`containsKey` can be removed 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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241800927
##########
core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java:
##########
@@ -155,13 +154,11 @@ public void testKeysWithNulls() {
map.put(record1, "aaa");
map.put(record2, "bbb");
- Assert.assertEquals("aaa", map.get(record1));
- Assert.assertEquals("bbb", map.get(record2));
+ assertThat(map).containsEntry(record1, "aaa").containsEntry(record2, "bbb");
Record record3 = record1.copy();
- Assert.assertTrue(map.containsKey(record3));
- Assert.assertEquals("aaa", map.get(record3));
+ assertThat(map).containsKey(record3).containsEntry(record3, "aaa");
Review Comment:
`containsKey` can be removed 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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241782716
##########
core/src/test/java/org/apache/iceberg/util/TestInMemoryLockManager.java:
##########
@@ -28,29 +30,26 @@
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.assertj.core.api.Assertions;
-import org.junit.After;
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Rule;
-import org.junit.Test;
-import org.junit.rules.Timeout;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.Timeout;
public class TestInMemoryLockManager {
private LockManagers.InMemoryLockManager lockManager;
private String lockEntityId;
private String ownerId;
- @Rule public Timeout timeout = new Timeout(5, TimeUnit.SECONDS);
-
- @Before
+ @Timeout(value = 5, unit = TimeUnit.SECONDS)
Review Comment:
should this be moved to the class level to be globally applied?
--
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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241790166
##########
core/src/test/java/org/apache/iceberg/util/TestReachableFileUtil.java:
##########
@@ -119,7 +115,7 @@ public void testMetadataFileLocationsWithMissingFiles() {
table.io().deleteFile(location);
Set<String> metadataFileLocations = ReachableFileUtil.metadataFileLocations(table, true);
- Assert.assertEquals(metadataFileLocations.size(), 2);
+ assertThat(metadataFileLocations.size()).isEqualTo(2);
Review Comment:
same as above. please also adjust the other places in this file
--
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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241798375
##########
core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java:
##########
@@ -79,38 +80,37 @@ public void testMultipleRecord() {
record3.setField("data", null);
Map<StructLike, String> map = StructLikeMap.create(STRUCT_TYPE);
- Assert.assertEquals(0, map.size());
+ assertThat(map).isEmpty();
map.putAll(ImmutableMap.of(record1, "1-aaa", record2, "2-bbb", record3, "3-null"));
- Assert.assertEquals(3, map.size());
- Assert.assertTrue(map.containsKey(record1));
- Assert.assertTrue(map.containsKey(record2));
- Assert.assertTrue(map.containsKey(record3));
- Assert.assertTrue(map.containsValue("1-aaa"));
- Assert.assertTrue(map.containsValue("2-bbb"));
- Assert.assertTrue(map.containsValue("3-null"));
- Assert.assertEquals("1-aaa", map.get(record1));
- Assert.assertEquals("2-bbb", map.get(record2));
- Assert.assertEquals("3-null", map.get(record3));
+ assertThat(map)
+ .hasSize(3)
+ .containsKey(record1)
Review Comment:
we don't need to check for `containsKey` and `containsValue` if we already check for `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] nastra commented on a diff in pull request #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241795914
##########
core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java:
##########
@@ -43,28 +43,29 @@ public void testSingleRecord() {
Record record1 = gRecord.copy(ImmutableMap.of("id", 1, "data", "aaa"));
Map<StructLike, String> map = StructLikeMap.create(STRUCT_TYPE);
- Assert.assertEquals(0, map.size());
+ assertThat(map).isEmpty();
map.put(record1, "1-aaa");
- Assert.assertEquals(1, map.size());
- Assert.assertFalse(map.isEmpty());
- Assert.assertTrue(map.containsKey(record1));
- Assert.assertTrue(map.containsValue("1-aaa"));
- Assert.assertEquals("1-aaa", map.get(record1));
+ assertThat(map.size()).isOne();
+ assertThat(map)
+ .isNotEmpty()
+ .containsKey(record1)
+ .containsValue("1-aaa")
+ .containsEntry(record1, "1-aaa");
Set<StructLike> keySet = map.keySet();
- Assert.assertEquals(1, keySet.size());
- Assert.assertTrue(keySet.contains(record1));
+ assertThat(keySet.size()).isOne();
+ assertThat(keySet).contains(record1);
Collection<String> values = map.values();
- Assert.assertEquals(1, values.size());
- Assert.assertEquals("1-aaa", values.toArray(new String[0])[0]);
+ assertThat(values.size()).isOne();
+ assertThat(values.toArray(new String[0])[0]).isEqualTo("1-aaa");
Review Comment:
why not `assertThat(values).hasSize(1).first().isEqualTo("1-aaa")`
--
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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241793111
##########
core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java:
##########
@@ -43,28 +43,29 @@ public void testSingleRecord() {
Record record1 = gRecord.copy(ImmutableMap.of("id", 1, "data", "aaa"));
Map<StructLike, String> map = StructLikeMap.create(STRUCT_TYPE);
- Assert.assertEquals(0, map.size());
+ assertThat(map).isEmpty();
map.put(record1, "1-aaa");
- Assert.assertEquals(1, map.size());
- Assert.assertFalse(map.isEmpty());
- Assert.assertTrue(map.containsKey(record1));
- Assert.assertTrue(map.containsValue("1-aaa"));
- Assert.assertEquals("1-aaa", map.get(record1));
+ assertThat(map.size()).isOne();
Review Comment:
in fact this line can be removed and the check can be combined with the next one
--
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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241792621
##########
core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java:
##########
@@ -43,28 +43,29 @@ public void testSingleRecord() {
Record record1 = gRecord.copy(ImmutableMap.of("id", 1, "data", "aaa"));
Map<StructLike, String> map = StructLikeMap.create(STRUCT_TYPE);
- Assert.assertEquals(0, map.size());
+ assertThat(map).isEmpty();
map.put(record1, "1-aaa");
- Assert.assertEquals(1, map.size());
- Assert.assertFalse(map.isEmpty());
- Assert.assertTrue(map.containsKey(record1));
- Assert.assertTrue(map.containsValue("1-aaa"));
- Assert.assertEquals("1-aaa", map.get(record1));
+ assertThat(map.size()).isOne();
Review Comment:
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] DaVincii commented on pull request #7895: Core: Migrating Util module to Junit5
Posted by "DaVincii (via GitHub)" <gi...@apache.org>.
DaVincii commented on PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#issuecomment-1623474980
> LGTM, thanks @DaVincii
I don't have write access, all the check have passed. Should be good to merge 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 #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1254025598
##########
core/src/test/java/org/apache/iceberg/util/TestJsonUtil.java:
##########
@@ -18,247 +18,182 @@
*/
package org.apache.iceberg.util;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
import com.fasterxml.jackson.core.JsonProcessingException;
-import java.nio.ByteBuffer;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
-import org.apache.iceberg.relocated.com.google.common.io.BaseEncoding;
-import org.assertj.core.api.Assertions;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class TestJsonUtil {
@Test
public void get() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.get("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.get("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing field: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.get("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.get("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing field: x");
- Assertions.assertThat(JsonUtil.get("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")).asText())
+ assertThat(JsonUtil.get("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")).asText())
.isEqualTo("23");
}
@Test
public void getInt() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing int: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: null");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: \"23\"");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
+ assertThatThrownBy(() -> JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: 23.0");
- Assertions.assertThat(JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
- .isEqualTo(23);
+ assertThat(JsonUtil.getInt("x", JsonUtil.mapper().readTree("{\"x\": 23}"))).isEqualTo(23);
}
@Test
public void getIntOrNull() throws JsonProcessingException {
- Assertions.assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
- Assertions.assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
- .isEqualTo(23);
- Assertions.assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
- .isNull();
+ assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}"))).isEqualTo(23);
+ assertThat(JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}"))).isNull();
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: \"23\"");
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getIntOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to an integer value: x: 23.0");
}
@Test
public void getLong() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing long: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: null");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: \"23\"");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
+ assertThatThrownBy(() -> JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: 23.0");
- Assertions.assertThat(JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
- .isEqualTo(23);
+ assertThat(JsonUtil.getLong("x", JsonUtil.mapper().readTree("{\"x\": 23}"))).isEqualTo(23);
}
@Test
public void getLongOrNull() throws JsonProcessingException {
- Assertions.assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
- Assertions.assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
+ assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
.isEqualTo(23);
- Assertions.assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
- .isNull();
+ assertThat(JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}"))).isNull();
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: \"23\"");
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getLongOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23.0}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a long value: x: 23.0");
}
@Test
public void getString() throws JsonProcessingException {
- Assertions.assertThatThrownBy(() -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{}")))
+ assertThatThrownBy(() -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse missing string: x");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": null}")))
+ assertThatThrownBy(() -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": null}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a string value: x: null");
- Assertions.assertThatThrownBy(
- () -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
+ assertThatThrownBy(() -> JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a string value: x: 23");
- Assertions.assertThat(JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThat(JsonUtil.getString("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isEqualTo("23");
}
@Test
public void getStringOrNull() throws JsonProcessingException {
- Assertions.assertThat(JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
- Assertions.assertThat(
- JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
+ assertThat(JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{}"))).isNull();
+ assertThat(JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": \"23\"}")))
.isEqualTo("23");
- Assertions.assertThat(
- JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}")))
- .isNull();
+ assertThat(JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": null}"))).isNull();
- Assertions.assertThatThrownBy(
+ assertThatThrownBy(
() -> JsonUtil.getStringOrNull("x", JsonUtil.mapper().readTree("{\"x\": 23}")))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot parse to a string value: x: 23");
}
- @Test
- public void getByteBufferOrNull() throws JsonProcessingException {
Review Comment:
why is this test method and a few others being 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] DaVincii commented on pull request #7895: Core: Migrating Util module to Junit5
Posted by "DaVincii (via GitHub)" <gi...@apache.org>.
DaVincii commented on PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#issuecomment-1622407657
@nastra Updated the pull request with the review changes, please 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] nastra commented on a diff in pull request #7895: Core: Migrating Util module to Junit5
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7895:
URL: https://github.com/apache/iceberg/pull/7895#discussion_r1241794690
##########
core/src/test/java/org/apache/iceberg/util/TestStructLikeMap.java:
##########
@@ -43,28 +43,29 @@ public void testSingleRecord() {
Record record1 = gRecord.copy(ImmutableMap.of("id", 1, "data", "aaa"));
Map<StructLike, String> map = StructLikeMap.create(STRUCT_TYPE);
- Assert.assertEquals(0, map.size());
+ assertThat(map).isEmpty();
map.put(record1, "1-aaa");
- Assert.assertEquals(1, map.size());
- Assert.assertFalse(map.isEmpty());
- Assert.assertTrue(map.containsKey(record1));
- Assert.assertTrue(map.containsValue("1-aaa"));
- Assert.assertEquals("1-aaa", map.get(record1));
+ assertThat(map.size()).isOne();
+ assertThat(map)
+ .isNotEmpty()
+ .containsKey(record1)
+ .containsValue("1-aaa")
+ .containsEntry(record1, "1-aaa");
Set<StructLike> keySet = map.keySet();
- Assert.assertEquals(1, keySet.size());
- Assert.assertTrue(keySet.contains(record1));
+ assertThat(keySet.size()).isOne();
+ assertThat(keySet).contains(record1);
Review Comment:
can be combined to `assertThat(keySet).hasSize(1).contains(record1)`.
Please also do the same for all the other places
--
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