Allow members annotated with both @ClassRule and @Rule to be static #932

rowanhill · 2014-06-16T21:28:20Z

Hi,

This is the change discussed in #793 to allow static members annotated with both @ClassRule and @Rule (whereas currently, all @Rule members must be non-static).

Although the change is relatively small, I've significantly refactored RuleFieldValidator (and renamed it to RuleMemberValidator). I've kept these two steps in separate commits, with one final commit to actually change the behaviour.

My changes include:

Changing RuleFieldValidator from an enum to a class. I couldn't see why it was an enum: it's values were never used in that sense, and they don't really seem to semantically be an enumeration.
Pulled the various bits of logic (controlled by flags to the constructor) out into strategy classes. Each such class has default visibility, so it shouldn't look like anythings changed to the world outside the package. These aren't individually tested, as I think the existing tests cover them well enough. I could add more fine grained tests if that's necessary, though.
Constructed the four validators using a builder, purely for enhanced readability; it's now (hopefully) very easy to look at the validator definition and read off the logic they're validating.
Renamed RuleFieldValidator to RuleMemberValidator, since we validate both methods and fields
Added new tests & updated one of the strategies to allow non-static @ClassRule members, if and only if they're also annotated as @Rule.

I've referred to @Rule annotated members as 'test rules' in a few places, as that wording already seemed to be present in the codebase, but I'm not sure if that's strictly correct terminology (since a TestRule can be annotated with @ClassRule).

I think that covers everything - let me know if anyone has questions, or if anything needs altering for this PR to be accepted.

Thanks,
Rowan

kcooney · 2014-06-18T05:11:51Z

src/main/java/org/junit/internal/runners/rules/RequireDeclaredNonStaticStrategy.java

