[JENKINS-45047] Support for plugin-to-plugin integration tests

[basil]

Finishes the great work started by @jglick in #66. I have left extensive comments in the implementation, so I will not write a long description here. This changes goes along with jenkinsci/plugin-compat-tester#360 and jenkinsci/bom#1059, which are getting close to ready but not quite. This change has been tested enough by now that I think it is ready for review. I have tested this PR in conjunction with the other two and tests have passed successfully on 100 out of 101 plugins in the managed set in bom-weekly (there is still a problem with checks-api, but I think it is a technical problem with the plugin itself and not with this code). In the coming days I will try this on bom-2.289.x but I fear some of the plugins in that line may be too old to support a newer toolchain.

Closes #66.

[jglick]

Very nice! Thank you for finally moving this forward. Seems safe to merge at any time and iterate, since it should have no effect when the new properties are not being used.

I have tested this PR in conjunction with the other two and tests have passed successfully on…

To what extent can we verify that in the bom run the test classpaths are actually what we expect without the POM rewrites? I suppose it would be compelling to find a recent example of a plugin update which is known to introduce a downstream regression?

[jglick]

If I follow correctly, if the pom.xml specifies a dep on io.jenkins.plugins:some-lib-wrapper:1.1 which had deps on some-lib.jar and some-helper.jar, and you request an override to some-lib-wrapper:1.2 in which some-lib.jar is updated but some-helper.jar is no longer included (the new version of some-lib switched frameworks or whatever), the revised project model will no longer have a (transitive) dep on some-helper and so this will be excluded from the Surefire classpath, right? I guess this is OK under the assumption that the plugin under test never referred directly to some-helper (in main or test sources) without explicitly declaring that dep; and if there were any other dependency trail leading to some-helper then it would not have been removed anyway.

[basil]

Yes, exactly. You can see this in action by finding a plugin with an older core baseline (e.g., text-finder) and testing with this PR and a megawar of a very new core. The deletions list is a nice summary of all the old JARs I have been removing from core over the past year.

[jglick]

Oh this applies to the jenkins-core dep too. Interesting, though if you are required to pass -Djenkins.version=… anyway, would that not already be making that change to the original project model?

[basil]

Oh this applies to the jenkins-core dep too. Interesting, though if you are required to pass -Djenkins.version=… anyway, would that not already be making that change to the original project model?

It would... now that I think about it, I went through that particular debugging exercise before I had added the requirement for -Djenkins.version, so I guess you wouldn't be able to replicate it now. 😄

[jglick]

This is a nice example of the sort of thing that is really hard to get right in the POM-rewriting part of PCT.

[basil]

Yeah, that and the application of upper bounds analysis. While it might be technically feasible to munge the XML file from the POM-rewriting part of PCT with the right algorithm, it isn't possible to determine upper bounds without actual dependency resolution.

[basil]

Thanks for the review Jesse.

To what extent can we verify that in the bom run the test classpaths are actually what we expect without the POM rewrites?

Not easily. But essentially the algorithm in this PR grew out of what I did manually dozens of times during the Guava upgrade project, and while developing debugging this code I spent quite a bit of time comparing, side-by-side, the results of automation with the results of manual POM editing for a number of plugins, so I am feeling confident that the manipulation is working as expected overall. The only thing I have not attempted to deal with at all is split plugins, but neither does jenkinsci/bom, so that can be done in some future enhancement if needed.

[basil]

I should also add that in this PR I'm printing the final dependency tree by invoking the tree goal from the dependency plugin against the synthetic dependency tree, which I found essential while debugging. So the process for debugging is pretty straightforward: apply the expected changes to a POM manually, run mvn dependency:tree, and compare the results to the automation. They should be identical, and if they are not the difference should point to where the bug is.

[basil]

(This tree debugging logic was written in order for me to be able to debug the managed/provided core BOM issue I had, which is what provoked the jenkins.version logic and related comment.)

[jglick]

verify that in the bom run the test classpaths are actually what we expect

I realize after I wrote this that I was actually asking something much more modest than the phrasing implied: a sanity check that a sampling of a few representative cases does show tests being run against some plugin versions taken from the BOM under test rather than the checked-in POM. For example, if there is a confirmed reproduction case for jenkinsci/bom#821, that using this new mode indeed appears to fix it; or test output somewhere in jenkinsci/bom#1059 that offers a clear example of a test running using a newer plugin dep version than what pct-work/*/pom.xml requests.

[basil]

looks good.

And yet you have not approved the PR.

I do not follow the issue with the BOM though - it seems like we can add a dependencyManagement for the jenkins bom to the project in order to resolve the correct core libraries without having to pass jenkins.version and then we can have a single step "PCT" IIUC. e

Thank you for the code review, James. In addition to submitting code reviews, I also encourage you to submit pull requests to jenkinsci/maven-hpi-plugin, jenkinsci/plugin-compat-tester, and jenkinsci/bom.

[jtnord]

I do not follow the issue with the BOM though - it seems like we can add a dependencyManagement for the jenkins bom to the project in order to resolve the correct core libraries without having to pass jenkins.version and then we can have a single step "PCT" IIUC. e

Thank you for the code review, James. In addition to submitting code reviews, I also encourage you to submit pull requests to jenkinsci/maven-hpi-plugin, jenkinsci/plugin-compat-tester, and jenkinsci/bom.

my point was I did not follow the conversation / code and due to that it looks wrong to me, and I did not make this clear as it has not been answered.

So to try again.

