Plugin compatibility tester is re-adding test-scoped dependencies rather than merely updating them

[basil]

Problem

Running PCT on pipeline-utility-steps with an updated workflow-step-api, I was getting the following error:

[WARNING] Some problems were encountered while building the effective model for org.jenkins-ci.plugins:pipeline-utility-steps:hpi:2.3.1
[WARNING] 'dependencies.dependency.(groupId:artifactId:type:classifier)' must be unique: org.jenkins-ci.plugins.workflow:workflow-step-api:jar -> duplicate declaration of version 2.20 @ line 271, column 17
[WARNING] 
[WARNING] It is highly recommended to fix these problems because they threaten the stability of your build.
[WARNING] 
[WARNING] For this reason, future Maven versions might no longer support building such malformed projects.

… followed by a series of test failures because workflow-step-api was missing when running the tests.

Evaluation

The pom for pipeline-utility-steps looks like this:

<properties>
    [...]
    <workflow-step-api.version>2.19</workflow-step-api.version>
</properties>
[...]
<dependencies>
    <dependency>
        <groupId>org.jenkins-ci.plugins.workflow</groupId>
        <artifactId>workflow-step-api</artifactId>
        <version>${workflow-step-api.version}</version>
    </dependency>
    [...]
    <dependency>
        <groupId>org.jenkins-ci.plugins.workflow</groupId>
        <artifactId>workflow-step-api</artifactId>
        <classifier>tests</classifier>
        <version>${workflow-step-api.version}</version>
        <scope>test</scope>
    </dependency>
    [...]
</dependencies>

So PCT was trying to update this dependency from 2.19 to 2.21. Unfortunately, here's the edit PCT came up with:

<?xml version="1.0" encoding="UTF-8"?>
<properties>
    [...]
    <workflow-step-api.version>2.19</workflow-step-api.version>
</properties>
<dependencies>
    <dependency>
        <groupId>org.jenkins-ci.plugins.workflow</groupId>
        <artifactId>workflow-step-api</artifactId>
        <exclusions>
            <exclusion>
                <groupId>org.jenkins-ci</groupId>
                <artifactId>SECURITY-144-compat</artifactId>
            </exclusion>
        </exclusions>
        <version>2.20</version>
    </dependency>
    [...]
    <dependency>
        <groupId>org.jenkins-ci.plugins.workflow</groupId>
        <artifactId>workflow-step-api</artifactId>
        <classifier>tests</classifier>
        <scope>test</scope>
        <exclusions>
            <exclusion>
                <groupId>org.jenkins-ci</groupId>
                <artifactId>SECURITY-144-compat</artifactId>
            </exclusion>
        </exclusions>
        <version>2.20</version>
    </dependency>
    [...]
    <!--SYNTHETIC-->
    [...]
    <dependency>
        <groupId>org.jenkins-ci.plugins.workflow</groupId>
        <artifactId>workflow-step-api</artifactId>
        <version>2.20</version>
        <scope>test</scope>
        <exclusions>
            <exclusion>
                <groupId>org.jenkins-ci</groupId>
                <artifactId>SECURITY-144-compat</artifactId>
            </exclusion>
        </exclusions>
    </dependency>
    [...]
</dependencies>

Unfortunately, this edit is just plain wrong. It edited the first two dependencies correctly, but it also inserted a new test-scoped dependency at the end, which was missing the classifier that the original (and still present!) test-scoped dependency had. The result is a malformed POM that can't build.

Solution

Fix the bug. Test-scoped dependencies should be edited in place rather than added at the end as synthetic dependencies.

Implementation

Looking at the code, I saw it was maintaining a toReplaceTestUsed map to determine what was replaced and what needs to be added as "synthetic". But the only time a test-scoped dependency was placed into that map was if that test-scope dependency wasn't already present as a non-test-scoped dependency. This isn't true in the case of pipeline-utility-steps and we cannot assume it is true in the general case. The solution is simple: after doing the replacement, check if the thing we replaced was test-scoped or not. If it was test-scoped, update the toReplaceTestUsed map so that the dependency isn't re-added as "synthetic" later on in the code.

Testing Done

Confirmed that the problematic duplicate dependency was inserted into the POM before this change and was not inserted into the POM after this change, and that the "Some problems were encountered while building the effective model" Maven error was no longer present.

[basil]

Yet another flake in CaSC tests, as in #181 (comment).

[2019-09-20T18:16:07.621Z] Exception in thread "main" java.io.IOException: Unable to open file /home/jenkins/workspace/ries_plugin-compat-tester_PR-192/out/pct-report.xsl for writing.
[2019-09-20T18:16:07.621Z] 	at org.codehaus.plexus.util.FileUtils.checkCanWrite(FileUtils.java:1240)
[2019-09-20T18:16:07.621Z] 	at org.codehaus.plexus.util.FileUtils.copyStreamToFile(FileUtils.java:1212)
[2019-09-20T18:16:07.621Z] 	at org.jenkins.tools.test.PluginCompatTester.testPlugins(PluginCompatTester.java:161)
[2019-09-20T18:16:07.621Z] 	at org.jenkins.tools.test.PluginCompatTesterCli.main(PluginCompatTesterCli.java:163)

[oleg-nenashev]

It would be great to have a unit test for this use-case, but looks like it is doing the right thing.
Would appreciate review from @raul-arabaolaza before merging

[oleg-nenashev]

I suspect it happens for BOM, so requesting review from @jglick

[jglick]

BTW SECURITY-144-compat is long obsolete, I think. Was used temporarily for maven-plugin after that security fix, but only affects really old versions.

[jglick]

Sounds right.