resolve-test-dependencies should not add deps unrelated to the model by default

[jglick]

Trying to figure out what #336 (comment) really meant.

(overrideWar does not appear to have any IT coverage, so this is just a placeholder to be tested in the context of bom.)

[basil]

resolve-test-dependencies should not add dependencies unrelated to the model

🤷‍♂️ I did this to test how plugins interact with each other. Some plugins have global effects (e.g., enabling timestamps for all Pipeline builds in Timestamper, a global option to add build user variables to the environment for all builds in Build User Vars, changing the scheduler in Least Load, etc). While in most cases these global changes are opt-in (e.g., the Timestamper and Build User Vars examples) in some cases they are not (e.g., the Least Load example). I deemed it useful to test the interaction between plugins when an unrelated plugin happens to be installed.

[jglick]

I see. That is something we opt into for bom builds via jth.jenkins-war.path, so this is effectively enforcing version bounds on those injected dependencies instead of / in addition to loading them from the megawar. Maybe then the test addition behavior should be gated by the presence of this variable, or made an explicit option of resolve-test-dependencies you would need to select.

[basil]

Should this not be done unconditionally? Why would anyone want to set overrideWarAdditions to true if the stated objective (testing how plugins interact with each other) is accomplished by setting jth.jenkins-war.path?

[basil]

Maybe then the test addition behavior should be gated by the presence of this variable, or made an explicit option of resolve-test-dependencies you would need to select.

If the BOM use case is covered by jth.jenkins-war.path then I see no need for this functionality at all. I doubt an interactive user running with overrideVersions would want this functionality, so I would be in favor of ripping it out.

[basil]

If there are any benefits to setting both overrideWarAdditions to true and jth.jenkins-war.path to a megawar, I am not seeing them.

[basil]

I filed #375 and #376 with some alternatives. My main objection to this PR is that it is overly conservative, leaving in some implementation logic that isn't used anywhere but in jenkinsci/bom, and is unnecessary at that (because jenkinsci/bom uses jth.jenkins-war.path, which totally satisfies the use case I had in mind for this logic, testing the interaction between "long-tail" plugins unrelated to the core build).

#375 is a bit riskier but has the simplest possible semantics. I think #376 finds a sweet spot between low risk and simplification.

[jglick]

any benefits to setting both overrideWarAdditions to true and jth.jenkins-war.path to a megawar

I do not know if there are any such benefits; you would be better placed to answer this question (if I understand the history correctly) since you added that clause in #336 despite testing only in the context of jenkinsci/bom which already did set jth.jenkins-war.path. Were there some PCT runs which failed, or passed when you would expect them to fail, etc., before you included the clause now gated by overrideWarAdditions? If not, then we can simply delete this clause and not even have an option for it, since bom will be running plugin tests with a megawar anyway, which has historically caused it to point out tests which presumed that the declared test classpath was small. (I recall some examples in branch-api that fail when you add some other plugins to the test classpath because there were assertions expecting only a certain set of extension point implementations to be present.)

Why would anyone want to set overrideWarAdditions to true if the stated objective (testing how plugins interact with each other) is accomplished by setting jth.jenkins-war.path?

In the CloudBees case, because our interest is in checking whether newer versions of core and plugins already in the declared test classpath turn up regressions. Additionally checking whether the presence of extra plugins in a test classpath will cause test failures might be interesting, but is far less of a priority: most such failures will be artificial (an assertion that needs to be relaxed, as above), with a few cases highlighting an actual mistake in production code.

At any rate, we cannot pass jth.jenkins-war.path (except I think in RealJenkinsRule tests) because it causes a massive number of test failures due to technical aspects of how the CloudBees WARs are built and in particular the Beekeeper system; @imonteroperez tried it and gave up. Possibly we could get around this by creating an artificial megawar based on jenkins.war plus a set of bundled plugins, but this would just be trying to work around the behavior of this mojo; passing jth.jenkins-war.path to surefire:test and providing a WAR as a convenient sort of manifest enumerating plugin versions are independent concerns that are best controlled explicitly with separate options.

