Skip to content

Prevent usage of @Test(expected = ...) and change existing tests #5221

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
danielcweeks merged 1 commit into from
Jul 19, 2022
Merged

Prevent usage of @Test(expected = ...) and change existing tests #5221

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
5 changes: 5 additions & 0 deletions .baseline/checkstyle/checkstyle.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,6 +410,11 @@
<property name="format" value="(void setUp\(\))|(void setup\(\))|(void setupStatic\(\))|(void setUpStatic\(\))|(void beforeTest\(\))|(void teardown\(\))|(void tearDown\(\))|(void beforeStatic\(\))|(void afterStatic\(\))"/>
<property name="message" value="Test setup/teardown methods are called before(), beforeClass(), after(), afterClass(), but not setUp, teardown, etc."/>
</module>
<module name="RegexpSinglelineJava">
<property name="ignoreComments" value="true"/>
<property name="format" value="@Test\(.*expected.*\)"/>
<property name="message" value="Prefer using Assertions.assertThatThrownBy(...).isInstanceOf(...) instead."/>
Copy link
Member

@RussellSpitzer RussellSpitzer Jul 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We actually have a helper for this in AssertHelpers

/**
   * A convenience method to avoid a large number of @Test(expected=...) tests
   * @param message A String message to describe this assertion
   * @param expected An Exception class that the Runnable should throw
   * @param containedInMessage A String that should be contained by the thrown
   *                           exception's message
   * @param runnable A Runnable that is expected to throw the runtime exception
   */
  public static void assertThrows(String message,
                                  Class<? extends Exception> expected,
                                  String containedInMessage,
                                  Runnable runnable) {
                                  ```

Copy link
Contributor Author

@nastra nastra Jul 11, 2022

Choose a reason for hiding this comment

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

I know but direct Assertions usage gives you much more flexibility around exception verification

Copy link
Member

@RussellSpitzer RussellSpitzer Jul 11, 2022

Choose a reason for hiding this comment

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

I think we would probably want to change all the instances in the code base to that then, rather than have two ways to do it.

Copy link
Contributor Author

@nastra nastra Jul 12, 2022

Choose a reason for hiding this comment

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

looks like there are about 800 usages of different variations of assertThrows in the codebase, so not sure we'd want to update all of those. Maybe we'll just mention in the checkstyle message Prefer using AssertHelper.assertThrows(...) or Assertions.assertThatThrownBy(...).isInstanceOf(...) instead. I believe both cases should be valid, where Assertions.assertThatThrownBy(...) just gives you more flexibility over things

Copy link
Contributor

@rdblue rdblue Jul 13, 2022

Choose a reason for hiding this comment

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

assertThrows was used before we were using Assertions. It's pretty old. I think it's fine to go either way, with a slight preference for using Assertions since it is easier to read. The main thing to watch out for is that the exception message is validated as well as the type.

Copy link
Contributor Author

@nastra nastra Jul 13, 2022

Choose a reason for hiding this comment

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

I'm in favor of steering people towards Assertions usage due to greater flexibility and more fluent assertion checks. In terms of checking the exception message, I think this is a good practice in general, since you really want to make sure you're getting the correct exception with the message you're expecting

Copy link
Contributor

@dimas-b dimas-b Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

From my POV, Assertions statements are pretty intuitive to read and interpret and in most cases result in very few lines of code. +1 to promoting them.

</module>
<module name="RightCurly"> <!-- Java Style Guide: Nonempty blocks: K & R style -->
<property name="option" value="same"/>
<property name="tokens" value="LITERAL_TRY, LITERAL_CATCH, LITERAL_FINALLY, LITERAL_IF, LITERAL_ELSE, LITERAL_DO"/>
Expand Down
22 changes: 11 additions & 11 deletions api/src/test/java/org/apache/iceberg/expressions/TestExpressionBinding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,19 +51,17 @@ public class TestExpressionBinding {
@Test
public void testMissingReference() {
Expression expr = and(equal("t", 5), equal("x", 7));
try {
Binder.bind(STRUCT, expr);
Assert.fail("Should not successfully bind to struct without field 't'");
} catch (ValidationException e) {
Assert.assertTrue("Should complain about missing field",
e.getMessage().contains("Cannot find field 't' in struct:"));
}
Assertions.assertThatThrownBy(() -> Binder.bind(STRUCT, expr))
.isInstanceOf(ValidationException.class)
.hasMessageContaining("Cannot find field 't' in struct");
}

@Test(expected = IllegalStateException.class)
@Test
public void testBoundExpressionFails() {
Expression expr = not(equal("x", 7));
Binder.bind(STRUCT, Binder.bind(STRUCT, expr));
Assertions.assertThatThrownBy(() -> Binder.bind(STRUCT, Binder.bind(STRUCT, expr)))
.isInstanceOf(IllegalStateException.class)
.hasMessageContaining("Found already bound predicate");
}

@Test
Expand All @@ -78,10 +76,12 @@ public void testCaseInsensitiveReference() {
TestHelpers.assertAllReferencesBound("Single reference", Binder.bind(STRUCT, expr, false));
}

@Test(expected = ValidationException.class)
@Test
public void testCaseSensitiveReference() {
Expression expr = not(equal("X", 7));
Binder.bind(STRUCT, expr, true);
Assertions.assertThatThrownBy(() -> Binder.bind(STRUCT, expr, true))
.isInstanceOf(ValidationException.class)
.hasMessageContaining("Cannot find field 'X' in struct");
}

@Test
Expand Down
13 changes: 9 additions & 4 deletions api/src/test/java/org/apache/iceberg/expressions/TestStringLiteralConversions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.apache.avro.Schema;
import org.apache.avro.data.TimeConversions;
import org.apache.iceberg.types.Types;
import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test;

Expand Down Expand Up @@ -159,18 +160,22 @@ public void testNegativeStringToTimestampLiteral() {

}

@Test(expected = DateTimeException.class)
@Test
public void testTimestampWithZoneWithoutZoneInLiteral() {
// Zone must be present in literals when converting to timestamp with zone
Literal<CharSequence> timestampStr = Literal.of("2017-08-18T14:21:01.919");
timestampStr.to(Types.TimestampType.withZone());
Assertions.assertThatThrownBy(() -> timestampStr.to(Types.TimestampType.withZone()))
.isInstanceOf(DateTimeException.class)
.hasMessageContaining("could not be parsed");
}

@Test(expected = DateTimeException.class)
@Test
public void testTimestampWithoutZoneWithZoneInLiteral() {
// Zone must not be present in literals when converting to timestamp without zone
Literal<CharSequence> timestampStr = Literal.of("2017-08-18T14:21:01.919+07:00");
timestampStr.to(Types.TimestampType.withoutZone());
Assertions.assertThatThrownBy(() -> timestampStr.to(Types.TimestampType.withoutZone()))
.isInstanceOf(DateTimeException.class)
.hasMessageContaining("could not be parsed");
}

@Test
Expand Down
7 changes: 5 additions & 2 deletions api/src/test/java/org/apache/iceberg/transforms/TestResiduals.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import org.apache.iceberg.expressions.ResidualEvaluator;
import org.apache.iceberg.expressions.UnboundPredicate;
import org.apache.iceberg.types.Types;
import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test;

Expand Down Expand Up @@ -129,7 +130,7 @@ public void testCaseInsensitiveIdentityTransformResiduals() {
Assert.assertEquals("Residual should be alwaysFalse", alwaysFalse(), residual);
}

@Test(expected = ValidationException.class)
@Test
public void testCaseSensitiveIdentityTransformResiduals() {
Schema schema = new Schema(
Types.NestedField.optional(50, "dateint", Types.IntegerType.get()),
Expand All @@ -142,7 +143,9 @@ public void testCaseSensitiveIdentityTransformResiduals() {

ResidualEvaluator resEval = ResidualEvaluator.of(spec, lessThan("DATEINT", 20170815), true);

resEval.residualFor(Row.of(20170815));
Assertions.assertThatThrownBy(() -> resEval.residualFor(Row.of(20170815)))
.isInstanceOf(ValidationException.class)
.hasMessageContaining("Cannot find field 'DATEINT' in struct");
}

@Test
Expand Down
13 changes: 9 additions & 4 deletions api/src/test/java/org/apache/iceberg/types/TestTypeUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.Lists;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.types.Types.IntegerType;
import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test;

Expand Down Expand Up @@ -427,7 +428,7 @@ public void testProjectMapNested() {
Assert.assertEquals(expected.asStruct(), actual.asStruct());
}

@Test(expected = IllegalArgumentException.class)
@Test
public void testReassignIdsIllegalArgumentException() {
Schema schema = new Schema(
required(1, "a", Types.IntegerType.get()),
Expand All @@ -436,10 +437,12 @@ public void testReassignIdsIllegalArgumentException() {
Schema sourceSchema = new Schema(
required(1, "a", Types.IntegerType.get())
);
TypeUtil.reassignIds(schema, sourceSchema);
Assertions.assertThatThrownBy(() -> TypeUtil.reassignIds(schema, sourceSchema))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Field b not found in source schema");
}

@Test(expected = RuntimeException.class)
@Test
public void testValidateSchemaViaIndexByName() {
Types.NestedField nestedType = Types.NestedField
.required(1, "a", Types.StructType.of(
Expand All @@ -450,7 +453,9 @@ public void testValidateSchemaViaIndexByName() {
)
);

TypeUtil.indexByName(Types.StructType.of(nestedType));
Assertions.assertThatThrownBy(() -> TypeUtil.indexByName(Types.StructType.of(nestedType)))
.isInstanceOf(RuntimeException.class)
.hasMessageContaining("Invalid schema: multiple fields for name a.b.c");
}

@Test
Expand Down
7 changes: 5 additions & 2 deletions arrow/src/test/java/org/apache/iceberg/arrow/vectorized/parquet/DecimalVectorUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
package org.apache.iceberg.arrow.vectorized.parquet;

import java.math.BigInteger;
import org.assertj.core.api.Assertions;
import org.junit.Test;

import static org.junit.Assert.assertEquals;
Expand Down Expand Up @@ -65,9 +66,11 @@ public void testPadBigEndianBytesZero() {
assertEquals(BigInteger.ZERO, result);
}

@Test(expected = IllegalArgumentException.class)
@Test
public void testPadBigEndianBytesOverflow() {
byte[] bytes = new byte[17];
DecimalVectorUtil.padBigEndianBytes(bytes, 16);
Assertions.assertThatThrownBy(() -> DecimalVectorUtil.padBigEndianBytes(bytes, 16))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Buffer size of 17 is larger than requested size of 16");
}
}
4 changes: 2 additions & 2 deletions core/src/main/java/org/apache/iceberg/hadoop/HadoopMetricsContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ public void initialize(Map<String, String> properties) {
}

/**
* The Hadoop implementation delegates to the Hadoop delegates to the
* FileSystem.Statistics implementation and therefore does not require
* The Hadoop implementation delegates to the FileSystem.Statistics
* implementation and therefore does not require
* support for operations like unit() and count() as the counter
* values are not directly consumed.
*
Expand Down
37 changes: 25 additions & 12 deletions data/src/test/java/org/apache/iceberg/data/avro/TestSingleMessageEncoding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.Ordering;
import org.apache.iceberg.relocated.com.google.common.collect.Sets;
import org.apache.iceberg.types.Types;
import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Test;

Expand Down Expand Up @@ -132,14 +133,16 @@ public void testSchemaEvolution() throws Exception {
Assert.assertEquals(allAsV2, decodedUsingV2);
}

@Test(expected = MissingSchemaException.class)
@Test
public void testCompatibleReadFailsWithoutSchema() throws Exception {
MessageEncoder<Record> v1Encoder = new IcebergEncoder<>(SCHEMA_V1);
MessageDecoder<Record> v2Decoder = new IcebergDecoder<>(SCHEMA_V2);

ByteBuffer v1Buffer = v1Encoder.encode(V1_RECORDS.get(3));

v2Decoder.decode(v1Buffer);
Assertions.assertThatThrownBy(() -> v2Decoder.decode(v1Buffer))
.isInstanceOf(MissingSchemaException.class)
.hasMessageContaining("Cannot resolve schema for fingerprint");
}

@Test
Expand Down Expand Up @@ -202,7 +205,7 @@ public void testBufferCopy() throws Exception {
V1_RECORDS.get(0), decoder.decode(b0));
}

@Test(expected = AvroRuntimeException.class)
@Test
public void testByteBufferMissingPayload() throws Exception {
MessageEncoder<Record> encoder = new IcebergEncoder<>(SCHEMA_V2);
MessageDecoder<Record> decoder = new IcebergDecoder<>(SCHEMA_V2);
Expand All @@ -211,10 +214,12 @@ public void testByteBufferMissingPayload() throws Exception {

buffer.limit(12);

decoder.decode(buffer);
Assertions.assertThatThrownBy(() -> decoder.decode(buffer))
.isInstanceOf(AvroRuntimeException.class)
.hasMessage("Decoding datum failed");
}

@Test(expected = BadHeaderException.class)
@Test
public void testByteBufferMissingFullHeader() throws Exception {
MessageEncoder<Record> encoder = new IcebergEncoder<>(SCHEMA_V2);
MessageDecoder<Record> decoder = new IcebergDecoder<>(SCHEMA_V2);
Expand All @@ -223,39 +228,47 @@ public void testByteBufferMissingFullHeader() throws Exception {

buffer.limit(8);

decoder.decode(buffer);
Assertions.assertThatThrownBy(() -> decoder.decode(buffer))
.isInstanceOf(BadHeaderException.class)
.hasMessage("Not enough header bytes");
}

@Test(expected = BadHeaderException.class)
@Test
public void testByteBufferBadMarkerByte() throws Exception {
MessageEncoder<Record> encoder = new IcebergEncoder<>(SCHEMA_V2);
MessageDecoder<Record> decoder = new IcebergDecoder<>(SCHEMA_V2);

ByteBuffer buffer = encoder.encode(V2_RECORDS.get(0));
buffer.array()[0] = 0x00;

decoder.decode(buffer);
Assertions.assertThatThrownBy(() -> decoder.decode(buffer))
.isInstanceOf(BadHeaderException.class)
.hasMessageContaining("Unrecognized header bytes");
}

@Test(expected = BadHeaderException.class)
@Test
public void testByteBufferBadVersionByte() throws Exception {
MessageEncoder<Record> encoder = new IcebergEncoder<>(SCHEMA_V2);
MessageDecoder<Record> decoder = new IcebergDecoder<>(SCHEMA_V2);

ByteBuffer buffer = encoder.encode(V2_RECORDS.get(0));
buffer.array()[1] = 0x00;

decoder.decode(buffer);
Assertions.assertThatThrownBy(() -> decoder.decode(buffer))
.isInstanceOf(BadHeaderException.class)
.hasMessageContaining("Unrecognized header bytes");
}

@Test(expected = MissingSchemaException.class)
@Test
public void testByteBufferUnknownSchema() throws Exception {
MessageEncoder<Record> encoder = new IcebergEncoder<>(SCHEMA_V2);
MessageDecoder<Record> decoder = new IcebergDecoder<>(SCHEMA_V2);

ByteBuffer buffer = encoder.encode(V2_RECORDS.get(0));
buffer.array()[4] = 0x00;

decoder.decode(buffer);
Assertions.assertThatThrownBy(() -> decoder.decode(buffer))
.isInstanceOf(MissingSchemaException.class)
.hasMessageContaining("Cannot resolve schema for fingerprint");
}
}
11 changes: 7 additions & 4 deletions hive-metastore/src/test/java/org/apache/iceberg/hive/HiveTableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
import org.apache.iceberg.relocated.com.google.common.collect.Maps;
import org.apache.iceberg.types.Types;
import org.apache.thrift.TException;
import org.assertj.core.api.Assertions;
import org.junit.Assert;
import org.junit.Rule;
import org.junit.Test;
Expand Down Expand Up @@ -285,16 +286,18 @@ public void testColumnTypeChangeInMetastore() throws TException {
Assert.assertEquals("Schema should match expected", expectedSchema.asStruct(), icebergTable.schema().asStruct());
}

@Test(expected = CommitFailedException.class)
@Test
public void testFailure() throws TException {
Table icebergTable = catalog.loadTable(TABLE_IDENTIFIER);
org.apache.hadoop.hive.metastore.api.Table table = metastoreClient.getTable(DB_NAME, TABLE_NAME);
String dummyLocation = "dummylocation";
table.getParameters().put(METADATA_LOCATION_PROP, dummyLocation);
metastoreClient.alter_table(DB_NAME, TABLE_NAME, table);
icebergTable.updateSchema()
.addColumn("data", Types.LongType.get())
.commit();
Assertions.assertThatThrownBy(() -> icebergTable.updateSchema()
.addColumn("data", Types.LongType.get())
.commit())
.isInstanceOf(CommitFailedException.class)
.hasMessageContaining("is not same as the current table metadata location 'dummylocation'");
}

@Test
Expand Down
13 changes: 8 additions & 5 deletions pig/src/test/java/org/apache/iceberg/pig/SchemaUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import org.apache.iceberg.types.Types.StructType;
import org.apache.pig.ResourceSchema;
import org.apache.pig.impl.logicalLayer.FrontendException;
import org.assertj.core.api.Assertions;
import org.junit.Test;

import static org.apache.iceberg.types.Types.NestedField.optional;
Expand Down Expand Up @@ -72,11 +73,13 @@ public void testComplex() throws IOException {
);
}

@Test(expected = FrontendException.class)
public void invalidMap() throws IOException {
convertToPigSchema(new Schema(
optional(1, "invalid", MapType.ofOptional(2, 3, IntegerType.get(), DoubleType.get()))
), "", "");
@Test
public void invalidMap() {
Assertions.assertThatThrownBy(() -> convertToPigSchema(new Schema(
optional(1, "invalid", MapType.ofOptional(2, 3, IntegerType.get(), DoubleType.get()))
), "", ""))
.isInstanceOf(FrontendException.class)
.hasMessageContaining("Unsupported map key type: int");
}

@Test
Expand Down
13 changes: 9 additions & 4 deletions spark/v2.4/spark/src/test/java/org/apache/iceberg/examples/SchemaEvolutionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.apache.spark.sql.types.DataTypes;
import org.apache.spark.sql.types.LongType$;
import org.apache.spark.sql.types.StructField;
import org.assertj.core.api.Assertions;
import org.junit.AfterClass;
import org.junit.Assert;
import org.junit.Before;
Expand Down Expand Up @@ -159,14 +160,18 @@ public void updateColumnTypeIntToLong() {
first.get().dataType() == LongType$.MODULE$);
}

@Test(expected = IllegalArgumentException.class)
@Test
public void updateColumnTypeIntToString() {
table.updateSchema().updateColumn("price", Types.StringType.get()).commit();
Assertions.assertThatThrownBy(() -> table.updateSchema().updateColumn("price", Types.StringType.get()).commit())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot change column type: price: int -> string");
}

@Test(expected = IllegalArgumentException.class)
@Test
public void updateColumnTypeStringToInt() {
table.updateSchema().updateColumn("author", Types.IntegerType.get()).commit();
Assertions.assertThatThrownBy(() -> table.updateSchema().updateColumn("author", Types.IntegerType.get()).commit())
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("Cannot change column type: author: string -> int");
}

@Test
Expand Down
Loading