Skip to content

Replace Assert.fail usage with AssertJ fluent testing #6029

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rdblue merged 1 commit into from
Oct 21, 2022
Merged

Replace Assert.fail usage with AssertJ fluent testing #6029

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 8 additions & 6 deletions aliyun/src/test/java/org/apache/iceberg/aliyun/oss/mock/TestLocalAliyunOSS.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import com.aliyun.oss.OSS;
import com.aliyun.oss.OSSErrorCode;
import com.aliyun.oss.OSSException;
import com.aliyun.oss.ServiceException;
import com.aliyun.oss.model.GetObjectRequest;
import com.aliyun.oss.model.PutObjectResult;
import java.io.ByteArrayInputStream;
Expand All @@ -32,6 +33,8 @@
import org.apache.iceberg.aliyun.TestUtility;
import org.apache.iceberg.aliyun.oss.AliyunOSSTestRule;
import org.apache.iceberg.relocated.com.google.common.io.ByteStreams;
import org.assertj.core.api.Assertions;
import org.assertj.core.api.InstanceOfAssertFactories;
import org.junit.After;
import org.junit.Assert;
import org.junit.Assume;
Expand All @@ -48,12 +51,11 @@ public class TestLocalAliyunOSS {
private final Random random = new Random(1);

private static void assertThrows(Runnable runnable, String expectedErrorCode) {
try {
runnable.run();
Assert.fail("No exception was thrown, expected errorCode: " + expectedErrorCode);
} catch (OSSException e) {
Assert.assertEquals(expectedErrorCode, e.getErrorCode());
}
Assertions.assertThatThrownBy(runnable::run)
.isInstanceOf(OSSException.class)
.asInstanceOf(InstanceOfAssertFactories.type(OSSException.class))
.extracting(ServiceException::getErrorCode)
.isEqualTo(expectedErrorCode);
}

@Before
Expand Down
13 changes: 7 additions & 6 deletions api/src/test/java/org/apache/iceberg/TestHelpers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.apache.iceberg.expressions.ExpressionVisitors;
import org.apache.iceberg.expressions.UnboundPredicate;
import org.apache.iceberg.util.ByteBuffers;
import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.objenesis.strategy.StdInstantiatorStrategy;

Expand Down Expand Up @@ -100,9 +101,9 @@ public static <T> T roundTripSerialize(T type) throws IOException, ClassNotFound
}

