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.

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

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

Closed
wants to merge 9 commits into from

Conversation

jglick
Copy link
Member

@jglick jglick commented Jun 21, 2017

JENKINS-45047: offers a way to override a plugin’s dependency versions (on Jenkins core and/or other plugins) just before surefire:test is about to run, without editing pom.xml as plugin-compat-tester currently does nor relying on the existence of overridable POM properties.

  • overrideVersions
  • useUpperBounds
  • load versions from megawar
  • patch to PCT
  • patch to buildPlugin.groovy

Copy link
Member

@abayer abayer left a comment

Choose a reason for hiding this comment

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

Looks good so far!

Copy link
Member

@stephenc stephenc left a comment

Choose a reason for hiding this comment

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

Is this still relevant?

@jglick
Copy link
Member Author

jglick commented Nov 16, 2018

Is this still relevant?

Yes I think so; there is still a pressing need for this feature and no good alternative that I know of. Whether I am going to have enough time to work on it is another question.

Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

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

In addition to completing the merge in jglick#1 I had an idea about how to fix one of the TODOs.

@SuppressWarnings("unchecked")
public Map<String, String> upperBounds() {
Map<String, String> r = new HashMap<>();
// TODO this does not suffice; does not find that workflow-api needs to go from 2.11 to 2.16, presumably because it was not a direct dependency to begin with
Copy link
Member

Choose a reason for hiding this comment

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

I think to resolve this we would need to mutate the shadow Project with the results of this method call, then re-do dependency resolution (i.e., do the equivalent of LifecycleDependencyResolver#getDependencies), keeping track of the versions before and after (re-)resolution, then apply the difference to overrideVersions map.

Copy link
Member

Choose a reason for hiding this comment

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

mutate the shadow Project with the results of this method call

To clarify, I mean doing this the same way you would if editing the POM manually: in the case of a direct dependency, by adjusting the version; in the case of a transitive dependency, by adding a new entry to the dependency management section.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. Sounds tricky but could work. (Bringing this PR over the finish line would require some dedicated attention, and in general my workday only allows for a few minutes here and there on Jenkins infra stuff.) Thanks for helping think about it.

@basil
Copy link
Member

basil commented Apr 23, 2022

Done in commit 42b83f5, and with that both of the new ITs are passing. I think we can cross off useUpperBounds from the TODO list.

The next task is "load versions from megawar". I don't really know or understand what a megawar is. It would be cool if you could take a whack at that when you have time.

@basil
Copy link
Member

basil commented Apr 23, 2022

I wrote some hacks in PCT and BOM to test this out against the 100 plugins in the BOM managed set, and it seems to work OK, modulo one issue in pipeline-model-definition. I think if you implement "load versions from megawar" on top of my branch, I could clean up the rest and take it across the finish line.

@basil
Copy link
Member

basil commented Apr 24, 2022

I did a bunch more cleanup and testing today on my branch and have gotten overriding of core working (diff from trunk here). C'mon Jesse, let's get this over the finish line together. 😄

@jglick
Copy link
Member Author

jglick commented Apr 25, 2022

I don't really know or understand what a megawar is.

Sorry for being unclear. Used in the sense of https://github.com/jenkinsci/bom/blob/3e42bee977d5354d81e4ae7dc9edf0053399b8e7/prep.sh#L33-L39 https://github.com/jenkinsci/bom/blob/3e42bee977d5354d81e4ae7dc9edf0053399b8e7/pct.sh#L15 https://github.com/jenkinsci/plugin-compat-tester/blob/7417cfaa8ef6cf006f298586e7fc586caa686189/plugins-compat-tester-cli/src/main/java/org/jenkins/tools/test/CliOptions.java#L60-L62 In other words, rather than forcing dependency versions to be specified one by one as in

<overrideVersions>org.jenkins-ci.plugins.workflow:workflow-step-api:2.11,org.jenkins-ci.plugins.workflow:workflow-api:2.17,org.jenkins-ci.plugins.workflow:workflow-cps:2.32</overrideVersions>
for use from PCT we would like to be able to pass a single *.war argument and ask for versions to be loaded from that rather than https://github.com/jenkinsci/plugin-compat-tester/blob/85e89b3c81462c2020c9abeef78fa8a2f1294f6e/plugins-compat-tester/src/main/java/org/jenkins/tools/test/PluginCompatTester.java#L277-L286

@basil
Copy link
Member

basil commented Apr 25, 2022

Can we work on this in a collaborative fashion? For example, you merge my branch into yours, you give me commit access to it, and then we both push commits until it's done. If you are just going to sit back while I do all the coding, then fine, but then please do not leave questions and suggestions in a code review in the final PR when I am done.

@jglick
Copy link
Member Author

jglick commented Apr 25, 2022

If you are actively working on this then I would suggest you open your own PR (whether from a fork or an origin branch). I can make no time commitments and I would hate to be a bottleneck, especially since you seem to have a clear idea of the goal and how to accomplish it at least for the BOM/PCT usage mode. (The original idea is actually much older and was oriented toward including tests of manually selected downstream plugins in an upstream plugin’s Jenkinsfile. I think this can use the same mechanisms and would just need a bit of glue code in buildPlugin() to be convenient and maintainable, but that could be developed independently.)

As always, you are free to ignore questions and suggestions which do not seem helpful. I do my best to leave comments earlier rather than later but that gets difficult when there are dozens of GH notifications in my inbox. In the unusual case that I think a PR from a regular contributor is actually making things worse (or introduces API commitments we should not), I will “request changes” until convinced otherwise or other experienced contributors think differently.

@basil
Copy link
Member

basil commented Apr 25, 2022

I will file my own PR and keep you credited as the co-author. Can you at least specify the API you had in mind for this (i.e. command-line flag and semantics)? It seems clear that you had a very specific vision for this, so I am trying to stay faithful to that in order to accommodate your original use case.

@jglick
Copy link
Member Author

jglick commented Apr 26, 2022

I started this five years ago 😢 and there is not a specific mojo / property interface I had in mind; whatever seems convenient. There are two basic use cases:

  • To be run from PCT (e.g., bom/pct.sh), in lieu of hacking up pom.xml with dozens of overrides in <dependencyManagement>, which have proven extremely error-prone. My hope was that having access to Maven model APIs here would make it simpler to see what would normally be in the test classpath and then just adjust the versions of those artifacts.
    • Currently PCT also runs two major Maven commands for each plugin: one to compile it against its original baseline; then one to run surefire:test against the updated dependencies using the bytecode created in the first step. (Rarely matters, but more faithfully reproduces the binary compatibility scenario seen in actual usage.) We ought to be able to use this new mojo to adjust the Surefire booter test classpath not only without physically editing pom.xml but also without affecting the behavior of maven-compiler-plugin and the like, meaning that a single mvn test would suffice.
  • To be run from CI of an upstream (often API-offering) plugin to verify that proposed changes do not regress important use cases in a downstream plugin. For example, workflow-cps-plugin/Jenkinsfile might say something like
buildPlugin(downstream: ['workflow-cps-global-lib'])

which would do something conceptually like

mvn -f workflow-cps-plugin install
# also on Windows in parallel, etc., like the main part of buildPlugin() today
version=$(mvn -f workflow-cps-plugin -Dset.changelist -Dexpression=project.version -q -DforceStdout help:evaluate)
for downstream in workflow-cps-global-lib-plugin
do
  mvn -f $downstream -Doverrides=workflow-cps=$version -Dmaven-hpi-plugin.disabledTestInjection test
done

ideally using junit to collect downstream plugin test results.

@jglick
Copy link
Member Author

jglick commented Apr 28, 2022

For the buildPlugin use case, I am not sure if I ever thought through how the version of the downstream plugin would be specified. (You would not want to just check out master since then the upstream repo’s CI would be nondeterministic.) Sometimes it is actually a test-scoped dependency of the upstream plugin (this is not considered a cycle), in which case you could infer the version from the Maven test classpath (perhaps managed by bom). It is hard to predict how well this would work in practice, when various plugins are adjusting their core baselines, making API changes, fixing bugs that were found to cause incompatibilities in other plugins, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants