Maven Surefire

I'm marking this won't fix, though I could be convinced otherwise. Forgetting to add @Test annotations is a common error for users upgrading from JUnit 3, and even occasionally for experienced users. Naming a class *Test without making it abstract and without putting any @Test annotations is quite unusual, and probably shouldn't be silently ignored.

This is more to address the situation where mock classes named *Test are not run by Surefire. Similar to JUnit3 where classes that do not extend TestCase are not run by Surefire (SUREFIRE-346), the defining factor of a JUnit4 test case is the occurrence of one or more @Test annotations, hence for consistency Surefire should ignore classes that do not contain any @Test annotations.

This also mirrors the behaviour in Eclipse, whereby:

• a JUnit3 test case can only be run if it extends TestCase
• a JUnit4 test case can only be run if it contains one or more @Test annotations

It'd be good to have consistency across Maven and IDEs.

Excuse me i don't understand why you fixed this same problem for Junit 3 (SUREFIRE-346) and you refuse to solve it for Junit 4.
Why you should put a restriction on class names when Junit does not?
As Mark pointed out, don't you think that behavior consistency, given that this request doesn't violate any rule, should be more important than anything else?
Thanks, Gabriele

Just to clarify better what i mean, why surefire should try to run a class just because it is named "Tester*" when that class does not extend TestCase nor has any Junit 4 annotation in it and it doesn't even import from any junit.* package?

Note that instead a class named "*Tester" is happily ignored.

It looks like surefire is violating its own rules

This is an issue for me too.

"Naming a class *Test without making it abstract and without putting any @Test annotation" does not seam unusual to me.

Mock object are one case.
Another one is starting a new project and creating a test class not having implemented any test methods yet. Having to make it abstract is an unnecessary nuisance. What is worse, you then have to remember to make it non-abstract again once you have written the first test.
Another case is wanting to temporarily disabling tests by annotating them with @Ignore. It is inconsistent that you can do it UNLESS there are no more methods with @Test annotation left in the class in which case the exception is raised.

In the end even if these cases were unusual they are not in any way an error and should not be reported as such and should not break a build.

I hope someone is convinced one day that this should be fixed

This blows my mind. Having just switched to maven for builds, it's ridiculous that I now have to come up with a set of naming patterns for tests that have never needed them previously. Blech.

There seems to be enough dissatisfaction with this issue to warrant reopening it. What do you think Dan, shall we take it to the mailing lists for a vote?

This has also bitten me more than once. Maven shouldn't get in my way too much and it should definitely NOT dictate how I have to name my test support classes (TestUtils.java anyone?).
I'm in the lucky situation where I started a new project with Maven, so it doesn't hurt too much. But imagine I would migrate an existing project with dozens of "Test*" classes which are no test cases, this would be a blocker to switch to maven.

This behavior should be configurable at least.

I would also vote for reopening this bug.

Perhaps, this solution would work:

@Ignore @Test
public void ignoredMethod() {...}

while the following one results in "java.lang.Exception: No runnable methods":

//@Ignore @Test
public void ignoredMethod() {...}

