Add support for test pre-processors #766

Open
vasiliygagin opened this Issue Nov 19, 2013 · 19 comments

Projects

None yet

4 participants

@vasiliygagin

Currently in order to apply any pre-processing, common approach is to override Runner (or subclass of it).
For example to add Spring Framework injection I'd use:

@RunWith(SpringJUnit4ClassRunner.class)

And for Unitils support I'd use:

@RunWith(UnitilsJUnit4TestClassRunner.class).

But what happens when you need support for both?
Unitils actually is trying to solve this problem by supporting some (but not all) of Spring annotations.

It would benefit Java world if JUnit would support something like this:

@ProcessWith({SpringJUnitTestProcessor.class, UnitilsJunitTestProcessor.class})

In this case default BlockJUnit4ClassRunner will be used but listed processors will take care of their own responsibilities and would not have to know about each other.
Each of the processors would have to implement so common interface with a method like this:

public void prepareTest(Object testInstance, Method testMethod).

If there is an interest in this features, I'd like to contribute.

@stefanbirkner
Collaborator

Is this something, which cannot be done with a TestRule?

@vasiliygagin

I do not see how it can be don wit TestRules.
Preprocessors need access to test instance (to inject resources in Spring case) and to method annotations (like in Unitils case). TestRule interface seem to be lacking in this regard.
In addition to that, this really needs to be a class level annotation, like @RunWith.
I think about it as an improvement to @RunWith, In majority cases people will use @ProcessWith instead of @RunWith, thus leaving runner to be default.
Another way to think about the Pre-processors, as a plugins. In the proposed change you can apply multiple plugins, while extension of Runner allows you to apply only one.
Maybe proposed annotation should be called @Plugin.

@dsaff
Member
dsaff commented Nov 22, 2013

@vasiliygagin, you can pass the test instance into a TestRule on creation, and get the method annotations from the Description object.

There are ways this could be made better / easier, but I don't yet see that a new concept is needed.

@vasiliygagin

@dsaff, thank you for information about TestRules.May be it can be done with Test rules. But in reality think about it from the position of developer.I literally write thousands test cases a year. If I need to use Spring feature I specify RunWith. If I use Mockito, I specify another RunWith.
Now, to use TestRule instead, I'd have to write a method in each of my test classes. Think how much clutter I'll have to produce. We try to avoid clutter as much as we can.
I'm guessing that developers of Spring, Mockito, DBunit and other frameworks share that point of view, because mechanism they provide to their clients, is a custom Runner, not a Rule.
And it is very convenient, unless you want use to frameworks simultaneously,

Personally, I think, those Plugins would be the best thing that happened to JUnit in years. But who knows.
Why would not you try. Especially if you see that this will make JUnit easier. At the very least you would get at least one happier developer :)

@kcooney
Member
kcooney commented Nov 25, 2013

We try to avoid clutter as much as we can.

@vasiliygagin just to make sure we understand, are you saying that this:

@RunWith(JUnit4.class)
@ProcessWith(SpringJUnitTestProcessor.class)
public class TestFoo {

  @Test
  public void shouldGoToHomePageAfterLogin() {
    ...
  }
}

...is less cluttered than this:

@RunWith(JUnit4.class)
@ProcessWith(SpringJUnitTestProcessor.class)
public class TestFoo {
  public final TestRule springRule = new SpringJUnitRule(this);

  @Test
  public void shouldGoToHomePageAfterLogin() {
    ...
  }
}

Both are just one line of code. Admittedly, your solution allows you to specify multiple processors in one line, but it also doesn't provide a way for the test author to specify parameters.

@vasiliygagin

Thank you for example. With small corrections you are right.
In both cases we do not need to specify a runner, right?
And do you need a @ClassRule or @Rule annotation for rules?
So it is:

@ProcessWith(SpringJUnitTestProcessor.class)
public class TestFoo {

  @Test
  public void shouldGoToHomePageAfterLogin() {
    ...
  }
}

