Upgrade tests to JUnit 5

[strangelookingnerd]

Upgraded the smoke test to use JUnit5JenkinsRule.

Testing done

Ran mvn clean verify

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]

test ends up not testing what it actually should be testing

[jtnord]

this is not the same thing as RealJenkinsRule, and the test is using this for good reason (JenkinsRule which is the older equivalent of WithJenkins uses a non realistic classpath)

Details are explained in the Javadoc for RealJenkinsRule

[strangelookingnerd]

Well in that case upgrading to JUnit5JenkinsRule or JUnit5 won't be possible I guess.

I was unaware that this is required in order to properly test this plugin, since other library plugins also use JUnit 5. Would you care to elaborate why this is important so I can understand it better?

[jtnord]

Well in that case upgrading to JUnit5JenkinsRule or JUnit5 won't be possible I guess.

at least until such time that an equivalent is introduced in jenkins-test-harness.

I was unaware that this is required in order to properly test this plugin, since other library plugins also use JUnit 5.

I would say that at least several of those smoke tests are close to useless (see below) :-)

Would you care to elaborate why this is important so I can understand it better?

when running with JenkinsRule. JenkinsSessionRule or WithJenkins the resulting Jenkins instance is run in the same JVM as the test. The test has its classpath set by maven-surefire, and as such is a big long flat list of jars.

In this scenario every class has the same classloader§, and as such all classess can see and load any other classes.

When you run Jenkins this is not the case at all as Jenkins has a different classloader for each plugin (and Jenkins itself) (more details here).

For a lot of tests this may not matter and there are some extra things in place to try and help developers stay inside the bubble where this should work (e.g. the requireUpperBounds enforcer rule) .

However, if a plugin is performing class loading, or is using special classloading then this becomes important, as the classloader they would inside Jenkins would be vastly different to the one when run by surefire.
e.g. Anything using reflection (Class.forName) to load classes.
in this case the Class.forName(...) can work in the flat hierachy but be completely broken when run in Jenkins.

There can be other subtleties where the difference shows,

for example as noted by the original author of the test (#82)

I included a new integration test with RealJenkinsRule that works against Jenkins 2.297 and earlier, as well as tip-of-trunk with jenkinsci/jenkins#5604. Without jenkinsci/jenkins#5604, the test fails on tip-of-trunk with the same error as in jenkinsci/azure-storage-plugin#194. So somehow we end up relying on the old version of Woodstox (3.x) from Jenkins core, even though jackson-dataformat-xml depends on a recent version of Woodstox as a compile-time dependency. I am not sure why. The test passes with regular JenkinsRule, so this must be a classpath issue with jenkins.war. This would be a useful starting point for anyone who is investigating removing Woodstox from Jenkins core.

§ not quite, as the JDK classes use the system classloader and their may be some extra things, but by inlarge you can think of it that every class can see every other class.

[strangelookingnerd]

Thank you very much for taking the time to explain it further, much appreciated!

So unless we get a JUnit5RealJenkinsRule there is no way to make these kind of tests to validate properly with JUnit5. I already had a closer look into the topic a while ago, maybe it's about time to pick it up again.

Thanks again, I'll close the PR.