junit-team / junit4

A programmer-oriented testing framework for Java.
https://junit.org/junit4
Eclipse Public License 1.0
8.53k stars 3.28k forks source link

JUnit should process annoations in package-info.java #670

Open brianegge opened 11 years ago

brianegge commented 11 years ago

It would be useful to be able to add package level annotations to package-info.java. For example adding @org.junit.Ignore to the package should ignore all tests in that package.

dsaff commented 11 years ago

FYI, I think we looked into this years ago, and found that package-level annotations were not available at runtime. If you find information that this is not true in later versions of Java, please let me know.

brianegge commented 11 years ago

I see a reference to package-level annoations here: http://www.onjava.com/pub/a/onjava/2004/04/21/declarative.html?page=3

Also, I can see the annotation in the class file:

$ strings package-info.class
SourceFile
package-info.java
RuntimeVisibleAnnotations
Lorg/junit/Ignore;
flpa commented 11 years ago

Would this be a feature worth looking into? Checking annotations on package-level seems pretty straightforward:

TheTestClass.class.getPackage().isAnnotationPresent(Ignore.class);

According to javadoc, this method exists since JDK 1.5.

dsaff commented 11 years ago

Hmm. I wonder what convinced us this wouldn't work?

Assuming we were just crazy, we'd still have to proceed with caution:

1) a developer who didn't pay careful attention to release notes might be surprised to see her test classes start changing behavior without any local changes.

2) We'd have to think about how package annotations would match up with superclass annotations.

I think I'd be open to something like an opt-in @Packaged annotation which meant "package annotations apply to this class".

brianegge commented 11 years ago

I think the package info class is so little used that a change would not likely have much of an effect. I seriously doubt too many other people have tried using package level annotations for JUnit, for if they had, someone in the past few year would probably have posted "Me too" on this request. I do think there are a number of use cases for package level annotations, and if supported, it would see some adoption.

flpa commented 11 years ago

@brianegge I think it's not about the immediate effect that this feature might have but about the additional layer of complexity it adds when trying to figure out why a particular test has been ignored, for example. Probably that's a matter of inter-team communication, though... At least I'd expect a colleague starting to use package-level annotations or changing such settings in packages that might affect me to drop me a line.

I guess this feature could both be quite useful, when used with care, and annoying, when you're desperately looking through class and superclass commit histories to find out why your test is behaving differently now.

Another thing that came to my mind while thinking about this is inheritance to sub-packages. I guess as soon as package-level annotations are supported, someone might ask for inheritance as well...

@dsaff, regarding 2): I would probably expect package-level annotations to overrule superclass annotations - but that's just my first thought. I'll also think about the opt-in approach.

kcooney commented 10 years ago

(going through old bugs)

Before proceeding with this, it would be useful to know what annotations other than @Ignore make sense at the package level. This would help, for example, in deciding whether package-level annotations would overrule superclass annotations.

brianegge commented 10 years ago

Probably just the ignore annotation. Otherwise, being able to set a default timeout for all classes in a package would be useful, but I don't think one can do that at the class level today.

marcphilipp commented 10 years ago

As @dsaff, I'm in favor of an opt-in annotation on test classes, so there's at least a small hint there. I think @InheritPackageAnnotations would be a better name.

kcooney commented 10 years ago

I'm not so sure about the opt-in annotation idea. If we required an opt-in annotation, and @Ignore is the only use case, then I don't think people would use this feature. When developers want to disable all classes in a package, I think would end up just annotating the classes with @Ignore rather than annotate the package-info with @Ignore and updating every class to to have the opt-in annotation.

dsaff commented 10 years ago

@kcooney, the idea was that a team would agree to add @InheritPackageAnnotations to all the classes in their project, and, having thus opted-in in advance, they could use package annotations later to @Ignore out a package.

kcooney commented 10 years ago

@dsaff I understand the idea. I just doubt many teams would add @InheritPackageAnnotations to all the classes in their project, since it looks like noise. I hear a lot of people wanting to create base classes for their tests because having one @Rule is "too much noise".

In any case, is it all that common for a team to want to wire off all current and future tests in a package?

marcphilipp commented 10 years ago

As an alternative to @InheritPackageAnnotations we could check for a certain system property, e.g. "org.junit.InheritPackageAnnotations=true". Thoughts?

brianegge commented 10 years ago

Package annotations are used by Javadoc, JAXB, Struts, Hibernate, javac and other popular Java tools. In none of these do you need to opt-in in order to use the features. One can mark an entire package as @Depreciated. If someone was trying to figure out why they were getting a depreciation warning, it might take them a few minutes to find it in the package-info. Package annotations have been part of Java for nine years now, so while they aren't used terribly often, they aren't entirely obscure either.

On Jan 29, 2014, at 12:42 AM, Marc Philipp notifications@github.com wrote:

As an alternative to @InheritPackageAnnotations we could check for a certain system property, e.g. "org.junit.InheritPackageAnnotations=true". Thoughts? — Reply to this email directly or view it on GitHub.

dsaff commented 10 years ago

@kcooney, I think we might be in violent agreement. I agree that the feature feels unlikely to be frequently used in practice, and even more so if it requires per-class opt-in. But I really don't like the idea of supporting it at the core level without opt-in, so the feature in general would have to be sold better.

@marcphilipp, you raise an interesting point. We already have a command-line flag for filters, which could be used to simply not run a given subset of packages.

@brianegge, I've tried hard to hold a line to limit the number of places that could affect the execution of a test class, because I believe that test code ranks quite high on the kinds of code where one person is often called upon to quickly understand the execution of code written far away by someone else. In general, my feeling has been that if a feature is not useful enough to merit adding a single line to the Java classes it would affect, then the potential confusion cost is higher than the feature's benefit.

adrianosimoes commented 7 years ago

Is this still a valid issue?

kcooney commented 7 years ago

@adrianosimoes I don't think the maintainers are thrilled with the idea of processing package-level annotations. I think we should leave it open in case someone has a use case other than @Ignore where it would be useful to process package-level annotations. If @Ignore is the only supported feature, then it doesn't seem very compelling IMHO.

kcooney commented 7 years ago

One use case just came up (@RunWith). I could imagine wanting to specify @Category at a package level as well. For @RunWith it is clear what happens when the package has the annotation and some classes in the package are also annotated. For @Category the desired behavior is less clear.

brianegge commented 7 years ago

Supporting package level annotations would be straight forward to do. However, the number of people actually using them would likely be between zero and one. It appears there is little demand for this feature, so it's probably best to close this request.

kcooney commented 7 years ago

There is discussion about possibly using them in #1423

cesar1000 commented 7 years ago

@brianegge I disagree. A package-level @Ignore annotation as you suggested in your original request can be really useful, as there's no convenient way to disable a set of test un current JUnit. So is a @RunWith annotation for defining a default runner for a set of tests.

As to whether this would see adoption: maybe not if annotations need to be added manually, but IDEs could use this feature and expose it through a high level interface.

kcooney commented 7 years ago

From a discussion in #1423

it appears that Java will only initialize Package objects when first loading a class from the package. This is ok if annotating the package containing the tests, but won't work with parent packages (unless adding placeholder classes to these packages and loading them explicitly, but that's a bad hack).