nastra commented on code in PR #7468:
URL: https://github.com/apache/iceberg/pull/7468#discussion_r1182186343
##########
api/src/test/java/org/apache/iceberg/TestSnapshotRef.java:
##########
@@ -67,52 +68,39 @@ public void testBranchWithOverride() {
@Test
public void testNoTypeFailure() {
- AssertHelpers.assertThrows(
- "Snapshot reference type must be specified",
- IllegalArgumentException.class,
- "Snapshot reference type must not be null",
- () -> SnapshotRef.builderFor(1L, null).build());
+ Assertions.assertThatThrownBy(() -> SnapshotRef.builderFor(1L,
null).build())
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Snapshot reference type must not be null");
Review Comment:
if possible we should prefer `.hasMessage(..)` where possible. For example,
here the exact error msg ends up being `Snapshot reference type must not be
null`, so there's no need to use `hasMessageContaining()`.
##########
api/src/test/java/org/apache/iceberg/io/TestCloseableIterable.java:
##########
@@ -91,10 +90,9 @@ public void testConcatWithEmptyIterables() {
// This will throw a NoSuchElementException
CloseableIterable<Integer> concat5 =
CloseableIterable.concat(Lists.newArrayList(empty, empty, empty));
- AssertHelpers.assertThrows(
- "should throw NoSuchElementException",
- NoSuchElementException.class,
- () -> Iterables.getLast(concat5));
+
+ Assertions.assertThatThrownBy(() -> Iterables.getLast(concat5))
+ .isInstanceOf(NoSuchElementException.class);
Review Comment:
would be good to check for the specific error msg here
##########
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java:
##########
@@ -731,46 +710,32 @@ public void testNotIn() {
@Test
public void testNotInExceptions() {
- AssertHelpers.assertThrows(
- "Throw exception if value is null",
- NullPointerException.class,
- "Cannot create expression literal from null",
- () -> notIn("x", (Literal) null));
-
- AssertHelpers.assertThrows(
- "Throw exception if value is null",
- NullPointerException.class,
- "Values cannot be null for NOT_IN predicate",
- () -> notIn("x", (Collection<?>) null));
-
- AssertHelpers.assertThrows(
- "Throw exception if calling literal() for IN predicate",
- IllegalArgumentException.class,
- "NOT_IN predicate cannot return a literal",
- () -> notIn("x", 5, 6).literal());
-
- AssertHelpers.assertThrows(
- "Throw exception if any value in the input is null",
- NullPointerException.class,
- "Cannot create expression literal from null",
- () -> notIn("x", 1, 2, null));
-
- AssertHelpers.assertThrows(
- "Throw exception if binding fails for any element in the set",
- ValidationException.class,
- "Invalid value for conversion to type int",
- () -> new Evaluator(STRUCT, notIn("x", 7, 8, 9.1)));
-
- AssertHelpers.assertThrows(
- "Throw exception if no input value",
- IllegalArgumentException.class,
- "Cannot create NOT_IN predicate without a value",
- () -> predicate(Expression.Operation.NOT_IN, "x"));
-
- AssertHelpers.assertThrows(
- "Implicit conversion NOT_IN to NOT_EQ and throw exception if binding
fails",
- ValidationException.class,
- "Invalid value for conversion to type int",
- () -> new Evaluator(STRUCT, predicate(Expression.Operation.NOT_IN,
"x", 5.1)));
+ Assertions.assertThatThrownBy(() -> notIn("x", (Literal) null))
+ .isInstanceOf(NullPointerException.class)
+ .hasMessageContaining("Cannot create expression literal from null");
+ Assertions.assertThatThrownBy(() -> notIn("x", (Collection<?>) null))
Review Comment:
newline missing
##########
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java:
##########
@@ -624,47 +622,28 @@ public void testIn() {
@Test
public void testInExceptions() {
- AssertHelpers.assertThrows(
- "Throw exception if value is null",
- NullPointerException.class,
- "Cannot create expression literal from null",
- () -> in("x", (Literal) null));
-
- AssertHelpers.assertThrows(
- "Throw exception if value is null",
- NullPointerException.class,
- "Values cannot be null for IN predicate",
- () -> in("x", (Collection<?>) null));
-
- AssertHelpers.assertThrows(
- "Throw exception if calling literal() for IN predicate",
- IllegalArgumentException.class,
- "IN predicate cannot return a literal",
- () -> in("x", 5, 6).literal());
-
- AssertHelpers.assertThrows(
- "Throw exception if any value in the input is null",
- NullPointerException.class,
- "Cannot create expression literal from null",
- () -> in("x", 1, 2, null));
-
- AssertHelpers.assertThrows(
- "Throw exception if binding fails for any element in the set",
- ValidationException.class,
- "Invalid value for conversion to type int",
- () -> new Evaluator(STRUCT, in("x", 7, 8, 9.1)));
-
- AssertHelpers.assertThrows(
- "Throw exception if no input value",
- IllegalArgumentException.class,
- "Cannot create IN predicate without a value",
- () -> predicate(Expression.Operation.IN, "x"));
-
- AssertHelpers.assertThrows(
- "Implicit conversion IN to EQ and throw exception if binding fails",
- ValidationException.class,
- "Invalid value for conversion to type int",
- () -> new Evaluator(STRUCT, predicate(Expression.Operation.IN, "x",
5.1)));
+ Assertions.assertThatThrownBy(() -> in("x", (Literal) null))
+ .isInstanceOf(NullPointerException.class)
+ .hasMessageContaining("Cannot create expression literal from null");
Review Comment:
better to add a newline after each statement and would also be good to
double-check whether we can use `.hasMessage()` here everywhere
##########
api/src/test/java/org/apache/iceberg/expressions/TestExpressionHelpers.java:
##########
@@ -174,20 +174,16 @@ public void testTransformExpressions() {
@Test
public void testNullName() {
- AssertHelpers.assertThrows(
- "Should catch null column names when creating expressions",
- NullPointerException.class,
- "Name cannot be null",
- () -> equal((String) null, 5));
+ Assertions.assertThatThrownBy(() -> equal((String) null, 5))
+ .isInstanceOf(NullPointerException.class)
+ .hasMessageContaining("Name cannot be null");
Review Comment:
`.hasMessage()`
##########
api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java:
##########
@@ -450,20 +449,14 @@ public void testProjectListNested() {
required(17, "x", Types.IntegerType.get()),
required(18, "y",
Types.IntegerType.get()))))))));
- AssertHelpers.assertThrows(
- "Cannot explicitly project List",
- IllegalArgumentException.class,
- () -> TypeUtil.project(schema, Sets.newHashSet(12)));
+ Assertions.assertThatThrownBy(() -> TypeUtil.project(schema,
Sets.newHashSet(12)))
+ .isInstanceOf(IllegalArgumentException.class);
Review Comment:
these should all check for a specific error msg, as otherwise the called
code could fail for a different reason that is being expected
##########
api/src/test/java/org/apache/iceberg/transforms/TestBucketing.java:
##########
@@ -358,11 +358,9 @@ public void testUUIDHash() {
@Test
public void testVerifiedIllegalNumBuckets() {
- AssertHelpers.assertThrows(
- "Should fail if numBucket is less than or equal to zero",
- IllegalArgumentException.class,
- "Invalid number of buckets: 0 (must be > 0)",
- () -> Bucket.get(0));
+ Assertions.assertThatThrownBy(() -> Bucket.get(0))
+ .isInstanceOf(IllegalArgumentException.class)
+ .hasMessageContaining("Invalid number of buckets: 0 (must be > 0)");
Review Comment:
`.hasMessage()`
--
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]
