Do not set the JTH War file

[jtnord]

as mentioned in the upstream PR that attempted to do this unconditionally this is broken by design.

The JTH and tests assume that the classpath is correctly setup however it is not, as demonstrated by d9dba5a

According to Jenkins there is a plugin installed (trilead-api) because it is in the megawar, however its classess are not in the flat classpath used by JenkinsRule and thus breakage ensues.

There are also issues where a plugin is present that is incompatible with a mode of running for the test - e.g. a system property is set when running JTH to test a mode of running for a plugin (e.g. FIPS) and the plugin itself is incompatable with that mode (which causes a BootFailure).

ref: jenkinsci/plugin-compat-tester#470 (comment)
fixes: #3231

Testing done

This commit has been run in a full build where failures occured with eddsa-api / trilead-api and mina-api plugins and a weekly build of just the eddsa-api plugin.
In both cases all plugins passed after introducing this change.

Submitter checklist

• Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
• Ensure that the pull request title represents the desired changelog entry
• Please describe what you did
• Link to relevant issues in GitHub or Jira
• Link to relevant pull requests, esp. upstream and downstream changes
• Ensure you have provided tests - that demonstrates feature works or fixes the issue

[jtnord]

NB have not set the weekly or full-build as this has been included in builds that have had this, and I did not want to load the infra and burn through MSFT credits. If someone wants to set these options to get full coverage be my guest :) )

[jglick]

We would lose some test coverage, but avoid a number of aggravating issues going forward. (@cloudbees runs PCT and does not use this mode.) Other maintainers may feel differently.

There may be technical debt in plugin test cases solely to support this mode, which it would be nice to delete; I am not sure if there is an efficient way to search for that however.

[MarkEWaite]

I set the full-test label and will perform another run because I want to see the results in this context.

[alecharp]

I don't see any problem to merge this as all the tests passed successfully.
Anyone disagreeing?

[MarkEWaite]

I thought that we needed to use the fat WAR file in order to actually run the tests of each plugin inside a Jenkins controller that has all the plugins from the bom loaded. However, the comment from James reminds me that a JenkinsRule test does not have the same classpath and runtime environment as a RealJenkinsRule test. James said:

According to Jenkins there is a plugin installed (trilead-api) because it is in the megawar, however its classess are not in the flat classpath used by JenkinsRule and thus breakage ensues.

When running a JenkinsRule based test in this pull request, will the tests still detect the failures in plugin tests that are due to global changes from other plugins?

[jtnord]

When running a JenkinsRule based test in this pull request, will the tests still detect the failures in plugin tests that are due to global changes from other plugins?

No it won't unless those plugins are in the plugin under test test classpath. (Ie test/compile scope dependencies)

And this can be considered a good thing as well as a downside as some plugins (eddsa) will cause failures in other plugin tests (when they specifically test FIPS behaviour e.g. email-ext) meaning they need to be disabled reducing coverage , and you need to add;workarounds (like adding trilead-api dependency to mina-ssh amongst others). (Which was caused by a false positive!)

[jglick]

The FIPS-specific problems are historically unusual and could probably be worked around at some expense.

The point of the flag was to catch cases such as the following example: plugin A defines a Pipeline step alpha and AlphaStepTest.smokes runs a build using this step. Now plugin B defines a Pipeline step listener (an @Extension invoked whenever any step is run) that inspects some step metadata looking for some condition, but has a subtle bug causing it to throw an ArrayIndexOutOfBoundsException (that breaks the build) when a certain sort of unusual step syntax is used, which alpha happens to trigger. Assuming neither A nor B depends on the other (developed independently), PCT by default will not catch this condition. With this flag on, AlphaStepTest will run with B loaded, and thus fail, highlighting the problem as soon as the PR to update B with this new listener is filed. (The Jenkins update center does not check bom, so Jenkins users may have already installed the faulty update and be filing bug reports.)

Does this actually catch useful bugs in practice? Not often if ever that I can recall. Most regressions are from changes to code which would be in the default test classpath; or involve problems that would not have been caught by existing automated test coverage.

[jglick]

(FTR apparently introduced in #20 (comment) jenkinsci/jenkins-test-harness#100)

[MarkEWaite]

Thanks @jtnord for the clarification on execution and thanks @jglick for the clarification that the concern on test coverage is not related to a high priority or high risk area of coverage. I think this is ready to merge.

Do we need an immediate release or is it good enough to wait until the end of the week for the regular release?

[jglick]

Do we need an immediate release

I do not think this change requires release at all. It was blocking integration of other PRs into bom; it has no effect on bom artifacts consumed by plugins.

[jtnord]

Thanks @MarkEWaite / @jglick