[basil]

If not, then we can simply delete this clause and not even have an option for it, since bom will be running plugin tests with a megawar anyway, which has historically caused it to point out tests which presumed that the declared test classpath was small.

Yeah I think I was just being overly ambitious and I would rather delete this clause and not even have an option for it for simplicity. I can test out a simple deletion if that would save you some time.

At any rate, we cannot pass jth.jenkins-war.path (except I think in RealJenkinsRule tests) because it causes a massive number of test failures due to technical aspects of how the CloudBees WARs are built

Ah that's too bad. Well in that case my proposed simplification PRs are not feasible then.

[jglick]

I can test out a simple deletion

Sure, that would be helpful.

[basil]

Done in this run. Even setting aside the PluginAutomaticTestBuilder$OtherTests.testPluginActive failures as "not interesting" (I think they are interesting), we still have things like this where the system classloader (i.e. Surefire) can't see "long tail" plugins (i.e., those not directly related to the plugin under test) but some other class loader (e.g. https://github.com/jenkinsci/job-dsl-plugin/blob/94d5aa82764ef808ab36c5e8ee0dd5404ef65844/job-dsl-plugin/src/main/groovy/javaposse/jobdsl/plugin/structs/DescribableHelper.groovy#L177) can.

So hm… using jth.jenkins-war.path makes "long tail" plugins available to UberClassLoader but not to Surefire's class loader (i.e., non-RealJenkinsRule tests) which effectively precludes JenkinsConfiguredWithCodeRule from working in the presence of https://github.com/jenkinsci/job-dsl-plugin/blob/94d5aa82764ef808ab36c5e8ee0dd5404ef65844/job-dsl-plugin/src/main/groovy/javaposse/jobdsl/plugin/structs/DescribableHelper.groovy#L177. My solution of adding all "long tail" plugins to the model in test scope solves this problem for non-RealJenkinsRule tests, at the cost of being an unrealistic "flat" class path. Although one could argue that is a general (unfixable) design flaw in non-RealJenkinsRule tests, and my solution simply takes this existing design (warts and all) to its logical conclusion (and that one should use RealJenkinsRule for a realistic class path). Easier said than done, especially for the many plugins that use JenkinsConfiguredWithCodeRule: I am not really ready to go converting all of those to RealJenkinsRule at this time. 😫

So I am left with the conclusion that adding these "long tail" plugins to the model in test scope is at least somewhat desirable, in a narrow sense. So I think I definitely want to keep this code around in the short term. The question for me is why you want to disable it: you started this conversation with a request for me to justify the inclusion of the code, which I believe I have now done, so now I make the same request in return: what would be your reason for wanting to disable it? Is it that putting everything all plugins into a flat classpath for non-RealJenkinsRule tests (which have the unfixably flawed design of a flat classpath) is just too much effort? If so I could agree to this change, since I made a similar argument about not wanting to put in more effort in the preceding paragraph. 😄

[jglick]

effectively precludes JenkinsConfiguredWithCodeRule from working

Whatever https://github.com/jenkinsci/job-dsl-plugin/blob/94d5aa82764ef808ab36c5e8ee0dd5404ef65844/job-dsl-plugin/build.gradle#L119 does, it seems to not work in https://github.com/jenkinsci/job-dsl-plugin/blob/94d5aa82764ef808ab36c5e8ee0dd5404ef65844/job-dsl-plugin/src/main/groovy/javaposse/jobdsl/plugin/JenkinsDslScriptLoader.java#L24-L25 in a JenkinsRule context? In general it should be fine for the Surefire class loader used for JenkinsRule tests to not be able to see classes visible to UberClassLoader. I suspect the issue in this case is that job-dsl is in the test classpath but config-file-provider is not, and so the optional dep is actually broken. If so, the solution is simply to add config-file-provider to the test classpath for matrix-auth and text-finder.

I definitely want to keep this code around in the short term

Fine with me, so long as it is optional, as in this PR.

