-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Resolved #589 where the method rules returned my methods were not e... #1015
Conversation
@@ -377,8 +378,13 @@ private Statement withMethodRules(FrameworkMethod method, List<TestRule> testRul | |||
* test | |||
*/ | |||
protected List<org.junit.rules.MethodRule> rules(Object target) { | |||
return getTestClass().getAnnotatedFieldValues(target, Rule.class, | |||
List<org.junit.rules.MethodRule> rules = getTestClass().getAnnotatedFieldValues(target, Rule.class, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no need to fully qualify MethodRule
here or below
Thanks for the fix! |
@kcooney Cheers man, I am a longtime user and now contributing. More contributions to come for sure. I have incorporated suggested changes. |
/** | ||
* If there are any public methods annotated with @Rule returning a {@link MethodRule} | ||
* then it should also be run. | ||
* <br/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style-nit: to add a blank line in Javadoc, we use <p>
as specified in the style guide here: https://google-styleguide.googlecode.com/svn/trunk/javaguide.html#s7.1.2-javadoc-paragraphs
For example:
/**
* If there are any public methods annotated with @Rule returning a {@link MethodRule}
* then it should also be run.
*
* <p>This case has been added with
* <a href="https://github.com/junit-team/junit/issues/589">Issue #589</a> -
Fantastic! Thanks for the changes. One very minor issue with the Javadoc. Once you fix that, would you please squash your commits to one commit? See https://github.com/junit-team/junit/wiki/Pull-Requests |
@kcooney I have squashed the commits including the change in javadoc. Thanks for your review and support. |
LGTM @junit-team/junit-committers do we want to include this in 4.12 or a bug-fix release? |
I'm in favor of including it into 4.12. |
+1 |
Resolved #589 where the method rules returned my methods were not e...
Have made following changes that resolves #589
MethodRule
s declared as fields, rules returned from rule provider methods which solves the issueTestClass#getAnnotatedMethodValues
method because previously while scanning for methods returning a rule of particular type method invocation was done and then the type was evaluated based on returned value. But this behavior created a side effect and failed a test when I incorporated behavior to resolve the issue. The caseTestRuleTest#testCallMethodOnlyOnceRule
failed becauseTestClass#getAnnotatedMethodValues
was called twice to scan for@TestRule
and@MethodRule
annotations and that called method rule provider method twice. To solve that change is incorporated inTestClass#getAnnotatedMethodValues