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

missing current instance in TestRule apply() #351

Open
RainerW opened this issue Oct 24, 2011 · 33 comments
Open

missing current instance in TestRule apply() #351

RainerW opened this issue Oct 24, 2011 · 33 comments
Labels

Comments

@RainerW
Copy link

RainerW commented Oct 24, 2011

I switched form 4.8 to 4.10 and decided to change my MethodRule's to TestRule ... bad idea.

I'm missing a way to get the FrameworkMethod, and the current test instance, they are missing now in the apply method:

old:
public Statement apply(final Statement base, final FrameworkMethod method, Object target)

new:
public Statement apply(final Statement base, final Description description)

How can i get them? I debugged and the actual Statement instance is of type InvokeMethod, this would have what i need, but dosn't have getter for the private fields.

So, at the moment i can just keep using MethodRule, but i'm afraid this will not work in further versions anymore.

@chbaranowski
Copy link

+1

would be nice to have ?

  • a getTarget() method in the Statement class, returns null if class rule
  • a getTargetClass() method in the Statement class
  • a getFrameworkMethod() method in the Statement class, return null if class rule

@dsaff
Copy link
Member

dsaff commented Oct 24, 2011

You can pass the class instance into the TestRule at creation time:

@rule public TestRule myRule = new MyRule(this);

@chbaranowski
Copy link

Ok not nice, but what is about the test method?
If you like to process custom annotations by a JUnit rule. How can we implement this with the new rule interface?

Here an example:


@rule public SetupDatabaseRule setupDatabaseRule = new SetupDatabaseRule ();

@testdataset(DemoDataSet.class) // custom annotations
@test public void testSample(){...}

@dsaff
Copy link
Member

dsaff commented Oct 24, 2011

Christian,

The annotations are available at Description#getAnnotations

@chbaranowski
Copy link

ok thanks a lot
With Description#getAnnotations and new MyRule(this); we should be able to move our rules to the the new interface

@RainerW
Copy link
Author

RainerW commented Oct 24, 2011

  • passing this into a rule is ugly, but i could live with that
  • Not having the actual method instance could be a problem with multiple overloadings
  • I could find annotations on parent class/hierarchy also via this

So thanks, i now know how to get the stuff i need. Sadly this has effects on the rules themself, so i need to actually change the rules, not only the base class.

Still i would recommend to at least pass $this in some way.

@RainerW
Copy link
Author

RainerW commented Oct 24, 2011

While rewriting i found that the description.getAnnotations() only finds Annotations from the current testMethod, but none from overwritten methods. This is actually a feature i need.

https://gist.github.com/1309806

So description.getAnnotations() should use AnnotationUtils.findAnnotation()

@RainerW
Copy link
Author

RainerW commented Oct 25, 2011

Actually, passing in $this, is really ugly. This breaks all existing projects, not only the rules. I have actually to change all projects and bloat up the constructor.

Isn't there any other way to get $this?

@dsaff
Copy link
Member

dsaff commented Oct 25, 2011

RainerW,

I can understand the frustration. I can imagine some change to the way we handle TestRule would be useful. Can you share a little more about what your current MethodRule does? Thanks,

David

@hprange
Copy link

hprange commented Nov 5, 2011

Hi David,

I feel the same frustration described by others. I've developed a JUnit extension that strongly relies on MethodRule. The rules of my framework can interact with the target object to create/initialize objects for annotated fields. Here is a simple example:

@Rule MyRule rule = new MyRule("parameter1");

@Dummy MyObject dummy; 

MyRule detects the presence of the @Dummy annotation, creates an instance of MyObject and sets it on the dummy field of the target object.

Passing 'this' as a parameter to MyRule isn't a solution because it moves the complexity to solve the problem to users of my framework, which is really undesirable. The lack of a reference to the target object has become a flaw in JUnit framework, and I'm sure the problem can be solved within JUnit internal logic.

@dsaff
Copy link
Member

dsaff commented Nov 9, 2011

Henrique, others,

If we simply undeprecate MethodRule, does this problem go away? Thanks,

David

@RainerW
Copy link
Author

