Skip to content
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

Add validation check for the return value of methods annotated with @Rule #1016

Closed
kcooney opened this issue Nov 1, 2014 · 6 comments
Closed

Comments

@kcooney
Copy link
Member

kcooney commented Nov 1, 2014

We should add a validation check that methods annotated with @Rule are public and return either a TestRule (or subclass) or a MethodRule (or subclass)

@kcooney
Copy link
Member Author

kcooney commented Nov 1, 2014

Also, methods annotated with @Rule should not be static (unless they are also annotated with @ClassRule) and methods annotated with @ClassRule should be static

@npathai
Copy link
Contributor

npathai commented Nov 1, 2014

@kcooney All validations are already in place.
Tested following validations:

  • @Rule methods must be public, non-static and return @TestRule or @MethodRule
  • @ClassRule methods must be public and static and return @TestRule or @MethodRule

@npathai
Copy link
Contributor

npathai commented Nov 1, 2014

@kcooney But as with #589 same problem is present with @ClassRule. Should we create a new issue for that? But I am still unclear about the difference between TestRule and MethodRule. Do you have some idea as to why was MethodRule was un-deprecated in version 4.9?

@kcooney
Copy link
Member Author

kcooney commented Nov 2, 2014

@NarendraPathai you can only use @ClassRule with TestRule.

BTW, MethodRule was un-deprecated because some developers preferred it over TestRule for rules that operate on methods.

@kcooney kcooney closed this as completed Nov 2, 2014
@npathai
Copy link
Contributor

npathai commented Nov 2, 2014

@kcooney @ClassRule methods do allow MethodRule while validating, but do not call them. So should we change the validation? Because this case passes without validation error:

// should fail validation if @ClassRule does not allow `MethodRule`
    public class ClassRuleTest {
        @ClassRule
        public static MethodRule methodRule() {
            return new MethodRule() {

                @Override
                public Statement apply(Statement base, FrameworkMethod method, Object target) {
                    return base;
                }
            };
        }

        @Test
        public void doNothing() {

        }
    }

@kcooney
Copy link
Member Author

kcooney commented Nov 2, 2014

@NarendraPathai we should certainly update the validation code for this. Class rules work on the class, so there is no method to pass to a MethodRule.

If you would like to work on this, note that we now support validating via annotations. See https://github.com/junit-team/junit/blob/master/src/main/java/org/junit/validator/AnnotationValidator.java We should simply update RuleMemberValidator. It's unfortunate that we have two separate ways to do validations, but I don't think we can fix this in the 4.x timeframe.

If you don't want to work on this, let's open a new bug so we can track it.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants