SUREFIRE-1383 dependenciesToScan Does Not Leverage Classpath Elements

[owenfarrell]

Added logic to prefer output directories of projects built as part of the current session.

[Tibor17]

Pls squash both commits to one.

[Tibor17]

Pls concatenate as follows mavenProject.getGroupId() + ":" + mavenProject.getArtifactId().
This is simple and javac would compile to StringBuilder anyway, but longer in sources.

[Tibor17]

dependenciesToScanList already contains plural. Pls rename dependenciesToScanList to dependenciesToScan.

[Tibor17]

No need to create two collections with
List<String> dependenciesToScanList = new ArrayList( Arrays.asList( getDependenciesToScan() ) );
One modifiable collection can be with this code:
List<String> dependenciesToScanList = new ArrayList();
Collections.addAll( dependenciesToScanList, getDependenciesToScan() );

[Tibor17]

dependenciesToScanFileList already contains plural. Pls rename dependenciesToScanFileList to dependenciesToScanFile.

[owenfarrell]

@Tibor17 - done.

[Tibor17]

Would you add integration test?
See the module surefire-integration-tests and src/test/java/org/apache/maven/surefire/its/jiras.
It's easy. Pickup some existing test, e.g. Surefire34SecurityManagerIT, and just inherit SurefireJUnit4IntegrationTestCaseand see the folder surefire-34-securityManager in test/resources.
Your IT will have the same principle. The point is to start surefire-34-securityManager/pom.xml as embedded or forked maven process from Surefire34SecurityManagerIT. I guess you want to test that multi-module Maven project will have own classes propagated from child modules and one dedicated module where Surefire has tests.

[owenfarrell]

I've put together an integration test that can validate the execution of SUREFIRE-1383. And all is good on that front.

While I was poking around the other ITs, I tripped on the IT for SUREFIRE-569. As written, the IT passes just fine. But the changes that I'm proposing make that test case moot - it uses the same packaging structure of 2 projects built within a single session.

I updated that test case to better reflect the original ask of scanning JAR dependencies, but that means splitting the test case execution in to 2 separate lifecycles. When I split the single lifecycle in to 2, I'm running in to a dependency resolution issue. For whatever reason, the second execution is unable to locate artifacts installed in the IT repo as part of the first lifecycle.

In looking around at the other ITs, it looks like this is the only test case that runs an install goal and is then dependent on the result of that install.

Thoughts?