RainerW commented Nov 9, 2011

This would solve the problem, but now you have two interfaces doing the same thing. So one is just cluttering your API. Which one should you use for new rules?

I would strongly recommend to get rid of one of the interfaces and pass in a clean RuleDetails:

interface RuleDetails {
Descriptor getDescriptor(); // new features from TestRule
Object getTarget(); // target instance of null for Classrules
FrameworkMethod getTestMethod(); // get Test Method ( needed for reflections )
}

Or get rid of both end clean your api for the next release.

@hprange
Copy link

hprange commented Nov 9, 2011

Hi David,

I think it is a good solution. Fix the problem and keep compatibility with older versions of JUnit. Thanks for your feedback.

Hi Rainer,

Each interface could have a meaning. Static fields annotated with @ClassRule should be a subtype of TestRule. Non static fields annotated with @Rule should be of type MethodRule. If a rule can be used as a @ClassRule and @Rule, it can implement both. This solution also avoids the misuse of TestRule as a MethodRule and vice-versa.

@RainerW
Copy link
Author

RainerW commented Nov 12, 2011

I completely agree, having a distinct interface for @ClassRule and @rule is a good thing.

But still you have to explain why in MethodRule you have no Descriptor parameter (Not that i can currently think of a usecase for that parameter ) ?

@hwilming
Copy link

Hello,

I’m really vote for the MethodRule interface, because it is a very good point to extend JUnit based Tests with custom functionality. To do something with the test instance, is in my opinion a common use case.

I think the removal of the interface will break a lot of JUnit based Frameworks and APIs. For example our API needs the test instance to initialize some fields of the object under test. This is an Open Source API that is already used in many projects.

I'm already looking for alternatives, but there is no elegant way to get access to the test instance. To passing this into a rule is very ugly and using inheritance instead of Rules is a step back in the wrong way.

I would be very happy, if you undeprecate the MethodRule.

Thanks a lot!
Heinz

@RainerW
Copy link
Author

RainerW commented Apr 11, 2012

To fix this this i would recommend:
https://gist.github.com/2358589

  • create a RuleStatement as replacement for MethodRule/TestRule
    The actual apply() method now has only a "RuleParameter"
  • create two sub-interfaces of RuleParameter :
    • one for @ClassRule : "ClassParameter "
    • one for @rule : "MethodParameter"

This should be more api-stable than the initial approach, because the actual Rule Interface (RuleStatement) dosn't need to be changed for new 'features'. The Interfaces has the ability/power of TestRule and MethodRule, therefore TestRule and MethodRule should be deprecated and can be removed at some point in the future.

step 1 (now):

step 2 (far future):

  • remove TestRule and MethodRule

@chbaranowski
Copy link

+1 for RainerW draft for the new JUnit rule API

The interfaces MethodParameter and ClassParameter, I think should be final classes but the rest of the API is nice.

@hwilming
Copy link

+1

The new API resolves the problem!

Thx.

@dsaff
Copy link
Member

dsaff commented Apr 12, 2012

Let me play with this a bit, and get back to you.

@kordzik
Copy link

kordzik commented May 10, 2012

I have a similar case. Definitely there is a need for a test-method-specific Rule API, along the lines of what method rule did. There is also cases for class and suite-scoped rules.

In fact I have a use case where I would like a rule instance created on a class level and invoked for both class AND each test method (the rule maintains a context that I would like to access on the method invocation level). So basically something that RainerW proposes + possibility to annotate a static instance with both @ClassRule and @rule and have it invoked accordingly

@RainerW
Copy link
Author

RainerW commented Sep 12, 2012

@dsaff did you play with the idea?

@dsaff
Copy link
Member

dsaff commented Sep 12, 2012

Thanks for pinging me. A few weeks ago, I circulated this among a couple people. What do you think about it as a way forward?

https://docs.google.com/document/d/1B6Z45BSGsY08rZqe2KUZTJ3RVsnUE1-HcXjl5MFm9ls/edit?pli=1

@RainerW
Copy link
Author

RainerW commented Oct 3, 2012

Sorry, for the late reply, here my comments to the proposal:

