RuleChain#around should not allow null args #1313
Conversation
By throwing IllegalArgEx from around(), we allow for better feedback to the final user, as the stacktrace will point to the exact line where the null rule is declared. Previously the stacktrace would have been something like this: java.lang.NullPointerException at org.junit.rules.RunRules.applyAll(RunRules.java:26) at org.junit.rules.RunRules.<init>(RunRules.java:15) at org.junit.rules.RuleChain.apply(RuleChain.java:96) at org.junit.rules.RunRules.applyAll(RunRules.java:26) at org.junit.rules.RunRules.<init>(RunRules.java:15) at org.junit.runners.BlockJUnit4ClassRunner.withTestRules(BlockJUnit4ClassRunner.java:424) at org.junit.runners.BlockJUnit4ClassRunner.withRules(BlockJUnit4ClassRunner.java:378) at org.junit.runners.BlockJUnit4ClassRunner.methodBlock(BlockJUnit4ClassRunner.java:299) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:83) at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:58) at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290) at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71) at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288) at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58) at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268) at org.junit.runners.ParentRunner.run(ParentRunner.java:363) at org.junit.runner.JUnitCore.run(JUnitCore.java:137) As you can see, it was pretty hard to figure out that the null pointer was one of the rules in the chain.
| */ | ||
| public RuleChain around(TestRule enclosedRule) { | ||
| if (enclosedRule == null) { | ||
| throw new IllegalArgumentException("The enclosed rule should not be null"); |
There was a problem hiding this comment.
We generally throw NullPointerException if someone passes in a null for a parameter that shouldn't be null (this is also what the Java Collections library does and what Effective Java recommends).
There was a problem hiding this comment.
I see. Done, although I really prefer IllegalArgumentEx as I find it more meaningful than a NPE: NPEs can be thrown by the JVM, whereas IllegalArgEx cannot.
For example, take the unit test: since it's now considering a NPE as expected, I had to add an assertion on the message to make sure that the NPE is really what we are expecting, and not some other null reference (a programming error).
| } | ||
|
|
||
| // TODO delete as soon as #1312 is merged | ||
| private static String stacktraceToString(Throwable e) { |
There was a problem hiding this comment.
In case you are going to merge #1312 soon, please merge that before this, so that I can remove this method.
|
Ready |
| @@ -77,8 +77,12 @@ private RuleChain(List<TestRule> rules) { | |||
| * | |||
| * @param enclosedRule the rule to enclose. | |||
There was a problem hiding this comment.
Please change this line to @param enclosedRule the rule to enclose; must not be {@code null}.
|
Any other concerns on this one? |
|
LGTM, thanks! @junit-team/junit-committers Please take a look! |
|
LGTM |
|
Thanks! @alb-i986 Please add a description of this improvement to the draft of the release notes. |
|
|
Thanks! 👍 |
…rom the start, but not noticed until junit-team/junit4#1313.
By throwing IllegalArgEx from around(),
we allow for better feedback to the final user,
as the stacktrace will point to the exact line where the null rule is declared.
Previously the stacktrace would have been something like this:
As you can see, it was pretty hard to figure out that the null pointer was one of the rules in the chain.
Whereas with this change it is:
where
at org.junit.rules.RuleChainTest$RuleChainWithNullRules.<init>(RuleChainTest.java:81)points at the line where the null rule is appended.Follows the sample test class used above: