Tests: Replace and ban ExpectedException usage (apache#5921)

It is generally not good practice to use the `ExpectedException` rule, because it is difficult to determine in multi-line tests where exactly an exception is thrown, since the rule is very broad.
fwessels · Oct 5, 2022 · 6e4c51f · 6e4c51f
1 parent 8433bbe
commit 6e4c51f
Show file tree

Hide file tree

Showing 16 changed files with 69 additions and 95 deletions.
diff --git a/.baseline/checkstyle/checkstyle.xml b/.baseline/checkstyle/checkstyle.xml
@@ -383,6 +383,11 @@
             <property name="format" value="@Test\(.*expected.*\)"/>
             <property name="message" value="Prefer using Assertions.assertThatThrownBy(...).isInstanceOf(...) instead."/>
         </module>
+        <module name="IllegalImport">
+            <property name="id" value="BanExpectedExceptionUsage"/>
+            <property name="illegalClasses" value="org.junit.rules.ExpectedException"/>
+            <message key="import.illegal" value="Prefer using Assertions.assertThatThrownBy(...).isInstanceOf(...) instead."/>
+        </module>
         <module name="RegexpSinglelineJava">
             <property name="ignoreComments" value="true"/>
             <property name="format" value="@Json(S|Des)erialize"/>

diff --git a/core/src/test/java/org/apache/iceberg/TableMetadataParserCodecTest.java b/core/src/test/java/org/apache/iceberg/TableMetadataParserCodecTest.java
@@ -19,15 +19,12 @@
 package org.apache.iceberg;
 
 import org.apache.iceberg.TableMetadataParser.Codec;
+import org.assertj.core.api.Assertions;
 import org.junit.Assert;
-import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 
 public class TableMetadataParserCodecTest {
 
-  @Rule public ExpectedException exceptionRule = ExpectedException.none();
-
   @Test
   public void testCompressionCodec() {
     Assert.assertEquals(Codec.GZIP, Codec.fromName("gzip"));
@@ -43,15 +40,15 @@ public void testCompressionCodec() {
 
   @Test
   public void testInvalidCodecName() {
-    exceptionRule.expect(IllegalArgumentException.class);
-    exceptionRule.expectMessage("No enum constant");
-    Codec.fromName("invalid");
+    Assertions.assertThatThrownBy(() -> Codec.fromName("invalid"))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("No enum constant org.apache.iceberg.TableMetadataParser.Codec.INVALID");
   }
 
   @Test
   public void testInvalidFileName() {
-    exceptionRule.expect(IllegalArgumentException.class);
-    exceptionRule.expectMessage("path/to/file.parquet is not a valid metadata file");
-    Codec.fromFileName("path/to/file.parquet");
+    Assertions.assertThatThrownBy(() -> Codec.fromFileName("path/to/file.parquet"))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("path/to/file.parquet is not a valid metadata file");
   }
 }
diff --git a/core/src/test/java/org/apache/iceberg/TestMetricsModes.java b/core/src/test/java/org/apache/iceberg/TestMetricsModes.java
@@ -29,11 +29,11 @@
 import org.apache.iceberg.MetricsModes.Truncate;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
 import org.apache.iceberg.types.Types;
+import org.assertj.core.api.Assertions;
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -52,8 +52,6 @@ public TestMetricsModes(int formatVersion) {
     this.formatVersion = formatVersion;
   }
 
-  @Rule public ExpectedException exceptionRule = ExpectedException.none();
-
   @Rule public TemporaryFolder temp = new TemporaryFolder();
 
   @After
@@ -75,9 +73,9 @@ public void testMetricsModeParsing() {
 
   @Test
   public void testInvalidTruncationLength() {
-    exceptionRule.expect(IllegalArgumentException.class);
-    exceptionRule.expectMessage("length should be positive");
-    MetricsModes.fromString("truncate(0)");
+    Assertions.assertThatThrownBy(() -> MetricsModes.fromString("truncate(0)"))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Truncate length should be positive");
   }
 
   @Test

diff --git a/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java b/core/src/test/java/org/apache/iceberg/TestSchemaUnionByFieldName.java
@@ -42,15 +42,12 @@
 import org.apache.iceberg.types.Types.TimeType;
 import org.apache.iceberg.types.Types.TimestampType;
 import org.apache.iceberg.types.Types.UUIDType;
+import org.assertj.core.api.Assertions;
 import org.junit.Assert;
-import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 
 public class TestSchemaUnionByFieldName {
 
-  @Rule public ExpectedException thrown = ExpectedException.none();
-
   private static List<? extends PrimitiveType> primitiveTypes() {
     return Lists.newArrayList(
         StringType.get(),
@@ -243,20 +240,18 @@ public void testAddNestedMaps() {
 
   @Test
   public void testDetectInvalidTopLevelList() {
-    thrown.expect(IllegalArgumentException.class);
-    thrown.expectMessage("Cannot change column type: aList.element: string -> long");
-
     Schema currentSchema =
         new Schema(optional(1, "aList", Types.ListType.ofOptional(2, StringType.get())));
     Schema newSchema =
         new Schema(optional(1, "aList", Types.ListType.ofOptional(2, LongType.get())));
-    new SchemaUpdate(currentSchema, 2).unionByNameWith(newSchema).apply();
+    Assertions.assertThatThrownBy(
+            () -> new SchemaUpdate(currentSchema, 2).unionByNameWith(newSchema).apply())
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Cannot change column type: aList.element: string -> long");
   }
 
   @Test
   public void testDetectInvalidTopLevelMapValue() {
-    thrown.expect(IllegalArgumentException.class);
-    thrown.expectMessage("Cannot change column type: aMap.value: string -> long");
 
     Schema currentSchema =
         new Schema(
@@ -265,22 +260,26 @@ public void testDetectInvalidTopLevelMapValue() {
     Schema newSchema =
         new Schema(
             optional(1, "aMap", Types.MapType.ofOptional(2, 3, StringType.get(), LongType.get())));
-    Schema apply = new SchemaUpdate(currentSchema, 3).unionByNameWith(newSchema).apply();
+
+    Assertions.assertThatThrownBy(
+            () -> new SchemaUpdate(currentSchema, 3).unionByNameWith(newSchema).apply())
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Cannot change column type: aMap.value: string -> long");
   }
 
   @Test
   public void testDetectInvalidTopLevelMapKey() {
-    thrown.expect(IllegalArgumentException.class);
-    thrown.expectMessage("Cannot change column type: aMap.key: string -> uuid");
-
     Schema currentSchema =
         new Schema(
             optional(
                 1, "aMap", Types.MapType.ofOptional(2, 3, StringType.get(), StringType.get())));
     Schema newSchema =
         new Schema(
             optional(1, "aMap", Types.MapType.ofOptional(2, 3, UUIDType.get(), StringType.get())));
-    new SchemaUpdate(currentSchema, 3).unionByNameWith(newSchema).apply();
+    Assertions.assertThatThrownBy(
+            () -> new SchemaUpdate(currentSchema, 3).unionByNameWith(newSchema).apply())
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Cannot change column type: aMap.key: string -> uuid");
   }
 
   @Test
@@ -311,13 +310,13 @@ public void testTypePromoteFloatToDouble() {
 
   @Test
   public void testInvalidTypePromoteDoubleToFloat() {
-    thrown.expect(IllegalArgumentException.class);
-    thrown.expectMessage("Cannot change column type: aCol: double -> float");
-
     Schema currentSchema = new Schema(required(1, "aCol", DoubleType.get()));
     Schema newSchema = new Schema(required(1, "aCol", FloatType.get()));
 
-    new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply();
+    Assertions.assertThatThrownBy(
+            () -> new SchemaUpdate(currentSchema, 1).unionByNameWith(newSchema).apply())
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Cannot change column type: aCol: double -> float");
   }
 
   @Test
@@ -394,13 +393,13 @@ public void testAddPrimitiveToNestedStruct() {
 
   @Test
   public void testReplaceListWithPrimitive() {
-    thrown.expect(IllegalArgumentException.class);
-    thrown.expectMessage("Cannot change column type: aColumn: list<string> -> string");
-
     Schema currentSchema =
         new Schema(optional(1, "aColumn", Types.ListType.ofOptional(2, StringType.get())));
     Schema newSchema = new Schema(optional(1, "aColumn", StringType.get()));
-    new SchemaUpdate(currentSchema, 2).unionByNameWith(newSchema).apply();
+    Assertions.assertThatThrownBy(
+            () -> new SchemaUpdate(currentSchema, 2).unionByNameWith(newSchema).apply())
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Cannot change column type: aColumn: list<string> -> string");
   }
 
   @Test

diff --git a/data/src/test/java/org/apache/iceberg/data/TestMetricsRowGroupFilter.java b/data/src/test/java/org/apache/iceberg/data/TestMetricsRowGroupFilter.java
@@ -77,12 +77,12 @@
 import org.apache.parquet.hadoop.metadata.BlockMetaData;
 import org.apache.parquet.io.DelegatingSeekableInputStream;
 import org.apache.parquet.schema.MessageType;
+import org.assertj.core.api.Assertions;
 import org.junit.Assert;
 import org.junit.Assume;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -351,15 +351,12 @@ public void testRequiredColumn() {
     Assert.assertFalse("Should skip: required columns are always non-null", shouldRead);
   }
 
-  @Rule public ExpectedException exceptionRule = ExpectedException.none();
-
   @Test
   public void testMissingColumn() {
-    exceptionRule.expect(ValidationException.class);
-    exceptionRule.expectMessage("Cannot find field 'missing'");
-    exceptionRule.reportMissingExceptionWithMessage(
-        "Should complain about missing column in expression");
-    shouldRead(lessThan("missing", 5));
+    Assertions.assertThatThrownBy(() -> shouldRead(lessThan("missing", 5)))
+        .as("Should complain about missing column in expression")
+        .isInstanceOf(ValidationException.class)
+        .hasMessageStartingWith("Cannot find field 'missing'");
   }
 
   @Test

diff --git a/orc/src/test/java/org/apache/iceberg/orc/TestBuildOrcProjection.java b/orc/src/test/java/org/apache/iceberg/orc/TestBuildOrcProjection.java
@@ -25,9 +25,8 @@
 import org.apache.iceberg.Schema;
 import org.apache.iceberg.types.Types;
 import org.apache.orc.TypeDescription;
-import org.junit.Rule;
+import org.assertj.core.api.Assertions;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 
 /** Test projections on ORC types. */
 public class TestBuildOrcProjection {
@@ -137,13 +136,8 @@ public void testEvolutionAddContainerField() {
     assertEquals(TypeDescription.Category.LONG, nestedCol.findSubtype("c_r3").getCategory());
   }
 
-  @Rule public ExpectedException exceptionRule = ExpectedException.none();
-
   @Test
   public void testRequiredNestedFieldMissingInFile() {
-    exceptionRule.expect(IllegalArgumentException.class);
-    exceptionRule.expectMessage("Field 4 of type long is required and was not found");
-
     Schema baseSchema =
         new Schema(
             required(1, "a", Types.IntegerType.get()),
@@ -160,6 +154,9 @@ public void testRequiredNestedFieldMissingInFile() {
                     required(3, "c", Types.LongType.get()),
                     required(4, "d", Types.LongType.get()))));
 
-    ORCSchemaUtil.buildOrcProjection(evolvedSchema, baseOrcSchema);
+    Assertions.assertThatThrownBy(
+            () -> ORCSchemaUtil.buildOrcProjection(evolvedSchema, baseOrcSchema))
+        .isInstanceOf(IllegalArgumentException.class)
+        .hasMessage("Field 4 of type long is required and was not found.");
   }
 }
diff --git a/spark/v2.4/spark/src/test/java/org/apache/iceberg/spark/source/TestStructuredStreaming.java b/spark/v2.4/spark/src/test/java/org/apache/iceberg/spark/source/TestStructuredStreaming.java
@@ -40,12 +40,12 @@
 import org.apache.spark.sql.streaming.DataStreamWriter;
 import org.apache.spark.sql.streaming.StreamingQuery;
 import org.apache.spark.sql.streaming.StreamingQueryException;
+import org.assertj.core.api.Assertions;
 import org.junit.AfterClass;
 import org.junit.Assert;
 import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 import scala.collection.JavaConversions;
 
@@ -58,7 +58,6 @@ public class TestStructuredStreaming {
   private static SparkSession spark = null;
 
   @Rule public TemporaryFolder temp = new TemporaryFolder();
-  @Rule public ExpectedException exceptionRule = ExpectedException.none();
 
   @BeforeClass
   public static void startSpark() {
@@ -259,8 +258,6 @@ public void testStreamingWriteCompleteModeWithProjection() throws Exception {
 
   @Test
   public void testStreamingWriteUpdateMode() throws Exception {
-    exceptionRule.expect(StreamingQueryException.class);
-
     File parent = temp.newFolder("parquet");
     File location = new File(parent, "test-table");
     File checkpoint = new File(parent, "checkpoint");
@@ -284,7 +281,9 @@ public void testStreamingWriteUpdateMode() throws Exception {
       StreamingQuery query = streamWriter.start();
       List<Integer> batch1 = Lists.newArrayList(1, 2);
       send(batch1, inputStream);
-      query.processAllAvailable();
+      Assertions.assertThatThrownBy(query::processAllAvailable)
+          .isInstanceOf(StreamingQueryException.class)
+          .hasMessageContaining("Output mode Update is not supported");
     } finally {
       for (StreamingQuery query : spark.streams().active()) {
         query.stop();

diff --git a/spark/v2.4/spark/src/test/java/org/apache/iceberg/spark/source/TestTimestampWithoutZone.java b/spark/v2.4/spark/src/test/java/org/apache/iceberg/spark/source/TestTimestampWithoutZone.java
@@ -60,7 +60,6 @@
 import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -159,8 +158,6 @@ public void testUnpartitionedTimestampWithoutZoneProjection() {
         read(unpartitioned.toString(), vectorized, "id", "ts"));
   }
 
-  @Rule public ExpectedException exception = ExpectedException.none();
-
   @Test
   public void testUnpartitionedTimestampWithoutZoneError() {
     AssertHelpers.assertThrows(

diff --git a/spark/v3.0/spark/src/test/java/org/apache/iceberg/spark/source/TestStructuredStreaming.java b/spark/v3.0/spark/src/test/java/org/apache/iceberg/spark/source/TestStructuredStreaming.java
@@ -40,12 +40,12 @@
 import org.apache.spark.sql.streaming.DataStreamWriter;
 import org.apache.spark.sql.streaming.StreamingQuery;
 import org.apache.spark.sql.streaming.StreamingQueryException;
+import org.assertj.core.api.Assertions;
 import org.junit.AfterClass;
 import org.junit.Assert;
 import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 import scala.Option;
 import scala.collection.JavaConversions;
@@ -59,7 +59,6 @@ public class TestStructuredStreaming {
   private static SparkSession spark = null;
 
   @Rule public TemporaryFolder temp = new TemporaryFolder();
-  @Rule public ExpectedException exceptionRule = ExpectedException.none();
 
   @BeforeClass
   public static void startSpark() {
@@ -260,8 +259,6 @@ public void testStreamingWriteCompleteModeWithProjection() throws Exception {
 
   @Test
   public void testStreamingWriteUpdateMode() throws Exception {
-    exceptionRule.expect(StreamingQueryException.class);
-
     File parent = temp.newFolder("parquet");
     File location = new File(parent, "test-table");
     File checkpoint = new File(parent, "checkpoint");
@@ -285,7 +282,9 @@ public void testStreamingWriteUpdateMode() throws Exception {
       StreamingQuery query = streamWriter.start();
       List<Integer> batch1 = Lists.newArrayList(1, 2);
       send(batch1, inputStream);
-      query.processAllAvailable();
+      Assertions.assertThatThrownBy(query::processAllAvailable)
+          .isInstanceOf(StreamingQueryException.class)
+          .hasMessageContaining("does not support Update mode");
     } finally {
       for (StreamingQuery query : spark.streams().active()) {
         query.stop();

diff --git a/spark/v3.0/spark/src/test/java/org/apache/iceberg/spark/source/TestTimestampWithoutZone.java b/spark/v3.0/spark/src/test/java/org/apache/iceberg/spark/source/TestTimestampWithoutZone.java
@@ -60,7 +60,6 @@
 import org.junit.BeforeClass;
 import org.junit.Rule;
 import org.junit.Test;
-import org.junit.rules.ExpectedException;
 import org.junit.rules.TemporaryFolder;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -159,8 +158,6 @@ public void testUnpartitionedTimestampWithoutZoneProjection() {
         read(unpartitioned.toString(), vectorized, "id", "ts"));
   }
 
-  @Rule public ExpectedException exception = ExpectedException.none();
-
   @Test
   public void testUnpartitionedTimestampWithoutZoneError() {
     AssertHelpers.assertThrows(