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

All JUnit annotations should be able to be applied as meta-annotations #194

Open
nealeu opened this issue Feb 24, 2011 · 14 comments
Open

All JUnit annotations should be able to be applied as meta-annotations #194

nealeu opened this issue Feb 24, 2011 · 14 comments
Labels

Comments

@nealeu
Copy link

nealeu commented Feb 24, 2011

As stated in https://github.com/KentBeck/junit/issues/100, it would enhance the DRYness and hence maintainability and readability of JUnit tests if @RunWith(SomeCustomRunner.class) could be specified in a single place, alongside other annotations to form testing meta-annotations.

At the moment it is suggested that SpringJUnit4Runner should be a @Rule (which has become the subject of issue 100), yet this migration would still require the repetition of:

@Rule @ClassRule 
public static TestRule springRule = new SpringContextRule();

At class level, and example would be @IntegrationTestSuite (which could contain custom @RunWith and/or @Rule (assuming https://github.com/KentBeck/junit/issues/#issue/32 is implemented).

At test level, allowing @Test to be on a meta annotation would allow standard timeouts to be declared, such as a meta-annotation @PerformanceCriticalReadOnlyTest with the annotations @Test(timeout=100) @Transactional(readOnly=true)

@nealeu
Copy link
Author

nealeu commented Feb 24, 2011

Some initial work for this (adds AnnotationUtils and works for @RunWith), can be found at https://github.com/nealeoc/junit/tree/meta-annotations

@dsaff
Copy link
Member

dsaff commented Mar 2, 2011

Neal,

This change has a trade-off: it provides one way to enhance DRY-ness, but. misused, could hamper the readability of JUnit tests, and the speed of tools that want to statically determine if a java file contains a test class or not. Could you open a discussion on junit@yahoogroups.com to gather some opinions on whether users and extenders are happy with this trade-off? Thanks.

@nealeu
Copy link
Author

nealeu commented Mar 3, 2011

If "misused", yes it could hamper readability, but that's true of existing features, Java and the English language ;-)

I don't think preventing misuse of a good feature is a reason not to have the feature, when if used well, can considerable help development.

On the tooling front, yes, I'll start a discussion, although I don't think this proposal is a breaking change. At worst, meta-annotations would only be incompatible when used, with current generation tools. This is true of most new features.

Is there any reason not to get this going in the 4.9 beta cycle and get feedback prior to release?

@kcooney
Copy link
Member

kcooney commented Mar 4, 2011

My first impressions after reading the message on the mailing list, and before reading the comments here, was similar to David's. Although I am a fan of DRY, it should not be done at the extent of clarity, and I think that the current proposal would have a significant impact on readability.

If you look at Google Guice, it allows you to define custom annotations to define injection points:

public class Robot
  @Inject @Left
  private Leg leftLeg;

  @Inject @Right
  private Leg rightLeg;

  @Inject
  Robot(FluxCapacitor c)
}

In the above code, @Left and @Right are binding annotations that qualify how Guice will decide which Leg object to inject.

The Guice developers could decided that @Inject was not necessary if you had a binding annotation, but they did not. I suspect they decided that it was important for the reader of the code to know that these are injection points. I do know as a developer in a very large code base that uses Guice that the code would be much harder to read if the @Inject was optional.

Going back to JUnit, I think the @Test annotation is similar. It states something important: this method is a test method, and will be called by the JUnit framework. Hiding that information will make tests harder to read. It would also lead to a number of places where there could be conflicts (what if someone adds @test with a timeout to a test that's annotated with a method that also provides a timeout?).

Overall, it feels like the driving reason for this change is that Rules have some limitations. If you could simply add one line to each test to indicate that it should use a specific rule, and rules were powerful enough to affect the test suite and test case level, then would this be needed?

@nealeu
Copy link
Author

nealeu commented Mar 5, 2011

Sorry to be blunt, but I think you're both being a bit patronising of developers.

The arguments that you are making are a great argument against inheritance, interfaces, traits (i.e. intertype defs).

The whole bloomin point of many programming language features is to provide abstraction. In fact, the default for @target(ElementType.TYPE) is actually that they are allowed to be applied to annotations. It's just that many don't provide that support when using annotations.

Please guys. Give us some flexibility in how we use JUnit, rather than having JUnit being a restriction on the expressiveness of our tests and forcing us to repeat everything, including documentation.

@kcooney
Copy link
Member

kcooney commented Mar 5, 2011

Neal, if my tone sounded patronizing, it was unintentional and I apologize. I do hope we can have a reasonable discussion on the pros and cons of your proposal as well as alternatives.

Also note that I'm just another developer that uses JUnit (occasionally providing the occasional patch). I have no ability to accept the pull requests.

Besides the points we discussed, there is also a concern that existing tools (like IDEs) are using those annotations, and developers using those tools would probably get broken behavior with those tools if they used your new approach. Could you verify that with your changes Eclipse will still support "Run with... JUnit test" at the class and method level for test classes using meta-annotations?

Looking again at the example that motivated this proposal, all of the annotations but two are from the spring Runner. JUnit requires one at the test level (@test) and, if you have a custom runner, one at the class level (@RunWith, which can be specified in a base class I believe). That sounds very reasonable to me.

One alternative: find a way for the Spring runner to use meta-annotations so you can follow DRY. The Spring runner could look at the annotations of the test method to see if there is a timeout indicated, and if not, provide a default timeout.

@nealeu
Copy link
Author

nealeu commented Mar 6, 2011

Sorry Kevin. Frustrating day.

I'm not that concerned about existing tooling given that to use meta-annotations is optional. The IDEs can catch up. I know my way around the Eclipse JUnit support having contributed some work better compatibility with surefire generated XML reports. A patch to the launcher code wouldn't be a problem.

It's a parallel to having new features in the Java language. It makes life easier for developers, but the language spec has to lead the IDE.

I think that if we standardise on supporting meta-annotations, then it allows some wins for developers now, and with some further options once IDE support is more standard.

The @inherited annotations (i.e. being able to specify them in a base class) is only of limited use, as on projects of any size, we soon find there are other reasons we want to use inheritance and Java only gives a single class to inherit. Annotations give us more trait-like alternatives which are doing us proud.

I think the only concern we really have to answer here is: Will we break anything and cause people a reason not to adopt 4.9?

As this requires no API changes, I can't see any way in which we can break things, even if people are extending the runners.

@dsaff
Copy link
Member

dsaff commented Mar 7, 2011

Re: IDE's:

It's excellent that you're in a position to shore up the JUnit support in Eclipse. However, I'm less concerned about whether the IDEs can catch up, and more concerned about the cost when they do. Many IDE's provide the ability to automatically run all of the tests in an area of the code base. Won't this change make that search more expensive (or necessarily incomplete)? If that's so, shouldn't we spend some time considering other ways of achieving the same goals that are less expensive in tool effort? I have some other ideas, if you're interested.

@nealeu
Copy link
Author

nealeu commented Mar 8, 2011

If you've got ideas, I'd be glad to hear them. I'd suggest creating another issue so we can discuss each.

The search cost issue is not a problem. I'd be very curious if someone did manage to make it a problem. To scan for @test, for example, we are loading every class that may contain tests and checking for annotations on the methods. If someone decided they wanted to create two or three stereotypes of @test, then we've got 2 or 3 extra classes loaded and a check on the annotations on those classes.

It's the sort of change that goes with auto complete now being standard... we've gained the processing power to give developers more freedom.

When ramping up a team of 20 or 30 developers, the responsiveness of the IDE is the last concern I have.

@dsaff
Copy link
Member

dsaff commented Mar 8, 2011

Neal,

To add two data points, search cost is an issue on Eclipse. Search cost is an issue at Google. For a proposal that avoids such costs, see: https://github.com/KentBeck/junit/issues/202

@nealeu
Copy link
Author

nealeu commented Mar 9, 2011

When you say "search cost is an issue", can you give a specific scenario? I'm interested in how that compares to say Spring IDE, which already searches for meta-annotations.

@kcooney
Copy link
Member

kcooney commented Mar 9, 2011

Many operations in Eclipse are O(number of jars on the classpath). You don't rally notice the slowdown with smaller code bases. Some Googlers have contributed patches to speed some of those, but I think some may be unavoidable without major changes.

If it comes down to the speed being the deciding factor in putting this feature in junit-core, I volunteer to write and publish a micro-benchmark.

I am concerned about tools. It would take time for the various tools to be updated to use the meta-annotations. Since we are talking about replacing the annotations that are used to discover tests and test classes, it's possible that some tools would find a different set of tests than other tools, leading to tests being silently not run.

Although this is being proposed as an optional feature, the developers using the feature might be unaware of which tools do not support meta-annotations. Sometimes, developers are not aware of all of the tools being used (in some organizations, release engineers use different tools, for example).

I suggest not using meta-annotations to replace @test for this reason. The @test only adds one annotation per method, and it's pretty useful for humans, IMHO, to know which methods are tests (especially in a code base with many custom annotations). I think this is less of a problem with @RunWith, because once the tools identify the test classes or test methods, they should call the JUnit code to run the tests.

One thing that @test does provide is an ability to set the timeout for the test. This is deprecated in favor of the Timeout Rule, but there currently isn't a way with Timeout to specify a different timeout for different methods. You might want to file a feature request for a Timeout.Override annotation that can be applied to a method to override the timeout specified in the Timeout constructor. Then you could put Timeout.Override on your meta-annotations so the tests would use the same override for all tests with a given meta-annotation.

@UnquietCode
Copy link

I'm joining this discussion a bit late, but this still matters to me, so here goes...

I agree that some annotations would benefit from being used as meta-annotations. @RunWith is an example of one which I would very much like to see detected in this way. Looking at the argument for @test however I would say that, no, that one probably should not be used as such.

So perhaps the next steps are to decide one annotation at a time how they should be detected and consumed by the test framework. Is it worth opening a new issue specific to each annotation to be discussed?

@marcphilipp
Copy link
Member

@UnquietCode Anything else besides @RunWith?

codecholeric added a commit to TNG/ArchUnit that referenced this issue Feb 9, 2020
This PR enhances `archunit-junit5-api` to support meta-annotations for `@ArchIgnore`, `@ArchTag` and `@ArchTags` (using JUnit 5's [AnnotationSupport](https://junit.org/junit5/docs/current/api/org/junit/platform/commons/support/AnnotationSupport.html)).

I left `archunit-junit4` unchanged as JUnit 4 itself does not support meta-annotations (junit-team/junit4#194).
codecholeric added a commit to TNG/ArchUnit that referenced this issue Feb 21, 2021
This PR enhances `archunit-junit5-api` to support meta-annotations for `@ArchIgnore`, `@ArchTag` and `@ArchTags` (using JUnit 5's [AnnotationSupport](https://junit.org/junit5/docs/current/api/org/junit/platform/commons/support/AnnotationSupport.html)).

I left `archunit-junit4` unchanged as JUnit 4 itself does not support meta-annotations (junit-team/junit4#194).
test-git-x-modules-app bot pushed a commit to vs/junit4 that referenced this issue Sep 13, 2022
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

5 participants