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