In my current project, support classes for testing can also be named like *Test.
I don't see why this is so strange, and why surefire cannot silently ignore this. The class doesn't extend testcase (which is by my knowledge required by JUnit 3) and it does not have any annotations (Isn't this also required for JUnit 4?)

This generates a lot of false failed tests.

Please fix this.
I don't think its reasonable to fail if you have non-junit-test-classes coincidently named Test* or *Test.
Use the annotation, that is what it is there for.

Reopening this bug as there's obvious dissatisfaction with the outcome. Please vote if you'd like to see it fixed.

I just got bit by this behavior and it is very annoying. the rule 'a JUnit4 test case can only be run if it contains one or more @Test annotations' seems to the correct and simple solution to the problem.

This is an issue with anonymous inner classes in a test class, that themselves do not contain @test's:

initializationError0(x.x.x.x.x.TestHomePage$1)  Time elapsed: 0.002 sec  <<< ERROR!
java.lang.Exception: Test class should have public zero-argument constructor
	at org.junit.internal.runners.MethodValidator.validateNoArgConstructor(MethodValidator.java:56)

NB: TestHomePage is an actual junit, but contains at least one anonymous inner class which is not.

Very annoying... To fix this, I'll have to rename 'test' out of the class name. Ugh.

I also have "TestPages" that are not actually junit tests, but SureFire still tries to run them as junits. Workaround was to put them all inside a package, and exclude it manually.

Oh dear - it only seems to run test classes that are names Test or Test - crap. I thought that was for detecting junit3 tests only. :/ this issue just got worse for me. Seems like surefire can't run my tests then?

I would also like to see surefire's JUnit 4.4 support fixed so that it matches what JUnit 4.4 does (i.e. run classes that contain @Test annotations). Currently, surefire requires that the class name contain "Test" in order for the class to be considered a test class. A workaround is to use <includes> and/or <excludes> inside the plugin configuration to include or exclude specific classes.

We had the same thing happen that @Antony Stubbs had.

x.x.x.x.TestDriveSecurityServiceTest$1.initializationError0 =================
java.lang.Exception: Test class should have public zero-argument constructor

Renaming the class to TDriveSecurityServiceTest solved our problem.

Hi, I would also like this to be fixed. I do want to name my classes as I wish without them to be executed as tests, especially when I use junit 4 annotations to specify what classes are actual tests or not.
Thank you !
Thomas

I'm a relative newcomer to Maven and this one bit me too. Seems odd that I should have to rename my class called DomainTestCase to something without Test in it in order to get Maven to process my tests without complaining. This really should be fixed.

It should be fixed. I have classes named Test* that are about some completely different kind of tests (i.e. do not talk about unit testing) that are in /src/main/java (not /src/test) and that surefire tries to run with no reasons.

A good workaround is to add @Ignore to the class that shouldn't be run.

This should be fixed, for me this is no real junit4 support if I need a naming-convention for my classfiles.

This bug is an embarrassment to Maven. Ignore the JUnit4 standard protocol and instead invent your own undocumented test discovery method! 2 years have passed since this was reported, and its still broken...

This is one of most annoying bug/feature I have ever come across. How can you go against the grain of Junit, it is like breaking the law. I want to disable all tests in my project and start fixing them one by one by having a Cobertura report that starts from 0 and goes up as we fix them. I took great pain in deleting/commenting the @Test and other annotations and also added a prefix 'skip' to all the test methods. And after all the effort I see this bug and the plugin owner's disgusting behaviour.

This is the second most popular issue for Surefire so we need to resolve it. First step would be for someone to contribute a patch that fixes this behaviour.

I took a quick peek in the source code. The logic that decides what to include is in the surefire-api artifact (maven-surefire-plugin -> surefire-booter -> surefire-api). In the plugin a default filename include pattern (Test*.java, *Test.java, *TestCase.java) is defined and that is handed over to the collectTests() method in the SurefireDirectoryScanner class. This method uses org.codehaus.plexus.util.DirectoryScanner to simply scan for files matching the patterns. There is no checking on extending of TestCase or annotations what so ever.

So.

It would not be too difficult to examine the resulting list of classes for these things. The problem is; the code at this level does not know what the actual test environment is, it just handles files. It has no awareness of JUnit3, JUnit4 or possibly other testers. I have not enough understanding of SureFire at this time.

Ok. I figured it out; in surefire-providers there are classes for different testrunners. A.o. there is a JUnit4DirectoryTestSuite which extends and heavily relies on AbstractDirectoryTestSuite which uses the directory scanner. This class should get additional code to filter the resulting classes on the @Test annotation. Likewise there is a JUnit 3 class which should filter on "extends TestCase".

I'll see what I can do.

Oh my, that was easy. JUnit4DirectoryTestSuite has a method that actually creates the test runner and it is allowed to return null. So all that needs to be done is check for a method with the @Test annotation. If not found, return null.

Here it is:

protected SurefireTestSet createTestSet( Class testClass, ClassLoader classLoader )
throws TestSetFailedException
{
// this is JUnit4: check to see if there are any methods with a @Test annotations
boolean lTestFound = false;
for (Method lMethod : testClass.getDeclaredMethods())
{
for (Annotation lAnnotation : lMethod.getAnnotations())
{
if (org.junit.Test.class.isAssignableFrom( lAnnotation.annotationType() ))

}
}
// if not test found, simply return null
if (lTestFound == false) return null;

// create the testset
return new JUnit4TestSet( testClass );
}

Please note that there is a solution to this issue already:

Surefire contains three different Junit providers, 3.x, 4.x and the (parallel) 4.7+ provider.

The 4.7 provider only runs valid junit tests, and it is possible to force the use of this provider even for serial runs, using this (somewhat exotic) configuration setting: <parallel>none</parallel>. As long as junit 4.7+ is used, this will only run proper junit tests.

Please note that due to a few bugs in the 2.5 junit 4.7 provider, it is probably wise to test this with 2.6-SNAPSHOT or better.

The fixed source for JUnit4.

IMHO adding those 6 lines of code to the JUnit4 provider is a better solution that the exotic configuration.

Thanks for the patch, Tom.

As can be seen from several of the comments to this issue, there is the problem that this bug has been around long enough to become a feature (pardon the pun). I also know from experience that this "feature" is used by quite a lot of projects.

I believe that any patch to this issue will need to include one or more of the following features:
A) Solve the issue by making the provider-switch more explicit (i.e. leave 4.x provider as-is)
B) Solve 4.x problem more or less as you suggest, but provide log warnings about classes that will not be run any more due to behavior change. While this initially may seem like a nice fix, the problem is that the message will have to be more or less permanently in place - which means we'll be nagging users indefinitely due to what is basically a bug.
C) Make some kind of "strictJunit4classscanning" attribute on surefire plugin that allows the correct behavior to be turned on/off.

I'm sure there's may be other ways to fix this, but I think the fix (in one way or another) needs to respect that it's become somewhat of a "feature".

There also needs to be an integration test for whatever fix is chosen, after all it's a test framework

Wow, I totally missed the discussion here. Please see my comment and patch at SUREFIRE-535.
I think this must be fixed. I can't see a use case when skipping a class without test methods would cause problems. Kristian, could you please elaborate on this? Why would anyone want the test without test methods to fail?

I will fix this issue for 2.7 as follows:

• Impelement the correct behaviour.
• Adda -Dsurefire.upgrade=true system property that can be run when upgrading, so that users will have warning if their project contains testcases that will not be run anymore (this is the reason why this issue is a bit hard to fix).
• Include a warning to run (at least once) with this flag when upgrading in release notes.

Fixed in r1033896. Users upgrading to this version of the plugin should run mvn -Dsurefire.junit4.upgradecheck=true install at least once to verify that they don't have any tests that will be ignored by the new plugin version.

Kristian, thank you for fixing this.

However, I think I've found a bug that could lead to more serious consequences than we have thought of.
When checking 'value' parameter for @RunWith attribute
instead of "if ( suite.isAssignableFrom( value ) )"
it should be "if ( runner.isAssignableFrom( value ) )"
or better yet "if ( true )".

As it is now, classes with custom runners, or even with a standard Theories runner could be ignored. Should I create a new issue with a test?

That's really good catch and a nice simplification ! r1033985 just scans for the presence of @RunWith, testcases updated.