You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "nastra (via GitHub)" <gi...@apache.org> on 2023/06/09 08:01:58 UTC
[GitHub] [iceberg] nastra commented on a diff in pull request #7803: Core: Switch tests to Junit5 in metrics,puffin,rest pakages
nastra commented on code in PR #7803:
URL: https://github.com/apache/iceberg/pull/7803#discussion_r1223972239
##########
core/src/test/java/org/apache/iceberg/rest/requests/TestCreateNamespaceRequest.java:
##########
@@ -148,8 +147,12 @@ public CreateNamespaceRequest createExampleInstance() {
@Override
public void assertEquals(CreateNamespaceRequest actual, CreateNamespaceRequest expected) {
- Assert.assertEquals("Namespaces should be equal", actual.namespace(), expected.namespace());
- Assert.assertEquals("Properties should be equal", actual.properties(), expected.properties());
+ Assertions.assertThat(actual.namespace())
+ .as("Namespaces should be equal")
Review Comment:
nit: I think this can be omitted as it's obvious
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponse.java:
##########
@@ -145,64 +144,59 @@ public void testCanUseNullAsPropertyValue() throws JsonProcessingException {
public void testDeserializeInvalidResponse() {
String jsonDefaultsHasWrongType =
"{\"defaults\":[\"warehouse\",\"s3://bucket/warehouse\"],\"overrides\":{\"clients\":\"5\"}}";
- AssertHelpers.assertThrows(
- "A JSON response with the wrong type for the defaults field should fail to deserialize",
- JsonProcessingException.class,
- () -> deserialize(jsonDefaultsHasWrongType));
+ Assertions.assertThatThrownBy(() -> deserialize(jsonDefaultsHasWrongType))
+ .as("A JSON response with the wrong type for the defaults field should fail to deserialize")
+ .isInstanceOf(JsonProcessingException.class);
Review Comment:
this should have `.hasMessage(..)` and we can omit `.as()` here.
All of the others in this file also need a `.hasMessage(..)`
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestConfigResponse.java:
##########
@@ -226,12 +220,13 @@ public void testMergeStripsNullValuedEntries() {
"b", "from_overrides",
"c", "from_client");
- Assert.assertEquals(
- "The merged properties map should use values from defaults, then client config, and finally overrides",
- expected,
- merged);
- Assert.assertFalse(
- "The merged properties map should omit keys with null values", merged.containsValue(null));
+ Assertions.assertThat(merged)
+ .as(
+ "The merged properties map should use values from defaults, then client config, and finally overrides")
+ .isEqualTo(expected);
+ Assertions.assertThat(merged.containsValue(null))
Review Comment:
should be `assertThat(merged).containsValue(..)` instead of `.isTrue()` / `.isFalse()`
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestUpdateNamespacePropertiesResponse.java:
##########
@@ -145,92 +143,98 @@ public void testDeserializeInvalidResponse() {
// Invalid top-level types
String jsonInvalidTypeOnRemovedField =
"{\"removed\":{\"foo\":true},\"updated\":[\"owner\"],\"missing\":[\"bar\"]}";
- AssertHelpers.assertThrows(
- "A JSON response with an invalid type for one of the fields should fail to parse",
- JsonProcessingException.class,
- () -> deserialize(jsonInvalidTypeOnRemovedField));
+ Assertions.assertThatThrownBy(() -> deserialize(jsonInvalidTypeOnRemovedField))
+ .as("A JSON response with an invalid type for one of the fields should fail to parse")
+ .isInstanceOf(JsonProcessingException.class);
String jsonInvalidTypeOnUpdatedField = "{\"updated\":\"owner\",\"missing\":[\"bar\"]}";
- AssertHelpers.assertThrows(
- "A JSON response with an invalid type for one of the fields should fail to parse",
- JsonProcessingException.class,
- () -> deserialize(jsonInvalidTypeOnUpdatedField));
+ Assertions.assertThatThrownBy(() -> deserialize(jsonInvalidTypeOnUpdatedField))
+ .as("A JSON response with an invalid type for one of the fields should fail to parse")
+ .isInstanceOf(JsonProcessingException.class);
// Valid top-level (array) types, but at least one entry in the list is not the expected type
String jsonInvalidValueOfTypeIntNestedInRemovedList =
"{\"removed\":[\"foo\", \"bar\", 123456], ,\"updated\":[\"owner\"],\"missing\":[\"bar\"]}";
- AssertHelpers.assertThrows(
- "A JSON response with an invalid type inside one of the list fields should fail to deserialize",
- JsonProcessingException.class,
- () -> deserialize(jsonInvalidValueOfTypeIntNestedInRemovedList));
+ Assertions.assertThatThrownBy(() -> deserialize(jsonInvalidValueOfTypeIntNestedInRemovedList))
+ .as(
+ "A JSON response with an invalid type inside one of the list fields should fail to deserialize")
+ .isInstanceOf(JsonProcessingException.class);
// Exception comes from Jackson
- AssertHelpers.assertThrows(
- "A null JSON response body should fail to deserialize",
- IllegalArgumentException.class,
- () -> deserialize(null));
+ Assertions.assertThatThrownBy(() -> deserialize(null))
+ .as("A null JSON response body should fail to deserialize")
+ .isInstanceOf(IllegalArgumentException.class);
Review Comment:
hasMessage()
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestCreateNamespaceResponse.java:
##########
@@ -165,8 +163,12 @@ public CreateNamespaceResponse createExampleInstance() {
@Override
public void assertEquals(CreateNamespaceResponse actual, CreateNamespaceResponse expected) {
- Assert.assertEquals("Namespaces should be equal", actual.namespace(), expected.namespace());
- Assert.assertEquals("Properties should be equal", actual.properties(), expected.properties());
+ Assertions.assertThat(actual.namespace())
+ .as("Namespaces should be equal")
Review Comment:
I don't think the `.as()` provides any additional help during debugging, so we can omit it here
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestCreateNamespaceResponse.java:
##########
@@ -85,69 +84,68 @@ public void testCanDeserializeWithoutDefaultValues() throws JsonProcessingExcept
public void testDeserializeInvalidResponse() {
String jsonResponseMalformedNamespaceValue =
"{\"namespace\":\"accounting%1Ftax\",\"properties\":null}";
- AssertHelpers.assertThrows(
- "A JSON response with the wrong type for the namespace field should fail to deserialize",
- JsonProcessingException.class,
- "Cannot parse string array from non-array",
- () -> deserialize(jsonResponseMalformedNamespaceValue));
+ Assertions.assertThatThrownBy(() -> deserialize(jsonResponseMalformedNamespaceValue))
+ .as(
+ "A JSON response with the wrong type for the namespace field should fail to deserialize")
+ .isInstanceOf(JsonProcessingException.class)
+ .hasMessageContaining("Cannot parse string array from non-array");
String jsonResponsePropertiesHasWrongType =
"{\"namespace\":[\"accounting\",\"tax\"],\"properties\":[]}";
- AssertHelpers.assertThrows(
- "A JSON response with the wrong type for the properties field should fail to deserialize",
- JsonProcessingException.class,
- () -> deserialize(jsonResponsePropertiesHasWrongType));
+ Assertions.assertThatThrownBy(() -> deserialize(jsonResponsePropertiesHasWrongType))
+ .as(
+ "A JSON response with the wrong type for the properties field should fail to deserialize")
+ .isInstanceOf(JsonProcessingException.class);
- AssertHelpers.assertThrows(
- "An empty JSON response should fail to deserialize",
- IllegalArgumentException.class,
- "Invalid namespace: null",
- () -> deserialize("{}"));
+ Assertions.assertThatThrownBy(() -> deserialize("{}"))
+ .as("An empty JSON response should fail to deserialize")
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Invalid namespace: null");
String jsonMisspelledKeys =
"{\"namepsace\":[\"accounting\",\"tax\"],\"propertiezzzz\":{\"owner\":\"Hank\"}}";
- AssertHelpers.assertThrows(
- "A JSON response with the keys spelled incorrectly should fail to deserialize and validate",
- IllegalArgumentException.class,
- "Invalid namespace: null",
- () -> deserialize(jsonMisspelledKeys));
-
- AssertHelpers.assertThrows(
- "A null JSON response body should fail to deserialize",
- IllegalArgumentException.class,
- () -> deserialize(null));
+ Assertions.assertThatThrownBy(() -> deserialize(jsonMisspelledKeys))
+ .as(
+ "A JSON response with the keys spelled incorrectly should fail to deserialize and validate")
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Invalid namespace: null");
+
+ Assertions.assertThatThrownBy(() -> deserialize(null))
+ .as("A null JSON response body should fail to deserialize")
+ .isInstanceOf(IllegalArgumentException.class);
Review Comment:
hasMessage(..) is missing
##########
core/src/test/java/org/apache/iceberg/rest/responses/TestGetNamespaceResponse.java:
##########
@@ -67,71 +66,64 @@ public void testCanDeserializeWithoutDefaultValues() throws JsonProcessingExcept
@Test
public void testDeserializeInvalidResponse() {
String jsonNamespaceHasWrongType = "{\"namespace\":\"accounting%1Ftax\",\"properties\":null}";
- AssertHelpers.assertThrows(
- "A JSON response with the wrong type for a field should fail to deserialize",
- JsonProcessingException.class,
- "Cannot parse string array from non-array",
- () -> deserialize(jsonNamespaceHasWrongType));
+ Assertions.assertThatThrownBy(() -> deserialize(jsonNamespaceHasWrongType))
+ .as("A JSON response with the wrong type for a field should fail to deserialize")
+ .isInstanceOf(JsonProcessingException.class)
+ .hasMessageContaining("Cannot parse string array from non-array");
String jsonPropertiesHasWrongType =
"{\"namespace\":[\"accounting\",\"tax\"],\"properties\":[]}";
- AssertHelpers.assertThrows(
- "A JSON response with the wrong type for a field should fail to deserialize",
- JsonProcessingException.class,
- () -> deserialize(jsonPropertiesHasWrongType));
+ Assertions.assertThatThrownBy(() -> deserialize(jsonPropertiesHasWrongType))
+ .as("A JSON response with the wrong type for a field should fail to deserialize")
+ .isInstanceOf(JsonProcessingException.class);
Review Comment:
hasMessage(..) here and in other files is missing
--
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