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.

Sign up for GitHub

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

[SUREFIRE-1266] Skip junit dependency check if module has no tests to run #388

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[SUREFIRE-1266] Skip junit dependency check if module has no tests to run #388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions ...-surefire-common/src/main/java/org/apache/maven/plugin/surefire/AbstractSurefireMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -924,7 +924,7 @@ public void execute()
setupStuff();
Platform platform = PLATFORM.withJdkExecAttributesForTests( getEffectiveJvm() );

if ( verifyParameters() && !hasExecutedBefore() )
if ( verifyParameters( false ) && !hasExecutedBefore() )
{
DefaultScanResult scan = scanForTestClasses();
if ( !hasSuiteXmlFiles() && scan.isEmpty() )
Expand All @@ -944,6 +944,7 @@ public void execute()
return;
}
}
verifyParameters( true );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

second call? in one methd.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CMoH @slawekjaranowski
Here I can see that the method verifyParameters() is called twice. Why?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The short answer is:

  • the first call is meant to run the fast parameters check, also determining if the plugin is enabled
  • the second is meant to run more checks, those that are only applicable to modules that do have test classes.

I believe such a split is necessary, since the test getTestClassesDirectory().exists() at the beginning of verifyParameters() is insufficient. The test directory might exist, empty, or containing files/classes which don't provide tests to run.

The actual problem for me here is warnIfDefunctGroupsCombinations(), which should not crash for modules that don't have tests due to parameters inherited from a parent pom.

The correct, more resource-expensive check, is done by scanForTestClasses(), and the second call to verifyParameters() only has any effect if there are indeed tests to run (and failIfNoTests==false).

The second benefit is that the splitting the responsibilities of verifyParameters() into two methods (although I only started with a quick fix via a boolean parameter) would help as placeholders to continue refining the validation code.

I touched on this also in this comment below.

I am able to assist with changes, if you feel this path is helpful to the plugin.

logReportsDirectory();
executeAfterPreconditionsChecked( scan, platform );
}
Expand Down Expand Up @@ -1101,7 +1102,7 @@ else if ( out.isDirectory() )
}
}

boolean verifyParameters()
boolean verifyParameters( boolean pluginActive )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does pluginActive means ... ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the name choice is not be the best. Please note that I created this to prototype a way to solve it, as well as to point out this design issue visible in verifyParameters() (which does an insufficient, but speedy check, if the test folder exists in a module). The second, more expensive check, is done in scanForTestClasses().

The reasoning behind it is that some fast parameter validation can be done based purely on configuration, while a better validation can be done after inspecting the test classes, which enables more precise heuristics.

This pointed me to the thought that there is an opportunity to implement these two phases: some parameters need to be checked before looking at tests, to determine if it's worth looking for tests. Next some parameters are not worth checking unless certain test providers and annotations are actually in use (the second phase).

I think your decision to make is whether such a design is desirable or not. I can adjust the contribution for a better design, if desired.

However, I noticed in the issue you want to solve it in a different way. Please close this merge request if you don't need it anymore.

throws MojoFailureException, MojoExecutionException
{
setProperties( new SurefireProperties( getProperties() ) );
Expand Down Expand Up @@ -1138,7 +1139,10 @@ boolean verifyParameters()
ensureParallelRunningCompatibility();
ensureThreadCountWithPerThread();
warnIfUselessUseSystemClassLoaderParameter();
warnIfDefunctGroupsCombinations();
if ( pluginActive )
{
warnIfDefunctGroupsCombinations();
}
warnIfRerunClashes();
warnIfWrongShutdownValue();
warnIfNotApplicableSkipAfterFailureCount();
Expand Down
2 changes: 1 addition & 1 deletion ...efire-common/src/test/java/org/apache/maven/plugin/surefire/AbstractSurefireMojoTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2022,7 +2022,7 @@ protected String getEnableProcessChecker()

e.expect( MojoFailureException.class );
e.expectMessage( "Unexpected value 'fake' in the configuration parameter 'enableProcessChecker'." );
mojo.verifyParameters();
mojo.verifyParameters( true );
}

private void setProjectDepedenciesToMojo( Artifact... deps )
Expand Down