mvn -Dit.test=**/Surefire569RunTestFromDependencyJarsIT.java,**/Surefire1383ScanSessionDependenciesIT.java clean integration-test`
...
[DEBUG] Using local repository at /..../maven-surefire/surefire-integration-tests/../surefire-setup-integration-tests/target/it-repo
...
[ERROR] Failed to execute goal on project surefire-569-module1: Could not resolve dependencies for project org.apache.maven.surefire:surefire-569-module1:jar:1.0: Failed to collect dependencies at org.apache.maven.surefire:surefire-569-testjar:jar:tests:1.0: Failed to read artifact descriptor for org.apache.maven.surefire:surefire-569-testjar:jar:tests:1.0: Failure to find org.apache.maven.surefire:it-parent:pom:1.0 in https://repo.maven.apache.org/maven2 was cached in the local repository, resolution will not be reattempted until the update interval of central has elapsed or updates are forced -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal on project surefire-569-module1: Could not resolve dependencies for project org.apache.maven.surefire:surefire-569-module1:jar:1.0: Failed to collect dependencies at org.apache.maven.surefire:surefire-569-testjar:jar:tests:1.0

[owenfarrell]

Looking at it a bit deeper, looks like the root cause is that the it-parent POM is not installed in the IT repo. The issue isn't the dependency itself.

[owenfarrell]

One workaround for this would be to install the missing it-parent POM as part of my first lifecycle (i.e. mid-test). But this seems kind of hacky.

<project>
    <artifactId>surefire-569-testjar</artifactId>
    ...
    <build>
        <plugins>
            <plugin>
                <groupId>org.apache.maven.plugins</groupId>
                <artifactId>maven-install-plugin</artifactId>
                <executions>
                    <execution>
                        <goals>
                            <goal>install-file</goal>
                        </goals>
                        <phase>install</phase>
                        <configuration>
                            <file>${project.basedir}/../pom.xml</file>
                            <pomFile>${project.basedir}/../pom.xml</pomFile>
                        </configuration>
                    </execution>
                </executions>
            </plugin>
        </plugins>
    </build>
</project>

[Tibor17]

No, no.
No worries.
You should build entire project mvn install and not the individual IT.
The ITs cannot run separately, you can trigger it from building entire project from root in git.

[owenfarrell]

I don't follow. When I run an install from any level, the org.apache.maven.surefire:it-parent:1.0:pom artifact (surefire-integration-tests/src/test/resources/pom.xml) does not get installed in to the IT repo (surefire-setup-integration-tests/target/it-repo). Am I missing something?

In order to execute an IT across multiple lifecycles, that POM needs to be installed it in to the local repository specified by failsafe.

It looks to me like every IT that references it-parent does so via a relative path (explicitly or by default). They never have to rely on the artifact resolver.

[owenfarrell]

Copying the it-parent POM over to the output directory as part of MavenLauncher.simpleExtractResources doesn't support resolving it-parent as part of a dependency.

Would it make sense to open a separate issue/enhancement for installing the it-parent artifact in the IT repo?

[Tibor17]

I have been busy with other ticket. Where did we finish. Do you need a help?

[owenfarrell]

And I got busy with other work as well. I think I found a solution that doesn't completely violate the pattern used by integration tests.

Long story short, the IT for SUREFIRE-569 has been updated to be completely independent modules. The first module leverages the flatten plugin to avoid any parent references, which then allows the second module to complete successfully.

The IT for SUREFIRE-1383 executes within a single process and verifies the updates that I've proposed.

Sorry for all the noise on the ticket - I hosed my fork while making the last round of updates.

[owenfarrell]

@Tibor17 - Have you had a chance to review this PR?

[Tibor17]

@owenfarrell
I have not had yet. I am reviewing other in parallel.
I will have a look in few days.

[owenfarrell]

@Tibor17 - thoughts?

[JoelNGeorge]

I have the exact same issue. This fix is very much appreciated.

[Tibor17]

@JoelNGeorge
@owenfarrell
Pls let me run the build and next steps.

[Tibor17]

@owenfarrell
I built the project. It was ok but then I realized you modified IT 569 which I do not like because this was a feature and I want to guarantee that old tests pass without modification and additionally I have to find out if our contributors mask some of their hidden errors. I or we have to find the root cause why 569 fails with your changes. We should debug your code and then fix it. The changes on 569 you have done are interesting but I prefer to add a new one very similar like "surefire-569-RunTestFromInstalledJar".

[owenfarrell]

@Tibor17 - IT569 does not fail when introducing my changes. But since the test was written as a single lifecycle, it inadvertently uses the code I've introduced in this PR (see comment above).

The only changes to IT569 were to split the single lifecycle in to multiple lifecycles. While there, I tried to clean up the test resources to match current standards/conventions.

But those changes to IT569 were to preserve its named intent: run tests from a dependency jar.

Looks like two options to me:

1. Leave PR as is with minimally modified IT569
2. Revert IT569 to original state, add a new version of IT569 which executes in multiple lifecycles and delete IT1383 (as it would be entirely redundant of IT569).

[Tibor17]

@owenfarrell
I found the root cause.
The dependency to scan is examined with outputDirectory, but IT 569 has sources to share in src/test/java so the artifact would match but that's the reason why target/surefire-reports is not found because your code scans Surefire569RunTestFromDependencyJarsIT_shouldScanAndRunTestsInDependencyJars\testjar\target\classes. I think you have to check if classifier tests exists which results to file name testjar-0.0.1-SNAPSHOT-tests.jar.
Additionally you should exclude packaging pom from the scan.

[Tibor17]

Do you use IntelliJ IDEA? It highlights the argument in yellow.
Use this:
DefaultScanResult scanResult = new DefaultScanResult( Collections.<String>emptyList() );

[Tibor17]

missing Generics on constructor. We have Java 1.6.
List<String> dependenciesToScan = new ArrayList<String>();

[Tibor17]

Pls align to single line after using static import:
import static java.lang.Thread.currentThread; import static org.apache.maven.plugin.surefire.util.DependencyScanner.filter; import static java.util.Collections.singletonMap;

[Tibor17]

Here some artifacts may have classifier tests. This means your loop may appear below test artifacts and only those project artifacts (gid:aid:*) should be excluded which appear in getTestArtifacts() and scans appended creating one aggregated scan result.

[owenfarrell]

Locally staged updates to reflect your generics comments. But I don't follow what you're asking for here.

Per the documentation, the dependenciesToScan property doesn't support classifiers - just group ID and artifact ID. So I didn't add any logic around classifier resolution. Is that what you were expecting?

[Tibor17]

@owenfarrell
It would be easier for you not to revert 569 in this PR but create a new PR from master. I pushed a new fix to master today. The master will be idle for you.

[asfbot]

Tibor Digana on dev@maven.apache.org replies:
This supports classifiers

project.getTestArtifacts()

If you find the one Artifact in this collection then you can simply ignore
it in

session.getSortedProjects()

If it basically opposite what you have done in your code with the removal.
So the old code says to take test dependencies match them with
dependenciesToScan. This is ok and after this we put more those which are
not in dependencies section but appear in depedenciesToScan as project
artifacts.

[asfbot]

Tibor Digana on dev@maven.apache.org replies:
I mean to reorganize the control flow in to this order which should work
for both of us:

List dependenciesToScan =
DependencyScanner.filter( project.getTestArtifacts(),
Arrays.asList( getDependenciesToScan() ) );
DependencyScanner scanner = new DependencyScanner( dependenciesToScan,
getIncludedAndExcludedTests() );
scanner.scan(); // !!! not to return yet !!!

for ( MavenProject mavenProject : session.getSortedProjects() )
{

IF groupId:artifactId IS NOT IN project.getTestArtifacts() THEN =>
additionally scan it and aggregate in to one single result

}

[owenfarrell]

I don't think that logic would work for my scenario.

For example:

1. Create multi-module project with a test runner JAR (a la org.apache.maven.surefire:surefire-1383:1.0-SNAPSHOT)
2. Recursively run the install goal over the multi-module project
3. Modify the test runner module (org.apache.maven.surefire:surefire-1383-runner:1.0-SNAPSHOT)
4. Run the test goal over the multi-module project

According to your logic, Surefire would prefer the already-installed artifact, effectively ignoring those changes made in step 3. That would put us right back where we started.

Using my logic, Surefire would prefer the working copy changes and execute accordingly. Since the existing DependencyScanner doesn't support classifiers as written, I don't think my approach is introducing any new limitations there.

DependencyScanner.java:114

String[] groupArtifact = groups.split( ":" );
if ( groupArtifact.length != 2 )
{
    throw new IllegalArgumentException(...);
}

[Tibor17]

@owenfarrell
I will try by myself. I will let you know with new branch and we can discuss it.

[Tibor17]

@owenfarrell
My tests passed on this code. I have to continue to develop and refactor more.

List<Artifact> testDepends = project.getTestArtifacts();
                removeReactorDependencies( session.getSortedProjects(), testDepends.iterator() );
                List<File> dependenciesToScan = filter( testDepends, asList( getDependenciesToScan() ) );
                TestListResolver testFilter = getIncludedAndExcludedTests();

                DefaultScanResult scanResult =  new DependencyScanner( dependenciesToScan, testFilter ).scan();

                for ( MavenProject project : session.getSortedProjects() )
                {
                    if ( !"pom".equals( project.getPackaging() ) && !containsProjectArtifact( testDepends, project ) )
                    {
                        // zapracuj getDependenciesToScan()
                        File reactorChildProjectOutputDir = new File( project.getBuild().getOutputDirectory() );
                        if ( reactorChildProjectOutputDir.isDirectory() )
                        {
                            DirectoryScanner scanner = new DirectoryScanner( reactorChildProjectOutputDir, testFilter );
                            scanResult = scanResult.append( scanner.scan() );
                        }
                    }
                }

[owenfarrell]

@Tibor17 I see what you're going for, that makes sense.

I've update my PR to just include IT contents now, with an improved IT that emulates a scenario where working copy changes are made (i.e. install a multi-module project, modify the contents to add more tests, test the modified contents).

If your solution gets my IT PR to pass, I think the solution will cover my bona fide scenarios.

[owenfarrell]

@Tibor17 - any updates?

[Tibor17]

@owenfarrell
Yeah, I have released 2.20.1 and I had to postpone this issue for 2.21.1.
OOM was fixed in 2.20.1 and users could not wait longer. This included Java9 support.
Java 9 is out and I want to release Surefire 2.21.0.Jigsaw in next few days which has higher priority.
And then Surefire 2.21.1 is planed for your and other pull request fixed as well.
Then 2.21.2 will fix a blocker issue, and then we will concentrate on 3.0.0.M1 and JUnit5.
So this is my plan.

[paulduffin]

Any progress on this?

[Tibor17]

@paulduffin
I am just making a clone and trying to dig into this problem again after year.

[Tibor17]

@owenfarrell
@paulduffin
I had this issue in several projects and compared them during the weekend.
Today I would write the method scanDependencies() in AbstractSurefireMojo different way because current change looks error prone. I think I can make your extension a bit more straightforward.
Let me try. I want to get this issue fixed in next release.

[Tibor17]

@owenfarrell
@paulduffin
I have finished the implementation in the branch 1383 but I changed the algorithm in AbstractSurefireMojo.

[Tibor17]

@owenfarrell
This feature was merged and will be released in version 2.22.0. You can close the PR.