vs:

public class TestFoo {

  @Rule
  public final TestRule springRule = new SpringJUnitRule(this);

  @Test
  public void shouldGoToHomePageAfterLogin() {
    ...
  }
}

Additional configuration can be passed to "processor/plugin" via another annotations. like @ContextConfiguration in Spring.
I admit that being able to pass any kind of parameters to a rule constructor is a cool thing, like passing this, but it also can be considered a negative thing. Why do I have to pass it all the time?
It also seems that annotations add some expressiveness. You can turn some framework features on like using @Transactional annotation in Spring turns on rollback at the end of test. Curious how you would do this with TestRule? would it be another another rule?

Frankly, I'm not concerned about number of lines. I do about having extra fields.
I guess that is not the way I usually extend some functionality.
Also, currently JUnit forces me to understand that there is a TestRule interface. While annotation approach hides those implementation details from me.

I can't speak for the developers of Spring and other frameworks. I can merely hope they will do provide SpringJUnitTestProcessor. I know I would have implemented it If Processors were available.
I do not think they provide SpringJUnitRule or anything like it, But they do provide a Runner instead.

Please do not take my words as a criticism against Rules in general. They just do not seem natural for plugging frameworks into a test case.

@kcooney
Member
kcooney commented Nov 25, 2013

In both cases we do not need to specify a runner, right?

Correct

Additional configuration can be passed to "processor/plugin" via another annotations.

There are limitations as to what values can be passed via an annotation (String, int, long, Class constants, Enum values, etc). Rules are actual classes, so they are more flexible (for example, you could use a Builder pattern to make a DSL for building the rule instance).

Frankly, I'm not concerned about number of lines. I do about having extra field.

Why is having an extra field a problem?

I can't speak for the developers of Spring and other frameworks, but I do not think they provide
SpringJUnitRule and likes of it, they do provide a Runner instead.

Runners predate Rules, so this could be simply a matter of them not migrating to the new API, or them wanting to support older versions of JUnit. Both of these would also be an issue if we introduced a different API.

Side note: I don't often see the need to use Mockito and Spring in the same test. I use Mockito to mock dependencies of pure JUnit tests. I use Spring for integration tests.

I admit that being able to pass any kind of parameters to a rule constructor is a cool thing,
like passing this, but it also can be considered a negative thing.
Why do I have to pass it all the time?

If your Rule only needs to be used with @Rule (and not @ClassRule), you can use MethodRule:

@Rule public final MethodRule springRule = new SpringJUnitRule()

Unfortunately, MethodRule doesn't currently work with RuleChain.

Please do not take my words as a criticism against Rules in general.
They just do not seem natural for plugging frameworks into a test case.

I'm not taking your words as criticism. I'm trying to understand why the existing APIs (TestRule and MethodRule) don't meet your needs before we add a new API that, on the surface, doesn't add functionality that isn't provided by existing APIs. Having multiple ways of doing the same thing increased the learning curve for users of JUnit.

@kcooney
Member
kcooney commented Nov 25, 2013

BTW, I just found an page that documents how to use Rules for Mockito and Spring: http://www.alexecollins.com/content/tutorial-junit-rule/

If you did this, then you could use the Spring Runner and the Mockito Rule

@vasiliygagin