The idea of rules solving different problems look's nice, but i'm not sure if this should be solve by that generic TestRule interface. My point against would be, that appying a rule around a.g. a Datapoint in an Paramterized Test does probably need totally different methods than just an apply -> Statement.
e.g. methods to add / remove items from the array.

I would say at least 50% of my rules are needing the this Object. And the Description::getAnnoations() actually does not return all Annotations. See : https://github.com/KentBeck/junit/issues/351#issuecomment-2507663

The factory concept looks nice, but not sure what the advantage really would be:

  • We would need to implement an additional Class for all scenarios.
  • Also the instance in the test is actually of the factory typ. Not sure if an ExpectedException rule would work that way.
  • When a user implements that Rule Factory interface, his factory class depends closely against the interface. Therefore all changes in the Frameworks Factory Interface will create compile/runtime errors. E.g. the interface cannot be changed, or it will break backward compatability.

I still think my proposal ( https://github.com/KentBeck/junit/issues/351#issuecomment-5066453 ) has more advantages:

  • The test-developer can decide how the rule will be applied, as Before/AfterClass or just as Before/After
  • The instance field in the test class is the actual executed Rule. So it's possible to get/set states like with ExpectedException.
  • The Rule implementation does not depend very close against an interface. You can add Methods to the RuleStatement (or it successors) etc. without breaking the backward compatability.

@borisbrodski
Copy link

+1 for the issue

I'm very interested in a clean solution for this bug (Would be happy to implement it myself)

I can't use the workaround

@Rule public TestRule myRule = new MyRule(this);

since I'm working on JMockit rule support and I need a generic way to find the current test class instance.

@dsaff
Copy link
Member

dsaff commented Apr 23, 2013

@borisbrodski, we've undeprecated MethodRule for this reason--please continue to use it.

@danhaywood
Copy link

Can confirm in 4.11 that MethodRule is undeprecated, but the Javadoc for MethodRule is out-of-date... it lists a number of classes as extending from MethodRule whereas they all - except TestWatcher - now extend from TestRule (presumably ported over in 4.10 or earlier). Could that Javadoc be fixed, 'tis confusing?

@kcooney
Copy link
Member

kcooney commented May 18, 2014

@danhaywood could you send us a pull request fixing the Javadoc?

@EarthCitizen
Copy link
Contributor

I submitted #1105 to clean up the JavaDoc of MethodRule

@mmichaelis
Copy link
Contributor

Unfortunately undeprecating MethodRule is only half of the way as mentioned in comment by me here:

because rule-validation prohibits MethodRules to be annotated with @ClassRule.

A first step might be to weaken org.junit.internal.runners.rules.RuleMemberValidator.MemberMustBeNonStaticOrAlsoClassRule - although I am not aware of the possible impact, or if it just would work.

@EarthCitizen
Copy link
Contributor

It seems that the way that rules are designed need to be re-thought, scrapped and re-designed from scratch. Getting rid of MethodRule was a bit premature, and it is not possible for all existing rules to switch to the new TestRule because of needing to be given the instance of the test class. I talk about this a bit here: #1106 (MethodRules and rule chaining)

Hopefully rules can be redesigned for JUnit 5.

@sbrannen
Copy link
Member

If you want to develop a rule that implements both TestRule and MethodRule (which is a requirement for the Spring TestContext Framework), simply weakening the restrictions in RuleMemberValidator is insufficient since the withMethodRules() method in BlockJUnit4ClassRunner explicitly precludes the ability to have a single rule instance invoked via the TestRule and MethodRule APIs for method-level callbacks!

If a rule implements both TestRule and MethodRule it will only be invoked via the TestRule API for method-level callbacks... which defeats the purpose of implementing MethodRule in the first place.

This is probably worthy of a bug fix in and of itself.

@kcooney
Copy link
Member

kcooney commented May 15, 2015

If someone wants to design a new API that works on the class level and method level, let's discuss that on #793

@kcooney
Copy link
Member

kcooney commented May 15, 2015

rule-validation prohibits MethodRules to be annotated with @ClassRule.

@mmichaelis see my comment in #793.

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

No branches or pull requests