This is an automated email from the ASF dual-hosted git repository.
yangjie01 pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/spark.git
The following commit(s) were added to refs/heads/master by this push:
new d12bec39cb1 [SPARK-44314][BUILD][CORE][TESTS] Add a new checkstyle
rule to prohibit the use of `@Test(expected = ExpectedException.class)`
d12bec39cb1 is described below
commit d12bec39cb141fbfab24d158022b327441d17945
Author: yangjie01 <yangji...@baidu.com>
AuthorDate: Thu Jul 6 22:17:17 2023 +0800
[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 <yangji...@baidu.com>
Signed-off-by: yangjie01 <yangji...@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 0847121b0cc..e0b3315aad1 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 class RemoteBlockPushResolverSuite {
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 class RemoteBlockPushResolverSuite {
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 class RemoteBlockPushResolverSuite {
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 343eaa4cfda..5af15318081 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>
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscr...@spark.apache.org
For additional commands, e-mail: commits-h...@spark.apache.org