It looks like the expectation to have to pass in jenkins.version is wrong - we can have the jenkins.verion from the war in the case it is provided, and then can use that to update the jenkins-bom in the depnednecy management. So there is no need to change the jenkins.version and do another phase, as we are manipulating the project model and re-forcing dependency resolution - so why is the bom not like anything else I fundamentally do not understand and neither the review comments nor the code comment help me to understand this (and if I do not understand it in the context of this PR, I am not sure others would in the future either).

And if this expectation is indeed incorrect and we did update the jenkins bom just like everything else then PCT can be nuked from orbit. We should not even need the PCT for plugins using an older parent as again to my knowledge the hpi-plugin has a stable mojo API.

So to run a "PCT" for a plugin all you need to do now is assuming a new enough parent pom.

createUberWarWithAllPLuginsIWantToTestAgainst.sh
git clone the-plugin.git
cd the-plugin
mvn -DsoverrideWar=fat.war test

and if the parent pom is not new enough we can do the following actually specify the plugin version on the CLI as it is (in all version 4 of the plugin-pom a property)

createUberWarWithAllPLuginsIWantToTestAgainst.sh
git clone the-plugin.git
cd the-plugin
mvn -DsoverrideWar=fat.war -Dhpi-plugin.version=3.28 test

And then to ignore parsing the POMs to find out what version we have - we can extract the version (or manually update it whenever we release a new version which is not so often) and

createUberWarWithAllPLuginsIWantToTestAgainst.sh
git clone the-plugin.git
cd the-plugin
mvn v
mvn -DsoverrideWar=fat.war -Dhpi-plugin.version=3.28 test

But doing the above is dependant on the code in here which I left a comment about which seems to be wrong.

If I am indeed correct - I would be happy to nuke the PCT from orbit and update the BOM build, but perhaps I am missing some tribal knowledge of this area that is not explained in this code or accompanying comments.

And yet you have not approved the PR.

because I left an inlinecode review comment where I felt the code was not correct, but as I was unsure I was not going to block the merging.

[basil]

It is often far easier to fix complex problems than to explain the nuances of those fixes to those who have not engaged with the code in question through code contributions. It is also often far easier to write comments criticizing solutions to complex problems than to fix those complex problems.

Demonstrating a willingness to engage with the code in question by making code contributions helps make the nuances of the problem space more clear. It also helps to ensure that one's criticism of others is reified with the context of having done implementation work with the code in question.

I would love to see you demonstrate this willingness by making code contributions to jenkinsci/maven-hpi-plugin, jenkinsci/plugin-compat-tester, and jenkinsci/bom. I believe this would help you become a better reviewer of the code in those repositories.

I felt the code was not correct, but as I was unsure

I would encourage you not to leave comments when you are unsure. It is difficult to prove a negative. The burden of proof is on the reviewer to demonstrate that the submission is incorrect, not on the author to demonstrate that the submission is correct.

If you are unsure about something, the best way to become sure is to start making code contributions to a given repository.

Otherwise, as one of my formative mentors once told me years ago, "to the implementer goes the decision."

[jglick]

the expectation to have to pass in jenkins.version is wrong - we can [infer that from overrideWar], and then can use that to update the jenkins-bom in [dependencyManagement]

Possibly so, assuming that the somewhat scary tricks here used to recreate a POM model in memory will honor that update. That would save you from the trouble of passing -Djenkins.version, and would allow you to test a plugin against a megawar in a single phase rather than the two phases we would otherwise need to prove that we are enforcing binary and not source compatibility. Discussed earlier in #336 (comment). So

mvn test-compile
mvn -Djenkins.version=… -DoverrideWar=… hpi:resolve-test-dependencies surefire:test

could be simplified to

mvn -DoverrideWar=… test

Seems like it would be a welcome improvement, but I do not think lack of that should block us from moving forward—someone can add it as time permits, unless I am missing something?

to my knowledge the hpi-plugin has a stable mojo API

For the most part, though we do sometimes introduce breaking changes, like #302. Less of an issue if we are anyway doing a two-phase build, since then the plugin version only matters for this one mojo:

mvn test-compile
mvn -Dhpi.version=… -Djenkins.version=… -DoverrideWar=… hpi:resolve-test-dependencies surefire:test

then PCT can be nuked from orbit

I think we can continue to use PCT to arrange for checkouts of plugins in the megawar, running Maven, collecting results, etc. The hope is that we can suppress the most complex and buggy portions that deal with POM rewrites. (Also making it less stressful to analyze PCT failures locally. Currently you have to actually run PCT and then open the patched repo clone in an IDE to study test failures—and if you come up with local patches to solve the failure, figure out how to apply those to a clean checkout.)

The handling of multimodule repositories is also unnecessarily complex in PCT. Would be better to aggregate all plugins coming from one repository:

cd pipeline-model-definition-plugin
mvn -Pquick-build install
mvn -Djenkins.version=… -DoverrideWar=… hpi:resolve-test-dependencies surefire:test

which would again be a bit smoother with automatic jenkins-bom handling:

cd pipeline-model-definition-plugin
mvn -DoverrideWar=… install

[basil]

As of PR build #36 I have the first passing jenkinsci/bom run with useUpperBounds, including a passing InjectedTest on all plugins.

[jetersen]

hopefully jenkinsci/bom#1275 have given a good use case for this? 😅

[basil]

hopefully jenkinsci/bom#1275 have given a good use case for this? sweat_smile

Sure, but I see little hope of ever completing this project so long as plugin-compat-tester maintainers continue to be unresponsive and do not merge my PRs. Help would certainly be appreciated if you are interested.

[jetersen]

included in plugin pom https://github.com/jenkinsci/plugin-pom/releases/tag/plugin-4.44 👏