what would be your reason for wanting to disable it?

Lots of tests fail with this clause applied to a CloudBees WAR file. (And fail slowly—the test run is much longer than it should be.) A bunch of stuff is loaded which was never intended to be used in a JenkinsRule context (BeekeeperUpdateSourceProvider for example) and will not work.

[basil]

Whatever https://github.com/jenkinsci/job-dsl-plugin/blob/94d5aa82764ef808ab36c5e8ee0dd5404ef65844/job-dsl-plugin/build.gradle#L119 does, it seems to not work in https://github.com/jenkinsci/job-dsl-plugin/blob/94d5aa82764ef808ab36c5e8ee0dd5404ef65844/job-dsl-plugin/src/main/groovy/javaposse/jobdsl/plugin/JenkinsDslScriptLoader.java#L24-L25 in a JenkinsRule context?

That is what I suspect. Have not looked into it closely yet.

In general it should be fine for the Surefire class loader used for JenkinsRule tests to not be able to see classes visible to UberClassLoader.

I guess? I could see arguments both ways here. On the one hand, an argument could be made that my approach of adding things to the model takes the "flat" classpath design of JenkinsRule (as flawed as it is) to its logical conclusion. On the other hand, an argument could be made that limiting the "flat" classpath to only direct and transitive dependencies (as opposed to "long tail" plugins) more closely resembles the actual production scenario of DependencyClassLoader. Actually, now that I typed that out, I am starting to come around to your way of thinking and am preferring the latter argument over the former.

I suspect the issue in this case is that job-dsl is in the test classpath but config-file-provider is not, and so the optional dep is actually broken. If so, the solution is simply to add config-file-provider to the test classpath for matrix-auth and text-finder.

Thanks, I will try that out and keep trying to remove this logic in jenkinsci/bom. In the meantime I see no reason to stand in your way.

Lots of tests fail with this clause applied to a CloudBees WAR file. (And fail slowly—the test run is much longer than it should be.) A bunch of stuff is loaded which was never intended to be used in a JenkinsRule context (BeekeeperUpdateSourceProvider for example) and will not work.

Thanks for the context but I am still not clear on the reason why these CloudBees tests are failing. But no matter: not including the "long tail" in the Surefire flat classpath is closer to the way DependencyClassLoader does things, so I am inclined to agree that we should get rid of this logic in the long term and certainly allow for disabling it in the short term.

[basil]

I will release this momentarily.

[jglick]

takes the "flat" classpath design of JenkinsRule […] to its logical conclusion

In some sense, yes. I am not sure we care.

[basil]

Got this scary warning when releasing:

[INFO] [WARNING] You are about to deploy a maven-plugin using Maven 3.8.6.
[INFO] [WARNING] This plugin should be used ONLY with Maven 3.9.0 and newer, as MNG-7055
[INFO] [WARNING] is fixed in those versions of Maven only!

I guess if we have any problems, we'll have to roll back Maven Deploy Plugin to 2.x?

[jglick]

Perhaps. https://issues.apache.org/jira/browse/MNG-7055 does not sound comforting.

[basil]

I do not quite understand the antecedent of "this" in "This plugin should be used ONLY with Maven 3.9.0 and newer": is it referring to the plugin being deployed (i.e., maven-hpi-plugin) or the plugin doing the deploying (i.e., maven-deploy-plugin)? jenkinsci/plugin-pom#597 (and local testing) seem to indicate the latter, in spite of me using Maven 3.8.6 to run maven-deploy-plugin, so I have reached a contradiction.

[jglick]

I presumed it referred to the plugin doing the deploying. I think when deploy works on something with maven-plugin packaging there is some special extra metadata that is supposed to get modified. Smells like jenkinsci/pom#283 should be reverted at least pending further investigation. jenkinsci/plugin-pom#578 presumably does not matter since this parent would not be used with maven-plugin packaging.

[basil]

If it ain't broke? On the other hand, if something does break, we'll have a better understanding of what this is all about…