From 85d20a00964418072faec0f6b5e1c70f28a9005e Mon Sep 17 00:00:00 2001
From: yangjie01 <yangjie01@baidu.com>
Date: Thu, 6 Jul 2023 22:17:17 +0800
Subject: [PATCH] [SPARK-44314][BUILD][CORE][TESTS] Add a new checkstyle rule
 to prohibit the use of `@Test(expected = ExpectedException.class)`

### What changes were proposed in this pull request?
This pr add a new java checkstyle rule

```
<module name="RegexpSinglelineJava">
  <property name="format" value="Test\(expected"/>
  <property name="message" value="Please use the `assertThrows` method to test for exceptions." />
</module>
```

to prohibit the use of Test(expected = ExpectedException.class)

### Why are the changes needed?
Refer to https://github.com/junit-team/junit4/wiki/Exception-testing

Junit 4 does not recommend `Specifying the expected annotation via the Test annotation.`,

```
The expected parameter should be used with care. The above test will pass if any code in the method throws IndexOutOfBoundsException. Using the method you also cannot test the value of the message in the exception, or the state of a domain object after the exception has been thrown.

For these reasons, the previous approaches are recommended.
```

And `Using assertThrows Method` is recommended.

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
- Pass Github Actions
- Manual checked:

run `dev/lint-java` without change `RemoteBlockPushResolverSuite.java`

```
Checkstyle checks failed at following occurrences:
[ERROR] src/test/java/org/apache/spark/network/shuffle/RemoteBlockPushResolverSuite.java:[284] (regexp) RegexpSinglelineJava: Please use the `assertThrows` method to test for exceptions.
[ERROR] src/test/java/org/apache/spark/network/shuffle/RemoteBlockPushResolverSuite.java:[301] (regexp) RegexpSinglelineJava: Please use the `assertThrows` method to test for exceptions.
```

Closes #41872 from LuciferYang/SPARK-44314.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: yangjie01 <yangjie01@baidu.com>
---
 .../shuffle/RemoteBlockPushResolverSuite.java | 26 +++++++------------
 dev/checkstyle.xml                            |  4 +++
 2 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/RemoteBlockPushResolverSuite.java b/common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/RemoteBlockPushResolverSuite.java
index 0847121b0ccb0..e0b3315aad189 100644
--- a/common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/RemoteBlockPushResolverSuite.java
+++ b/common/network-shuffle/src/test/java/org/apache/spark/network/shuffle/RemoteBlockPushResolverSuite.java
@@ -281,7 +281,7 @@ public void testDuplicateBlocksAreIgnoredWhenPrevStreamIsInProgress() throws IOE
     verifyMetrics(4, 0, 0, 0, 0, 0, 4);
   }
 
-  @Test(expected = RuntimeException.class)
+  @Test
   public void testFailureAfterData() throws IOException {
     StreamCallbackWithID stream =
       pushResolver.receiveBlockDataAsStream(
@@ -289,16 +289,13 @@ public void testFailureAfterData() throws IOException {
     stream.onData(stream.getID(), ByteBuffer.wrap(new byte[4]));
     stream.onFailure(stream.getID(), new RuntimeException("Forced Failure"));
     pushResolver.finalizeShuffleMerge(new FinalizeShuffleMerge(TEST_APP, NO_ATTEMPT_ID, 0, 0));
-    try {
-      pushResolver.getMergedBlockMeta(TEST_APP, 0, 0, 0);
-    } catch (RuntimeException e) {
-      assertTrue(e.getMessage().contains("is empty"));
-      verifyMetrics(4, 0, 0, 0, 0, 0, 4);
-      throw e;
-    }
+    RuntimeException e = assertThrows(RuntimeException.class,
+      () -> pushResolver.getMergedBlockMeta(TEST_APP, 0, 0, 0));
+    assertTrue(e.getMessage().contains("is empty"));
+    verifyMetrics(4, 0, 0, 0, 0, 0, 4);
   }
 
-  @Test(expected = RuntimeException.class)
+  @Test
   public void testFailureAfterMultipleDataBlocks() throws IOException {
     StreamCallbackWithID stream =
       pushResolver.receiveBlockDataAsStream(
@@ -308,13 +305,10 @@ public void testFailureAfterMultipleDataBlocks() throws IOException {
     stream.onData(stream.getID(), ByteBuffer.wrap(new byte[4]));
     stream.onFailure(stream.getID(), new RuntimeException("Forced Failure"));
     pushResolver.finalizeShuffleMerge(new FinalizeShuffleMerge(TEST_APP, NO_ATTEMPT_ID, 0, 0));
-    try {
-      pushResolver.getMergedBlockMeta(TEST_APP, 0, 0, 0);
-    } catch (RuntimeException e) {
-      assertTrue(e.getMessage().contains("is empty"));
-      verifyMetrics(9, 0, 0, 0, 0, 0, 9);
-      throw e;
-    }
+    RuntimeException e = assertThrows(RuntimeException.class,
+      () -> pushResolver.getMergedBlockMeta(TEST_APP, 0, 0, 0));
+    assertTrue(e.getMessage().contains("is empty"));
+    verifyMetrics(9, 0, 0, 0, 0, 0, 9);
   }
 
   @Test
diff --git a/dev/checkstyle.xml b/dev/checkstyle.xml
index 343eaa4cfda7c..5af15318081a6 100644
--- a/dev/checkstyle.xml
+++ b/dev/checkstyle.xml
@@ -180,6 +180,10 @@
               value="Avoid using com.google.common.io.Files.createTempDir() due to CVE-2020-8908.
                 Use org.apache.spark.network.util.JavaUtils.createTempDir() instead." />
         </module>
+        <module name="RegexpSinglelineJava">
+            <property name="format" value="@Test\(expected"/>
+            <property name="message" value="Please use the `assertThrows` method to test for exceptions." />
+        </module>
         <module name="IllegalImport">
             <property name="illegalPkgs" value="org.apache.log4j" />
         </module>