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/05/02 08:32:22 UTC
[GitHub] [iceberg] nastra commented on a diff in pull request #7468: API: Remove deprecated AssertHelpers usage
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: 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