@Rule(order=N) #1445

panchenko · 2017-05-03T18:19:38Z

Yet another attempt to order @Rules, based on the #1417 idea, issue #1221.

No breaking API changes, with a cost of using ThreadLocal :-)
both MethodRule & TestRule are supported.
Will add javadocs later, when we agree on the approach.

kcooney

Thanks for the contribution.

I'm usually not a fan of using thread locals in this way, but assuming we can limit who can access the thread-local, the harm seems small (some custom runners won't get ordered rules, but they would have to re implement large parts of BlockJUnit4ClassRunner for that to happen).

kcooney · 2017-05-05T18:45:03Z

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

        List<T> results = new ArrayList<T>();
        for (FrameworkField each : getAnnotatedFields(annotationClass)) {
            try {
                Object fieldValue = each.get(test);
                if (valueClass.isInstance(fieldValue)) {
+                    valueListener.acceptValue(each, fieldValue);


I'd prefer not to have this depend on the thread-local; it's particularly far from the point where it is set, and this method can be called from other contexts.

When I was making an attempt at adding priorities, I ended up extracting classes to replace getAnnotatedFieldValues() and getAnnotatedMethodValues(). See #1448 (in particular, MethodValueCollector (usage) and FieldValueCollector (usage)

I ended up wrapping the rules with adapters that could be asked about the priority of the rule, but I wasn't all that excited about that particular approach. But perhaps you could use classes like those two to populate your RuleContainer?

If we do this, then perhaps the thread local can be in a package-scope class.

kcooney · 2017-05-05T18:46:11Z

src/main/java/org/junit/internal/Throwables.java

@@ -224,9 +224,7 @@ private static boolean isTestFrameworkMethod(String methodName) {
    private static final String[] REFLECTION_METHOD_NAME_PREFIXES = {
        "sun.reflect.",
        "java.lang.reflect.",
-        "org.junit.rules.RunRules.<init>(",


I prefer to keep RunRules listed here, in case custom runners use that class.

kcooney · 2017-05-05T18:57:53Z

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

+
+    private final List<RuleEntry> ruleEntries = new ArrayList<RuleEntry>();
+
+    public void add(MethodRule methodRule) {


It's a bit fragile that this method should only be called after the container collected all of it's data (via the AnnotatedValueListener.

There is a similar issue with between the add() and sort() methods, and between the sort() and apply() methods.

I'm hoping this problem goes away if you use something similar to my MethodValueCollector (see the other comment). If not, then I think think you either need some kind of a state enum (and throw IllegalStateException if the methods are called out of order) or extract classes or interfaces.

kcooney · 2017-05-05T18:59:09Z

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

+        ruleEntries.add(new TestRuleEntry(testRule, orderValues.get(testRule)));
+    }
+
+    public void sort() {


This is only called right before the caller calls apply. I suggest making it private and calling it from apply())

kcooney · 2017-05-05T19:00:03Z

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

+            this.order = order != null ? order.intValue() : -1;
+        }
+
+        public int getInterfacePriority() {


I suggest just making this method abstract

kcooney · 2017-05-05T19:08:57Z

src/main/java/org/junit/runners/BlockJUnit4ClassRunner.java

+        };
+        List<TestRule> testRules;
+        List<MethodRule> methodRules;
+        AnnotatedValueListener.CURRENT.set(ruleContainer);


We would probably want to do something similar in ParentRunner.withClassRules()

panchenko · 2017-05-06T08:31:56Z

@kcooney Thanks for the feedback, I think I've adjusted most of the concerns, could you please give another look? It terms of code flow it should be better and we could possibly discuss how it should be named and in what packages the new classes should go :-)

kcooney

Wow. Much cleaner than my attempt.

I like the package location and class naming. Missing some Javadoc, and a few minor issues, but it looks like we'll get Rule ordering in 4.13!

kcooney · 2017-05-06T16:10:40Z

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

@@ -255,14 +284,13 @@ public String getName() {
                 */
                if (valueClass.isAssignableFrom(each.getReturnType())) {
                    Object fieldValue = each.invokeExplosively(test);
-                    results.add(valueClass.cast(fieldValue));
+                    consumer.accept(each, valueClass.cast(fieldValue));


One thing I noticed in my attempt is that we will call MethodRule methods twice if the return value implements both TestRule and MethodRule (one of the values would be thrown away in withRules(). My MethodValueCollector class avoided this, because the collector subclass can filter out the values before the method value was called.

I'm not sure how much we should worry about that. Your approach is quite nice.

kcooney · 2017-05-06T16:12:57Z

src/main/java/org/junit/runners/BlockJUnit4ClassRunner.java

+        RuleContainer.CURRENT.set(ruleContainer);
+        try {
+            List<TestRule> testRules = getTestRules(target);
+            List<MethodRule> methodRules = rules(target);


Since methodRules is used once, I suggest inlining it:

for (MethodRule each : rules(target)) { ...

(no idea why MethodRule was fully qualified here)

kcooney · 2017-05-06T16:16:42Z

src/main/java/org/junit/runners/RuleContainer.java

+/**
+ * @since 4.13
+ */
+public final class RuleContainer {


Let's make this package-scope.

The only question is how likely someone might want to reuse it? The only part which should stay "hidden" here is the ThreadLocal field, the rest of the class can be moved into the internal package.

kcooney · 2017-05-06T16:19:07Z

src/main/java/org/junit/runners/ParentRunner.java

+        public void accept(FrameworkMember member, TestRule value) {
+            ClassRule rule = member.getAnnotation(ClassRule.class);
+            if (rule != null) {
+                RuleContainer.recordRuleOrder(value, rule.order());


It's not obvious here that we are modifying a thread local. I would prefer that recordRuleOrder() be a non-static method.

You can use ThreadLocal.inititalValue() to initialize the thread local to a no-op value

Actually, instead of doing all the work to create a no-op version of RuleCollector, just do the null check of the thread-local here.

It's used twice - for @Rule and @ClassRule, are we OK with the small code duplication?

kcooney · 2017-05-06T16:21:05Z

src/main/java/org/junit/runners/RuleContainer.java

+        testRules.add(testRule);
+    }
+
+    public void addTestRules(List<? extends TestRule> rules) {


nit: let's either name this addAll() or rename the add() methods to addTestRule() and addMethodRule()

Or just get rid of it. It's trivial to inline, and you don't have addMethodRules()

It's used twice, so I've renamed the add() methods.

I would still prefer to leave it out. If you keep it, rename it to addAllTestRules and add a method named addAllMethodRules

kcooney · 2017-05-06T16:23:11Z

src/main/java/org/junit/runners/RuleContainer.java

+     * Returns entries in the order how they should be applied, i.e. inner-to-outer.
+     */
+    private List<RuleEntry> getSortedEntries() {
+        List<RuleEntry> ruleEntries = new ArrayList<RuleEntry>();


Nit: new ArrayList<RuleEntry>(methodRules.size() + testRules.size())

kcooney · 2017-05-06T16:26:28Z

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

+/**
+ * @since 4.13
+ */
+public interface MemberValueConsumer<T> {


I like the name and the packaging. Could you add a short description of what it does in the class Javadoc, and add method Javadoc to every new public method that is in a public class?

kcooney · 2017-05-06T16:29:19Z

src/test/java/org/junit/runners/RuleContainerTest.java

+    public void methodRulesOnly() {
+        container.add(MRule.M1);
+        container.add(MRule.M2);
+        assertEquals("[M1, M2]", container.getSortedRules().toString());


That's clever making the rules an enum and using toString().

kcooney · 2017-05-06T16:31:36Z

src/main/java/org/junit/ClassRule.java

+     *
+     * @since 4.13
+     */
+    int order() default -1;


I personally like priority best. I can live with order. I don't like level

@junit-team/junit-committers what are your preferences?

I'm fine with order.

kcooney · 2017-05-06T18:39:57Z

If we merge this, should we deprecate RuleChain?

panchenko · 2017-05-07T07:28:16Z

If we merge this, should we deprecate RuleChain?

I was also thinking of this.

panchenko · 2017-05-09T15:55:04Z

I've made some more changes here, particularly ordering of @ClassRule happens in a more direct way, no ThreadLocal is needed there, so it's moved into BlockJUnit4ClassRunner.

kcooney · 2017-05-09T22:51:04Z

@panchenko thanks. I've busy with my primary job, so I might not have time to look into this until the weekend.

kcooney · 2017-05-10T03:14:10Z

src/main/java/org/junit/rules/RunRules.java

+     *
+     * @since 4.13
+     */
+    public static Statement applyAll(Statement base, Iterable<TestRule> rules,


I don't understand why you need to create this new method. What's wrong with the RunRules API?

kcooney · 2017-05-10T03:20:26Z

src/main/java/org/junit/runners/RuleContainer.java

+        testRules.add(testRule);
+    }
+
+    public void addTestRules(List<? extends TestRule> rules) {


I would still prefer to leave it out. If you keep it, rename it to addAllTestRules and add a method named addAllMethodRules

kcooney · 2017-05-10T20:56:02Z

src/main/java/org/junit/runners/ParentRunner.java

-            RuleContainerHelper.CURRENT.remove();
-        }
-        return ruleContainer.apply(null, getDescription(), null, statement);
+        return RunRules.applyAll(statement, classRules(), getDescription());


We should not be applying rules in this method. Instead, we should return a Statement which applies the rules, so the rules are applied when the final statement is evaluated. That's why RunRules is a Statement.

panchenko · 2017-05-11T00:44:37Z

@kcooney currently RunRules applies all the rules in the constructor, so the only difference between

new RunRules(statement, rules)
RunRules.applyAll(statement, rules)

is an additional RunRules instance with the evaluatemethod like this

public void evaluate() throws Throwable {
    statement.evaluate();
}

As an optimization I've removed this instance and make use of the constructed statement directly. I agree this change is not strictly necessary, and at the same time I can't understand why an extra object is needed here :-)

kcooney · 2017-05-12T00:00:10Z

@panchenko ah didn't realize RunRules did the work in the constructor. Still, I prefer not to change it because 1) the change is unrelated to this pull 2) we should avoid depreciating things unless there is a clear reason for the current users to change, 3) the overhead of creating an object is low and 4) maybe we should apply the rules at statement execution time.

panchenko · 2017-05-12T07:23:40Z

@kcooney reverted that change, will suggest it separately :-)
"4)" would be a behavior change, I don't think we should do it.

kcooney · 2017-05-12T18:03:39Z

This LGTM. Thanks!

The only open issue in my mind is the naming of the attribute. Order might be confusing, since if you ordered two ExternalResource rules, I would expect the before() method of the rule with the lower order to happen first, but it happens the other way around. But for Verifier rules, I think I would expect the opposite. I think priority makes it clear that the attribute affects which one is applied first, but forces the user to read the Javadoc to get clarity (though we might want to provide an example of what it means to be "inner", since if you use rules but don't write them you might not understand)

panchenko · 2017-05-13T01:16:08Z

I agree that naming is always an issue.

Currently the rules with lower values are executed first, so for some kind of rules it's already intuitively clear, from the test:

@Rule(order = 1)
public final TestRule a = new LoggingTestRule(ruleLog, "outer");
@Rule(order = 2)
public final TestRule z = new LoggingTestRule(ruleLog, "inner");
....
assertEquals(" outer.begin inner.begin foo inner.end outer.end", ruleLog.toString());

priority might cause more confusion, e.g. in Windows higher value means higher priority, so one might assume it would be executed the other way around. Reading documentation is a good thing, but sometimes a question to GitHub/StackOverflow/forum is posted before doing that :-)

I would suggest optimizing for the common use case, which I think is about creating some resource depending on another one. That's why order and sorting this way.

marcphilipp · 2017-05-14T15:27:46Z

src/main/java/org/junit/runners/BlockJUnit4ClassRunner.java

@@ -463,4 +441,22 @@ private long getTimeout(Test annotation) {
        }
        return annotation.timeout();
    }
+
+    private static final ThreadLocal<RuleContainer> CURRENT_RULE_CONTAINER =
+            new ThreadLocal<RuleContainer>();


Why does this have to be a ThreadLocal? Can't we use an instance variable and pass it to the RuleCollector constructor?

This complexity comes from different methods returning MethodRule and TestRule instances - both are merged and ordered together.

marcphilipp · 2017-05-14T15:33:42Z

src/main/java/org/junit/runners/RuleContainer.java

+                result = ((TestRuleEntry) ruleEntry).rule.apply(result, description);
+            } else {
+                result = ((MethodRuleEntry) ruleEntry).rule.apply(result, method, target);
+            }


The if can be replaced my introducing an apply(FrameworkMethod method, Description description, Object target, Statement statement) method in RuleEntry.

It is possible, however I am not sure that would improve readability. The number of lines would be increased for sure.

Well, if you don't want to change this, then please remove TestRuleEntry and MethodRuleEntry and do the instanceof check on the contained rule class directly.

marcphilipp · 2017-05-14T15:35:03Z

src/main/java/org/junit/runners/RuleContainer.java

+        RuleEntry(T rule, int order) {
+            this.rule = rule;
+            this.order = order;
+        }


Do we really need both constructors (same below)?

Changed this.

marcphilipp · 2017-05-17T13:33:06Z

@kcooney Are you fine with order now? PTAL!

panchenko · 2017-05-17T13:37:01Z

@marcphilipp What's your opinion on deprecating RuleChain?

marcphilipp · 2017-05-17T14:22:38Z

FWIW I think we should deprecate RuleChain.

kcooney · 2017-05-25T16:29:52Z

Changes look great!

I'd like to merge this as two commits. The second commit would have the RuleChain changes and the first would have anything else. @panchenko are you comfortable enough with git to do this? If not, I can do it, but that would mess up the history in your repo.

panchenko · 2017-05-26T07:03:03Z

@kcooney Sure, I've changed this to 2 commits.

kcooney · 2017-05-26T17:06:56Z

@panchenko would it be possible to put a few more details in the first commit message? No rush; I won't be spending time on JUnit this weekend.

Thanks for all of the work recently!

kcooney · 2017-08-07T00:14:46Z

@marcphilipp merged. Would you please update the release notes on the wki?

panchenko · 2017-08-26T04:41:14Z

@kcooney I've added this to release notes.

Closes junit-team#1445

kcooney requested changes May 5, 2017

View reviewed changes

kcooney approved these changes May 6, 2017

View reviewed changes

This was referenced May 6, 2017

Allow specifying ordering of Rules via @Rule(priority = <value>) #1448

Closed

allow RuleChain construction by annotation: @Rule(priority=42) #1417

Closed

kcooney added the 4.13 label May 6, 2017

kcooney reviewed May 10, 2017

View reviewed changes

panchenko force-pushed the Rule_order branch from bb16301 to 5d80d40 Compare May 12, 2017 07:11

This was referenced May 14, 2017

MethodRule TestRule ordering issue #383

Open

Update TestWatcher Javadoc to specify that it should be the "outer" rule #1436

Closed

marcphilipp reviewed May 14, 2017

View reviewed changes

panchenko force-pushed the Rule_order branch from b30517e to 3ee4bcc Compare May 25, 2017 16:37

kcooney mentioned this pull request May 26, 2017

Introduce class TestRuleAdapter / RuleChain support for MethodRule #1349

Closed

panchenko added 2 commits May 27, 2017 08:02

Declarative ordering of @Rule/@ClassRule rules

fdf5ae7

deprecate RuleChain

0bb88ac

panchenko force-pushed the Rule_order branch from 3ee4bcc to 0bb88ac Compare May 27, 2017 01:06

kcooney mentioned this pull request Jun 15, 2017

ErrorCollector will not fail tests if used in conjunction with an ExpectedException #1466

Closed

panchenko mentioned this pull request Jul 21, 2017

Add getter for @Rule in a RuleChain #1474

Closed

kcooney modified the milestone: 4.13 Aug 6, 2017

kcooney removed the 4.13 label Aug 6, 2017

kcooney closed this in e024f83 Aug 7, 2017

kcooney added the needs release notes update label Aug 7, 2017

panchenko deleted the Rule_order branch August 26, 2017 04:41

stefanbirkner removed the needs release notes update label Feb 7, 2018

stefanbirkner mentioned this pull request Feb 13, 2018

RuleChain annotation equivalent #1221

Closed

sebasjm pushed a commit to sebasjm/junit4 that referenced this pull request Mar 11, 2018

deprecate RuleChain

67fbc7b

Closes junit-team#1445

aristotle0x01 pushed a commit to aristotle0x01/junit4 that referenced this pull request Jun 27, 2022

deprecate RuleChain

beb042d

Closes junit-team#1445


		private final List<RuleEntry> ruleEntries = new ArrayList<RuleEntry>();

		public void add(MethodRule methodRule) {

@Rule(order=N) #1445

@Rule(order=N) #1445

Conversation

panchenko commented May 3, 2017 • edited Loading

kcooney left a comment

Choose a reason for hiding this comment

kcooney May 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panchenko commented May 6, 2017

kcooney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kcooney commented May 6, 2017

panchenko commented May 7, 2017

panchenko commented May 9, 2017

kcooney commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

panchenko commented May 11, 2017 • edited Loading

kcooney commented May 12, 2017

panchenko commented May 12, 2017 • edited Loading

kcooney commented May 12, 2017

panchenko commented May 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcphilipp commented May 17, 2017

panchenko commented May 17, 2017

marcphilipp commented May 17, 2017

kcooney commented May 25, 2017

panchenko commented May 26, 2017

kcooney commented May 26, 2017

kcooney commented Aug 7, 2017

panchenko commented Aug 26, 2017

panchenko commented May 3, 2017 •

edited

Loading

kcooney May 5, 2017 •

edited

Loading

panchenko commented May 11, 2017 •

edited

Loading

panchenko commented May 12, 2017 •

edited

Loading

panchenko commented May 13, 2017 •

edited

Loading