+ * Requires the validated member to be non-static
+ */
+class RequireDeclaredNonStaticStrategy extends ValidatorStrategy {
+    RequireDeclaredNonStaticStrategy(ValidationErrorAdder fValidationErrorAdder) {


style-nit: we don't start parameter names with 'f'

kcooney · 2014-06-18T05:48:24Z

Thanks for the contribution. Unfortunately, github doesn't make it easy for me to comment on the refactoring independent from the functional change. I like having the changes separate, but let's worry about doing that once we resolve the issues.

rowanhill · 2014-06-18T08:55:19Z

Thanks for the detailed review! I've made all but one of your suggested changes (see the inline comment about getAnnotation vs getAnnotations above). I'm never sure whether to amend commits and force push, or append commits and then rebase once everyone's happy: I've done the former here, I hope that's okay.

I was a bit hesitant about putting all the validators into nested classes - I actually originally wrote them like that, but I felt it made the parent class a bit ungainly. It's not so bad now the Builder is a lot smaller, though.

Also note that I've introduced a fourth commit, for making the static MethodRule validation message more precise (so it didn't get lost in the refactoring), and I've added a couple of tests for it.

A couple of small changes I have left in the refactoring (but possibly shouldn't have done):

One of the error messages had a double space in; I've fixed that, and the corresponding tests
I've re-worded the must-be-non-static error message very slightly (has to be -> must be)

kcooney · 2014-06-18T14:34:27Z

src/main/java/org/junit/internal/runners/rules/RuleMemberValidator.java

+    interface RuleValidator {
+        /**
+         * Examine the given member and add any violations of the strategy's validation logic to the given list of errors
+         *  @param member The member (field or member) to examine


style-nit: extra space before @

Oops - not sure how that got there. Fixed

kcooney · 2014-06-18T14:46:56Z

Wow, fantastic turn around! A few minor issues. Don't worry about merging commits at this time (unless it makes it easier for you); let's do it right at the end.

Unfortunately, you have merge conflicts (possibly due to #921). Note that as of that pull, we no longer use the "f" prefix for fields.

rowanhill · 2014-06-18T20:07:15Z

Okay - I've rebased (and got rid of all the fFields - yay!) and addressed your comments. Since I was rebasing anyway, I've made the changes in the amended commits. I've left notes on each of your comments, explaining what I've done if it wasn't trivial.

kcooney · 2014-06-18T21:02:09Z

src/main/java/org/junit/runners/model/Annotatable.java

+    /**
+     * @param annotationType The class of annotation to return
+     * @param <T> The type of the annotation to return
+     * @return The annotation on the model element of the given type, or null


If the choice is between either having a method description or having a @return tag, I always prefer having a method description. The first sentence of the method Javadoc is used in the class overview.

Incidentally, I generally use @code{null} in Javadoc when referring to null

Done - it did all seem a bit repetitious :)

kcooney · 2014-06-18T21:03:35Z

src/test/java/org/junit/runners/model/FrameworkFieldTest.java

@@ -1,10 +1,14 @@
 package org.junit.runners.model;

+import static org.hamcrest.CoreMatchers.*;


We don't use wildcard imports. They tend to cause problems when new static methods are added to a class

(but thank you very much for adding a test; I wouldn't have bothered because it's trivial, but it's nice to have)

Ah, sorry, fixed; I hadn't realised IntelliJ had done that.

kcooney · 2014-06-18T21:08:45Z

Sweet. Nervously excited that I can merge this tonight

rowanhill · 2014-06-18T21:42:01Z

Okay, I've updated that last commit. Hopefully that's got it this time - thanks for being patient with me!

Allow members annotated with both @ClassRule and @rule to be static

kcooney · 2014-06-19T04:16:36Z

@rowanhill Thank you for being patient! Could you please update the release notes to mention this commit?

rowanhill · 2014-06-19T06:24:30Z

Sure thing. The release notes are now updated.

Thanks again!

kcooney · 2014-06-19T16:26:36Z

Thanks. I updated the release notes to warn about static fields/methods annotated with @Rule and running tests in parallel

kcooney reviewed Jun 18, 2014
View reviewed changes

kcooney changed the title ~~Allow non-static members annotated both @ClassRule and @Rule~~ Allow members annotated with both @ClassRule and @Rule to be static Jun 18, 2014

kcooney reviewed Jun 18, 2014
View reviewed changes

rowanhill added 3 commits June 18, 2014 20:22

Refactor RuleFieldValidator into strategy classes

759061a

More precise error message for static MethodRules

6ca812e

Rename RuleFieldValidator to RuleMemberValidator

dc23211

kcooney reviewed Jun 18, 2014
View reviewed changes

Allow static @rule members if also @ClassRule

15c925c

kcooney added a commit that referenced this pull request Jun 19, 2014

Merge pull request #932 from rowanhill/non-static-class-rules

d9b90bc

Allow members annotated with both @ClassRule and @rule to be static

kcooney merged commit d9b90bc into junit-team:master Jun 19, 2014

stefanbirkner added this to the 4.12 milestone Jun 19, 2014

stefanbirkner added the rules label Jun 19, 2014

spring-projects-issues mentioned this pull request Jan 11, 2019

Provide @Rule alternative to SpringJUnit4ClassRunner [SPR-7731] spring-projects/spring-framework#12387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow members annotated with both @ClassRule and @Rule to be static #932

Allow members annotated with both @ClassRule and @Rule to be static #932

rowanhill commented Jun 16, 2014

kcooney Jun 18, 2014

kcooney commented Jun 18, 2014

rowanhill commented Jun 18, 2014

kcooney Jun 18, 2014

rowanhill Jun 18, 2014

kcooney commented Jun 18, 2014

rowanhill commented Jun 18, 2014

kcooney Jun 18, 2014

rowanhill Jun 18, 2014

kcooney Jun 18, 2014

rowanhill Jun 18, 2014

kcooney commented Jun 18, 2014

rowanhill commented Jun 18, 2014

kcooney commented Jun 19, 2014

rowanhill commented Jun 19, 2014

kcooney commented Jun 19, 2014

		@@ -1,10 +1,14 @@
		package org.junit.runners.model;

		import static org.hamcrest.CoreMatchers.*;

Allow members annotated with both @ClassRule and @Rule to be static #932

Allow members annotated with both @ClassRule and @Rule to be static #932

Conversation

rowanhill commented Jun 16, 2014

kcooney Jun 18, 2014

Choose a reason for hiding this comment

kcooney commented Jun 18, 2014

rowanhill commented Jun 18, 2014

kcooney Jun 18, 2014

Choose a reason for hiding this comment

rowanhill Jun 18, 2014

Choose a reason for hiding this comment

kcooney commented Jun 18, 2014

rowanhill commented Jun 18, 2014

kcooney Jun 18, 2014

Choose a reason for hiding this comment

rowanhill Jun 18, 2014

Choose a reason for hiding this comment

kcooney Jun 18, 2014

Choose a reason for hiding this comment

rowanhill Jun 18, 2014

Choose a reason for hiding this comment

kcooney commented Jun 18, 2014

rowanhill commented Jun 18, 2014

kcooney commented Jun 19, 2014

rowanhill commented Jun 19, 2014

kcooney commented Jun 19, 2014