junit4 icon indicating copy to clipboard operation
junit4 copied to clipboard

missing current instance in TestRule apply()

Open RainerW opened this issue 14 years ago • 33 comments

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.

RainerW avatar Oct 24 '11 09:10 RainerW

+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

chbaranowski avatar Oct 24 '11 12:10 chbaranowski

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

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

dsaff avatar Oct 24 '11 14:10 dsaff

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(){...}

chbaranowski avatar Oct 24 '11 16:10 chbaranowski

Christian,

The annotations are available at Description#getAnnotations

dsaff avatar Oct 24 '11 17:10 dsaff

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

chbaranowski avatar Oct 24 '11 17:10 chbaranowski

  • 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 avatar Oct 24 '11 17:10 RainerW

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 avatar Oct 24 '11 18:10 RainerW

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?

RainerW avatar Oct 25 '11 07:10 RainerW

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

dsaff avatar Oct 25 '11 15:10 dsaff

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.

hprange avatar Nov 05 '11 19:11 hprange

Henrique, others,

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

David

dsaff avatar Nov 09 '11 03:11 dsaff

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.

RainerW avatar Nov 09 '11 12:11 RainerW

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.

hprange avatar Nov 09 '11 13:11 hprange

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 ) ?

RainerW avatar Nov 12 '11 21:11 RainerW

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

hwilming avatar Feb 12 '12 15:02 hwilming

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):

  • implement : https://gist.github.com/2358589
  • Depecated TestRule and MethodRule
  • fix #383 because this still makes a newer junit version incompatibe (But inside JUnit this should now be easyly to implement this in a generic way).

step 2 (far future):

  • remove TestRule and MethodRule

RainerW avatar Apr 11 '12 11:04 RainerW

+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.

chbaranowski avatar Apr 11 '12 12:04 chbaranowski

+1

The new API resolves the problem!

Thx.

hwilming avatar Apr 11 '12 18:04 hwilming

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

dsaff avatar Apr 12 '12 18:04 dsaff

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

kordzik avatar May 10 '12 10:05 kordzik

@dsaff did you play with the idea?

RainerW avatar Sep 12 '12 16:09 RainerW

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

dsaff avatar Sep 12 '12 18:09 dsaff

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.

RainerW avatar Oct 03 '12 21:10 RainerW

+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.

borisbrodski avatar Apr 23 '13 11:04 borisbrodski

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

dsaff avatar Apr 23 '13 14:04 dsaff

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?

danhaywood avatar May 18 '14 10:05 danhaywood

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

kcooney avatar May 18 '14 14:05 kcooney

I submitted #1105 to clean up the JavaDoc of MethodRule

EarthCitizen avatar Mar 30 '15 02:03 EarthCitizen

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.

mmichaelis avatar May 14 '15 15:05 mmichaelis

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.

EarthCitizen avatar May 14 '15 15:05 EarthCitizen