Skip to content

Commit

Permalink
[SPARK-44314][BUILD][CORE][TESTS] Add a new checkstyle rule to prohib…
Browse files Browse the repository at this point in the history
…it 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 apache#41872 from LuciferYang/SPARK-44314.

Authored-by: yangjie01 <[email protected]>
Signed-off-by: yangjie01 <[email protected]>
  • Loading branch information
LuciferYang authored and ragnarok56 committed Mar 2, 2024
1 parent c2d3981 commit 85d20a0
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -281,24 +281,21 @@ 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(
new PushBlockStream(TEST_APP, NO_ATTEMPT_ID, 0, 0, 0, 0, 0));
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(
Expand All @@ -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
Expand Down
4 changes: 4 additions & 0 deletions dev/checkstyle.xml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down

0 comments on commit 85d20a0

Please sign in to comment.