I would agree, with not mixing Mockito and Spring. I'm pretty sure that that was used as an example only.
Sometimes there is a need to mix Spring with Unitils.
Absolutely agree with the fact there are some limitations on annotation configuration.
And I have no issues with rules being there. I'm only stating that in my experience, when I need to hook up some framework, annotations were enough. And if I'll ever need more, I'll look into rules.
Your guess about why Spring and other developers do not provide rules is as good as mine.
Though I disagree that adding this increases learning curve. It does not take much to learn to add annotation. I know it did not take long for me to learn to add @RunWith(SpringJUnit4ClassRunner.class). It takes little more to learn about TestRules. (as I'm learning now).

As for fields, they do not add value for the test itself, they serve some test wiring purpose (here is the need for learning curve). I personally prefer my test be focused on actual testing. It is easier to understand what test is doing, when there is no extra fields and methods.

@kcooney
Member
kcooney commented Nov 26, 2013

@vasiliygagin Thanks for the explanation. I have to admit I still don't understand the problem with fields. In addition to the limitations about what you can specify in an annotation, you can't get dynamic data out of an annotation (if, for example, you needed direct access to the Spring context, there wouldn't be a clean way to do that).

But you aren't the first to want to use annotations to add custom behavior. I have a counter-proposal.

Usage:

@SpringLoaded(contextPath = "path/to/context.xml")
@RunWith(JUnit.class)
public class TestFoo {

  @Test
  public void shouldGoToHomePageAfterLogin() {
    ...
  }
}

In this case SpringLoaded is a custom annotation defined by you (or, hopefully, the Spring team) that looks like this:

@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
@RuleFactory(SpringLoadedFactory.class)
public @interface SpringLoaded {
    String contextPath();
}

Note that this allows you to pass additional data that your extension point needs, and that data is clearly associated with the annotation. That seems better than this:

@ProcessWith(SpringJUnitTestProcessor.class) 
@SpringJUnitTestProcessorParams(context = "path/to/context.xml")
public class TestFoo {

The RuleFactory annotation would be provided by JUnit. The type of the class passed into the annotation would look something like this:

interface RuleFactory {
  TestRule createRule(Object instance, Annotation annotation);
}

Note that this returns a rule, so we aren't creating a new interface for doing something we can do with existing APIs. See https://docs.google.com/document/d/1B6Z45BSGsY08rZqe2KUZTJ3RVsnUE1-HcXjl5MFm9ls/edit?pli=1 for a related discussion.

@vasiliygagin

@kcooney thanks for you interest in this discussion.

Your proposal is pretty exciting.

But first let me try to explain fields "problem". I'm programming in Java for 20+ years. In old times we were writing code starting from main and calling constructors and reading property files manually. Then frameworks like Spring came along and we pushed some low-level stuff to context / configuration files. Then with addition of annotations we are getting rid of those configuration files too. You know all of that.
I like current state of things in Java world. I can write a very clean Java class where each field and method serves a particular business requirement or use case and all low level / wiring clutter is pushed to annotations or even better completely out if there are conventions of sorts. And I and people around me trying hard to keep our code follow those rules. (It makes it more readable, supportable, etc.). We apply same rules to test cases too. When I see field, I think state. When I see @Rule field, it is unnatural use of field for me. Not a problem though. We still can adapt. But not ideal. Sorry for a long lecture.

Just as FYI, If I need something from plugin / rule one way to get it to declare a field and annotate it with something like @Resource (this annotation plugin specific). and let plugin/rule to inject it. (No low level code here :)

Back to really important thing. I like the annotated annotation. I'm assuming that 2nd parameter "annotation" is passed so factory can extract annotation parameters.
I think that there is a need for a Method parameter on the factory (could be optional). Spring might not need it,
But frameworks like Unitils use annotations on test methods to specify per test data, like xml with validation data.

interface RuleFactory {
  TestRule createRule(Object instance, Annotation annotation, Method testMethod);
}

What do you think?

@kcooney
Member
kcooney commented Nov 27, 2013

When I see field, I think state.

Fields can be used for all sorts of things. If you use dependency injection, I can use fields to store state, references to services used by the class or even configuration values. Users of JUnit have a large variety of coding styles and preferences. We unfortunately cannot cater to all of them.

In the particular case of rules, some of the built-in rules do have state (for example, TemporaryFolder has the path to the directory that was created).

I've heard some people strongly argue against solutions that put data in annotations vs in simple Java classes. Whatever solution we choose, someone will be unhappy.

I'm assuming that 2nd parameter "annotation" is passed so factory can extract annotation parameters.

Exactly.

We don't really need to pass in the annotation, since the factory is passed an instance of the class, and so it could fetch the annotations itself.

The factory class would have to have a public zero-argument constructor.

One drawback of this approach is if you need to define a factory, you need a factory class and define an annotation, which seems like one step too many. But from the user of the factories, the API is easy to understand.

I think that there is a need for a Method parameter on the factory

Not all tests are associated with Java methods.

Did you realize that the Description instance passed to the TestRule.apply() has all of the method's annotations?

For MethodRule, the apply() method is passed in a FrameworkMethod and a target.

Before we move forward on this idea, we would have to iron out a lot of details. For instance, do we allow something similar for class rules? Are the rules that are created via this mechanism run around rules that are defined via fields, or are the rules defined via fields run around the rules defined by annotations? What if the class has a base class that is annotated? What if it has multiple interfaces that are annotated?

@vasiliygagin

It feels like we are beating a dead horse here discussing coding preferences. I'll just say that sure they can be used to inject some services or logic. Lets move away from this part of discussion.

No problem with rules having state, which can come from Annotation or explicitly constructed via @Rule.

When we are talking about API, I think we are on the same page. But just in case I will state this:
There is 1 Junit team, and may be 1000 teams which extend Junit with their functionality and there are millions of people who use Junit. To write test. I think that you would agree that making Junit API clear for them is the 1st goal and making it clear for extenders is 2nd, though it is still very, very important.

When I was proposing this I was mostly thinking about BlockJunit4ClassRunner. Am Hearing you correctly, when you are talking about test cases not associated with Java methods, that you considering to implement this on a ParentRunner level? That would make total sense.

About Descriptions. What annotations would it have in case of non-method base test cases? Probably none?
In this case it is no different, then passing optional Method. Except it is done at different time in test lifecycle.In one case it will be when you constructing Statements, in another when you are executing them. I haven't put any thought into it yet, At first glance execution time is better. But may be both way can be implemented so people can choose. Just thinking aloud.
Though here is potential issue with Descriptions. If it is a representation of a test, java method based or not, why it is filled with Java annotations. May be there need to be abstract representation of annotations there too. I was trying to guess why Java annotations we added to Description. I see in 1 case that framework is checking it test is ignored by looking for @Ignored annotation. But if Junit wants to provide a robust support for non-java method based tests, then there should be a way to ignore those too. That might be a different discussion though.

I think that processors are to be created once per class. And at creation time JUnit should be able to construct them in any order. Though final user should be able to control the order in which TestRules are executed at run time. With respect to ClassRule which is static, Processors which are not static should be constructed later and probably their rules executed later too.

With respect to method rules, If final user writes a rule should have access to the sate processor initialized, For example, processor can inject a datasource and user can write a rule which will use it to init a database. We can assume that processor will not need anything from customer rule implemented by user. So I think processors run 1st and rules after.

As far as annotation inheritance goes, it seems to be a valuable thing. You do not want to run same processor twice. But you probably can aggregate all the annotations from superclasses and interfaces. I'm conserned about the order of procssor execution though. Not sure if Java before version 8 guarantees an annotation ordering when you call getAnnotations(). If not, then we might need something like

@Rules({@RuleFactory1,@RuleFactory2})
@dsaff
Member
dsaff commented Dec 2, 2013

@vasiliygagin,

I'm swimming in this discussion a bit. Would it be possible to boil down to a 50-word paragraph that describes the problem that JUnit isn't currently solving, and what should be added in order to solve that problem?

@kcooney
Member
kcooney commented Dec 3, 2013

@vasiliygagin Lots to respond to there :-)

When I was proposing this I was mostly thinking about BlockJunit4ClassRunner.
Am Hearing you correctly, when you are talking about test cases not associated
with Java methods, that you considering to implement this on a ParentRunner level?

Possibly yes, or possibly we put the code in BlockJunit4ClassRunner and provide APIs that allow other custom test runners to get the same behavior. I'm just saying that not all tests have methods, so passing in a Method into this proposed API is questionable.

So my first question to you is this: can you give me a concrete example of behavior you want to change that couldn't be done via the MethodRule or TestRule APIs? Ignore for a moment how we attach that behavior to the test itself. I'd first like to understand why these two extension points don't allow you to do what you want.

I think that processors are to be created once per class.

Why once per class, and not one per instance? Almost everything else (except the runner) is created once per test instance. The reason for that is we want to minimize the possibility of having the result of one test method depending on the outcome of a previous test method.

And at creation time JUnit should be able to construct them in any order.

How would the test writer indicate the order? Currently, for TestRule users can use RuleChain to specify the order within a test class. I can't see a clean way of specifying the ordering rules (or test processors) specified via annotations compared to rules specified via @Rule.

I think getting a good answer to this would be required before we can go forward with a proposal to declare rules (or rule-like constructs) on tests via annotations.

Not sure if Java before version 8 guarantees an annotation ordering when you call getAnnotations().

The Javadoc for JDK 7 is sadly silent about the ordering. Do you see anything in JDK 8 that specifies an ordering?

If not, then we might need something like @Rules({@RuleFactory1,@RuleFactory2})

That doesn't provide a clean way for the rule factories to get whatever data they might need. If we had the above API, the only way to get data to those factories would be to require more annotations on the class, and then the person reading the code would be left figuring out which annotations applied to which factories.

Sorry for all the questions, but getting APIs like these correct is difficult. We have to think beyond the simple cases to the more complicated possible usages and how the APIs interact with existing APIs.

@vasiliygagin

I'll try to summarize rationale for proposed change.
Problem (from test coder point of view):
99% of my tests are enhanced with some framework: Spring or Mockito or Unitils or custom-made. All of which provide custom runners.
Occasionally (rare) I need to enhance test case with 2 frameworks But I can't apply 2 Runners.
Can this be solved with TestRules: in theory yes. But framework developers do not provide them. And I do not want to write those integration rules myself.

One can say that the problem is not with JUnit, but with framework developers.
I think part of the blame is on JUnit. JUnit provides at least 2 extension points: class annotation @RunWith and filed and method annotation @Rule. It seems that frameworks I use all favor class annotation. Despite the fact that it creates this problem.

There was a thought that it just a matter of time, before framework developers will implement rules. They probably will. But I'm facing this problem for several years now, and I'm trying to facilitate the solution with this proposal.

I also believe that not everything that can be done, must be done. Before runners and Rules we were doing tests. That can be done. We were producing way more unnecessary code though. It is better now. And it can be even better.

For me as a test coder and occasional junit "extender" ideal solution would be a class level annotation:

@Plugins({SpringPlugin.class,OtherPlugin.class})

There were variations on this . but these are for later discussions.

@dsaff
Member
dsaff commented Dec 3, 2013

@vasiliygagin, have you tried asking one or more of the teams you mention (Spring or Mockito or Unitils) why they haven't provided Rules yet?

If it's just that they don't have enough time, then they won't suddenly get more time if we add yet a third extension point. If they've looked at it, and there's technical problems with Rules that prevent their use, then I'd love to either fix Rules, or consider additional extension points like yours, in order to solve those problems.

@vasiliygagin

Just found a Spring Jira related to this: https://jira.springsource.org/browse/SPR-7731
It is going for several years now. You can read it in several ways. The way I read it, implementation based on rules is not attractive enough. It seems that their thought is that JUnit will benefit from a "broader interceptor model" which is not far from what I was suggesting.
There is one comment on that jira which summarized my feelings about this:

Good luck with getting that into JUnit.

I'm not getting lucky, Am I?

@dsaff
Member
dsaff commented Dec 5, 2013

@vasiliygagin,

Even more relevant is spring-projects/spring-framework#222, which proposes a Rule for Spring. Doesn't look like anything's happened on it for 10 months. Would it work for your purposes?

I honestly don't remember what happened with Neal's JUnit patches, I'm sorry to say.

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