public static void assertSameSchemaList(List<Schema> list1, List<Schema> list2) {
if (list1.size() != list2.size()) {
Assert.fail("Should have same number of schemas in both lists");
}
Assertions.assertThat(list1)
.as("Should have same number of schemas in both lists")
.hasSameSizeAs(list2);

IntStream.range(0, list1.size())
.forEach(
Expand Down Expand Up @@ -137,9 +138,9 @@ public static void assertSerializedAndLoadedMetadata(Table expected, Table actua
}

public static void assertSameSchemaMap(Map<Integer, Schema> map1, Map<Integer, Schema> map2) {
if (map1.size() != map2.size()) {
Assert.fail("Should have same number of schemas in both maps");
}
Assertions.assertThat(map1)
.as("Should have same number of schemas in both maps")
.hasSameSizeAs(map2);

map1.forEach(
(schemaId, schema1) -> {
Expand Down
14 changes: 6 additions & 8 deletions api/src/test/java/org/apache/iceberg/expressions/TestExpressionUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,12 +558,8 @@ public void testSelectsPartitions() {
}

private void assertEquals(Expression expected, Expression actual) {
if (expected instanceof UnboundPredicate) {
Assert.assertTrue("Should be an UnboundPredicate", actual instanceof UnboundPredicate);
assertEquals((UnboundPredicate<?>) expected, (UnboundPredicate<?>) actual);
} else {
Assert.fail("Unknown expected expression: " + expected);
}
Assertions.assertThat(expected).isInstanceOf(UnboundPredicate.class);
assertEquals((UnboundPredicate<?>) expected, (UnboundPredicate<?>) actual);
}

private void assertEquals(UnboundPredicate<?> expected, UnboundPredicate<?> actual) {
Expand All @@ -573,14 +569,16 @@ private void assertEquals(UnboundPredicate<?> expected, UnboundPredicate<?> actu
}

private void assertEquals(UnboundTerm<?> expected, UnboundTerm<?> actual) {
Assertions.assertThat(expected)
.as("Unknown expected term: " + expected)
.isOfAnyClassIn(NamedReference.class, UnboundTransform.class);

if (expected instanceof NamedReference) {
Assert.assertTrue("Should be a NamedReference", actual instanceof NamedReference);
assertEquals((NamedReference<?>) expected, (NamedReference<?>) actual);
} else if (expected instanceof UnboundTransform) {
Assert.assertTrue("Should be an UnboundTransform", actual instanceof UnboundTransform);
assertEquals((UnboundTransform<?, ?>) expected, (UnboundTransform<?, ?>) actual);
} else {
Assert.fail("Unknown expected term: " + expected);
}
}

Expand Down
31 changes: 7 additions & 24 deletions api/src/test/java/org/apache/iceberg/expressions/TestPredicateBinding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
import org.apache.iceberg.exceptions.ValidationException;
import org.apache.iceberg.types.Types;
import org.apache.iceberg.types.Types.StructType;
import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test;

Expand All @@ -54,7 +55,6 @@ public class TestPredicateBinding {
Arrays.asList(LT, LT_EQ, GT, GT_EQ, EQ, NOT_EQ);

@Test
@SuppressWarnings("unchecked")
public void testMultipleFields() {
StructType struct =
StructType.of(
Expand All @@ -81,18 +81,12 @@ public void testMissingField() {
StructType struct = StructType.of(required(13, "x", Types.IntegerType.get()));

UnboundPredicate<Integer> unbound = new UnboundPredicate<>(LT, ref("missing"), 6);
try {
unbound.bind(struct);
Assert.fail("Binding a missing field should fail");
} catch (ValidationException e) {
Assert.assertTrue(
"Validation should complain about missing field",
e.getMessage().contains("Cannot find field 'missing' in struct:"));
}
Assertions.assertThatThrownBy(() -> unbound.bind(struct))
Copy link
Contributor

@rdblue rdblue Oct 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, this was so old it predated adding the assertThrows methods!

.isInstanceOf(ValidationException.class)
.hasMessageContaining("Cannot find field 'missing' in struct:");
}

@Test
@SuppressWarnings("unchecked")
public void testComparisonPredicateBinding() {
StructType struct = StructType.of(required(14, "x", Types.IntegerType.get()));

Expand All @@ -113,7 +107,6 @@ public void testComparisonPredicateBinding() {
}

@Test
@SuppressWarnings("unchecked")
public void testPredicateBindingForStringPrefixComparisons() {
StructType struct = StructType.of(required(17, "x", Types.StringType.get()));

Expand All @@ -132,7 +125,6 @@ public void testPredicateBindingForStringPrefixComparisons() {
}

@Test
@SuppressWarnings("unchecked")
public void testLiteralConversion() {
StructType struct = StructType.of(required(15, "d", Types.DecimalType.of(9, 2)));

Expand All @@ -158,20 +150,13 @@ public void testInvalidConversions() {
for (Expression.Operation op : COMPARISONS) {
UnboundPredicate<String> unbound = new UnboundPredicate<>(op, ref("f"), "12.40");

try {
unbound.bind(struct);
Assert.fail("Should not convert string to float");
} catch (ValidationException e) {
Assert.assertEquals(
"Should ",
e.getMessage(),
"Invalid value for conversion to type float: 12.40 (java.lang.String)");
}
Assertions.assertThatThrownBy(() -> unbound.bind(struct))
.isInstanceOf(ValidationException.class)
.hasMessage("Invalid value for conversion to type float: 12.40 (java.lang.String)");
}
}

@Test
@SuppressWarnings("unchecked")
public void testLongToIntegerConversion() {
StructType struct = StructType.of(required(17, "i", Types.IntegerType.get()));

Expand Down Expand Up @@ -260,7 +245,6 @@ public void testLongToIntegerConversion() {
}

@Test
@SuppressWarnings("unchecked")
public void testDoubleToFloatConversion() {
StructType struct = StructType.of(required(18, "f", Types.FloatType.get()));

Expand Down Expand Up @@ -351,7 +335,6 @@ public void testDoubleToFloatConversion() {
}

@Test
@SuppressWarnings("unchecked")
public void testIsNull() {
StructType optional = StructType.of(optional(19, "s", Types.StringType.get()));

Expand Down
Loading