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/30 07:15:12 UTC
[GitHub] [iceberg] nastra commented on a diff in pull request #7730: Api: Switch iceberg-api tests to Junit5
nastra commented on code in PR #7730:
URL: https://github.com/apache/iceberg/pull/7730#discussion_r1209745291
##########
api/src/test/java/org/apache/iceberg/expressions/TestNumericLiteralConversions.java:
##########
@@ -18,155 +18,153 @@
*/
package org.apache.iceberg.expressions;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.data.Offset.offset;
+
import java.math.BigDecimal;
import java.util.stream.IntStream;
import org.apache.iceberg.types.Types;
-import org.junit.Assert;
-import org.junit.Test;
+import org.junit.jupiter.api.Test;
public class TestNumericLiteralConversions {
@Test
public void testIntegerToLongConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Long> longLit = lit.to(Types.LongType.get());
- Assert.assertEquals("Value should match", 34L, (long) longLit.value());
+ assertThat((long) longLit.value()).as("Value should match").isEqualTo(34L);
}
@Test
public void testIntegerToFloatConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Float> floatLit = lit.to(Types.FloatType.get());
- Assert.assertEquals("Value should match", 34.0F, floatLit.value(), 0.0000000001D);
+ assertThat(floatLit.value()).as("Value should match").isCloseTo(34.0F, offset(0.0000000001F));
}
@Test
public void testIntegerToDoubleConversion() {
Literal<Integer> lit = Literal.of(34);
Literal<Double> doubleLit = lit.to(Types.DoubleType.get());
- Assert.assertEquals("Value should match", 34.0D, doubleLit.value(), 0.0000000001D);
+ assertThat(doubleLit.value()).as("Value should match").isCloseTo(34.0D, offset(0.0000000001D));
}
@Test
public void testIntegerToDecimalConversion() {
Literal<Integer> lit = Literal.of(34);
- Assert.assertEquals(
- "Value should match", new BigDecimal("34"), lit.to(Types.DecimalType.of(9, 0)).value());
- Assert.assertEquals(
- "Value should match", new BigDecimal("34.00"), lit.to(Types.DecimalType.of(9, 2)).value());
- Assert.assertEquals(
- "Value should match",
- new BigDecimal("34.0000"),
- lit.to(Types.DecimalType.of(9, 4)).value());
+ assertThat(lit.to(Types.DecimalType.of(9, 0)).value())
+ .as("Value should match")
Review Comment:
nit: going forward I think we can omit the `.as(..)` for something that's obvious because we always expect the value to match
##########
api/src/test/java/org/apache/iceberg/expressions/TestPredicateBinding.java:
##########
@@ -160,87 +158,81 @@ public void testLongToIntegerConversion() {
StructType struct = StructType.of(required(17, "i", Types.IntegerType.get()));
UnboundPredicate<Long> lt = new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than above max should be alwaysTrue", Expressions.alwaysTrue(), lt.bind(struct));
+ assertThat(lt.bind(struct))
+ .as("Less than above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> lteq =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Less than or equal above max should be alwaysTrue",
- Expressions.alwaysTrue(),
- lteq.bind(struct));
+ assertThat(lteq.bind(struct))
+ .as("Less than or equal above max should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gt = new UnboundPredicate<>(GT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than below min should be alwaysTrue", Expressions.alwaysTrue(), gt.bind(struct));
+ assertThat(gt.bind(struct))
+ .as("Greater than below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gteq =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Greater than or equal below min should be alwaysTrue",
- Expressions.alwaysTrue(),
- gteq.bind(struct));
+ assertThat(gteq.bind(struct))
+ .as("Greater than or equal below min should be alwaysTrue")
+ .isEqualTo(Expressions.alwaysTrue());
UnboundPredicate<Long> gtMax =
new UnboundPredicate<>(GT, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gtMax.bind(struct));
+ assertThat(gtMax.bind(struct))
+ .as("Greater than above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> gteqMax =
new UnboundPredicate<>(GT_EQ, ref("i"), (long) Integer.MAX_VALUE + 1L);
- Assert.assertEquals(
- "Greater than or equal above max should be alwaysFalse",
- Expressions.alwaysFalse(),
- gteqMax.bind(struct));
+ assertThat(gteqMax.bind(struct))
+ .as("Greater than or equal above max should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> ltMin =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than below min should be alwaysFalse", Expressions.alwaysFalse(), ltMin.bind(struct));
+ assertThat(ltMin.bind(struct))
+ .as("Less than below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
UnboundPredicate<Long> lteqMin =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MIN_VALUE - 1L);
- Assert.assertEquals(
- "Less than or equal below min should be alwaysFalse",
- Expressions.alwaysFalse(),
- lteqMin.bind(struct));
+ assertThat(lteqMin.bind(struct))
+ .as("Less than or equal below min should be alwaysFalse")
+ .isEqualTo(Expressions.alwaysFalse());
Expression ltExpr =
new UnboundPredicate<>(LT, ref("i"), (long) Integer.MAX_VALUE).bind(struct, true);
BoundPredicate<Integer> ltMax = assertAndUnwrap(ltExpr);
- Assert.assertTrue("Should be a literal predicate", ltMax.isLiteralPredicate());
- Assert.assertEquals(
- "Should translate bound to Integer",
- (Integer) Integer.MAX_VALUE,
- ltMax.asLiteralPredicate().literal().value());
+ assertThat(ltMax.isLiteralPredicate()).as("Should be a literal predicate").isTrue();
+ assertThat(ltMax.asLiteralPredicate().literal().value())
+ .as("Should translate bound to Integer")
+ .isEqualTo((Integer) Integer.MAX_VALUE);
Expression lteqExpr =
new UnboundPredicate<>(LT_EQ, ref("i"), (long) Integer.MAX_VALUE).bind(struct);
BoundPredicate<Integer> lteqMax = assertAndUnwrap(lteqExpr);
- Assert.assertTrue("Should be a literal predicate", lteqMax.isLiteralPredicate());
- Assert.assertEquals(
- "Should translate bound to Integer",
- (Integer) Integer.MAX_VALUE,
- lteqMax.asLiteralPredicate().literal().value());
+ assertThat(lteqMax.isLiteralPredicate()).as("Should be a literal predicate").isTrue();
+ assertThat(lteqMax.asLiteralPredicate().literal().value())
+ .as("Should translate bound to Integer")
+ .isEqualTo((Integer) Integer.MAX_VALUE);
Review Comment:
I don't think the type casting in all of these assertions within that file are actually required
##########
api/src/test/java/org/apache/iceberg/expressions/TestNumericLiteralConversions.java:
##########
@@ -176,30 +174,36 @@ public void testDecimalToDecimalConversion() {
IntStream.range(0, 10)
.forEach(
scale -> {
- Assert.assertSame(
- "Should return identical object", lit, lit.to(Types.DecimalType.of(9, scale)));
- Assert.assertSame(
- "Should return identical object", lit, lit.to(Types.DecimalType.of(11, scale)));
+ assertThat(lit.to(Types.DecimalType.of(9, scale)))
+ .as("Should return identical object")
+ .isSameAs(lit);
+ assertThat(lit.to(Types.DecimalType.of(11, scale)))
+ .as("Should return identical object")
+ .isSameAs(lit);
});
}
@Test
public void testIntegerToDateConversion() {
Literal<Integer> lit = Literal.of(0);
- Assert.assertEquals(
- "Dates should be equal", lit.to(Types.DateType.get()), new Literals.DateLiteral(0));
+ assertThat(new Literals.DateLiteral(0))
+ .as("Dates should be equal")
Review Comment:
same for this kind of `.as()` message. In a equality comparison we always expect two given values to be equal, so going forward I think it's ok to omit this kind of `.as()` msg
##########
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java:
##########
@@ -654,64 +710,73 @@ public void testInExceptions() {
@Test
public void testNotIn() {
- Assert.assertEquals(3, notIn("s", 7, 8, 9).literals().size());
- Assert.assertEquals(3, notIn("s", 7, 8.1, Long.MAX_VALUE).literals().size());
- Assert.assertEquals(3, notIn("s", "abc", "abd", "abc").literals().size());
- Assert.assertEquals(0, notIn("s").literals().size());
- Assert.assertEquals(1, notIn("s", 5).literals().size());
- Assert.assertEquals(2, notIn("s", 5, 5).literals().size());
- Assert.assertEquals(2, notIn("s", Arrays.asList(5, 5)).literals().size());
- Assert.assertEquals(0, notIn("s", Collections.emptyList()).literals().size());
+ assertThat(notIn("s", 7, 8, 9).literals().size()).isEqualTo(3);
Review Comment:
same as above
##########
api/src/test/java/org/apache/iceberg/expressions/TestEvaluator.java:
##########
@@ -552,72 +599,81 @@ public void testCaseSensitiveNot() {
public void testCharSeqValue() {
StructType struct = StructType.of(required(34, "s", Types.StringType.get()));
Evaluator evaluator = new Evaluator(struct, equal("s", "abc"));
- Assert.assertTrue(
- "string(abc) == utf8(abc) => true", evaluator.eval(TestHelpers.Row.of(new Utf8("abc"))));
- Assert.assertFalse(
- "string(abc) == utf8(abcd) => false", evaluator.eval(TestHelpers.Row.of(new Utf8("abcd"))));
+ assertThat(evaluator.eval(TestHelpers.Row.of(new Utf8("abc"))))
+ .as("string(abc) == utf8(abc) => true")
+ .isTrue();
+ assertThat(evaluator.eval(TestHelpers.Row.of(new Utf8("abcd"))))
+ .as("string(abc) == utf8(abcd) => false")
+ .isFalse();
}
@Test
public void testIn() {
- Assert.assertEquals(3, in("s", 7, 8, 9).literals().size());
- Assert.assertEquals(3, in("s", 7, 8.1, Long.MAX_VALUE).literals().size());
- Assert.assertEquals(3, in("s", "abc", "abd", "abc").literals().size());
- Assert.assertEquals(0, in("s").literals().size());
- Assert.assertEquals(1, in("s", 5).literals().size());
- Assert.assertEquals(2, in("s", 5, 5).literals().size());
- Assert.assertEquals(2, in("s", Arrays.asList(5, 5)).literals().size());
- Assert.assertEquals(0, in("s", Collections.emptyList()).literals().size());
+ assertThat(in("s", 7, 8, 9).literals().size()).isEqualTo(3);
Review Comment:
these all can be switched to `assertThat(in(...).literals()).hasSize(...)` as that will show the content of `literals()` when the check ever fails
--
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