ExpediaGroup / jenkins-spock

Unit-test Jenkins pipeline code with Spock
https://javadoc.io/doc/com.homeaway.devtools.jenkins/jenkins-spock
Apache License 2.0
187 stars 76 forks source link

Classloader issues #65

Closed rnc closed 4 years ago

rnc commented 4 years ago

Expected Behavior / Actual Behavior

I am seeing some strange behaviour with classloaders and when tests use loadPipelineScriptForTest. Objects are loaded in a groovy classloader instead of the standard app classloader.

I am not sure whether this is to be 'expected' or there is an error in the way we have setup our project.

Steps to Reproduce

I did try to make a unit test but found it easier to provide a cut down project showing the error and a modified loadPipelineScriptForTest -> loadCustomPipelineScriptForTest that resolves the issue for me. The project may be found at https://github.com/rnc/classloader-reproducer

awittha commented 4 years ago

Is this just an unexpected behavior, or does it create a problem in the process of testing some of your pipeline code?

rnc commented 4 years ago

@awittha As per the linked repository I have had to create a custom loadPipelineScriptForTest to make things work.

awittha commented 4 years ago

In the linked repository, mvn clean verify results in 3 passing tests for me, which print out information:

### setupAdvisory classloader groovy.lang.GroovyClassLoader$InnerLoader@1aa6e3c0
### this classloader sun.misc.Launcher$AppClassLoader@7d4991ad
### Errata classloader sun.misc.Launcher$AppClassLoader@7d4991ad
### reflected class com.redhat.cpaas.pipeline.model.Errata and returned class com.redhat.cpaas.pipeline.model.Errata
### setupAdvisory classloader groovy.lang.GroovyClassLoader$InnerLoader@69228e85
### this classloader sun.misc.Launcher$AppClassLoader@7d4991ad
### Errata classloader sun.misc.Launcher$AppClassLoader@7d4991ad
### reflected class com.redhat.cpaas.pipeline.model.Errata and returned class com.redhat.cpaas.pipeline.model.Errata
### setupAdvisory classloader groovy.lang.GroovyClassLoader$InnerLoader@1835d3ed
### this classloader sun.misc.Launcher$AppClassLoader@7d4991ad
### Errata classloader sun.misc.Launcher$AppClassLoader@7d4991ad
### reflected class com.redhat.cpaas.pipeline.model.Errata and returned class com.redhat.cpaas.pipeline.model.Errata

Looking at the test code, I see that they are highlighting different classloading behavior, but it is not clear to me why that matters. It would be perfectly-legal to have a custom JenkinsSpockClassloader, even, that loaded classes unless that prevented consumers from being able to test their pipeline code.

Is there such a case? Maybe using reflection in Groovy classes in src/main/groovy of a shared library, or something?

I see that the Classloader is different between some classes (and that would be expected given the current implementation), but what problem does that cause?

rnc commented 4 years ago

Apologies I didn't make it clear enough.

So with the standard loadPipelineScriptForTest for some reason the Errata class is being created in the GroovyClassLoader$InnerLoader ; however with the modified loadPipelineScriptForTest it is then created in the root/app classloader which means the comparison works. Obviously its a very contrived example to demonstrate the class loading. I think its due to use of GroovyScriptEngine in loadPipelineScriptForTest but I am not clear why that was chosen as opposed to just using GroovyClassLoader. Perhaps you could explain that to me?

This causes a problem for us because the class loading then means that tests that verify and use class objects can unexpectedly fail as objects are being created in different classloaders. Attempting to replicate this inside the jenkins-spock repository with a unit test was problematic as the Errata class was always being created with the root/app classloader instead of, as in this case, the GroovyClassLoader.

awittha commented 4 years ago

I do not remember why that particular approach was chosen.

I will experiment on a branch and see if the change can be made without breaking things.

awittha commented 4 years ago

I figured it out!

You must make the vars available on the classpath in order for them to be loaded with the same classloader as the test, well, classes.

In the case of a shared library, this section: https://github.com/rnc/classloader-reproducer/blob/9937715b6f306b6db16470300171f08800a8c2a3/pom.xml#L299-L303 of the pom.xml needs to look like this:

<testResources>
  <testResource>
      <includes>
          <include>vars/**/*.groovy</include>
      </includes>
      <directory>${project.basedir}</directory>
  </testResource>
  <testResource>
      <directory>test/resources</directory>
  </testResource>
  <testResource>
      <directory>resources</directory>
  </testResource>
</testResources>

-- https://github.com/ExpediaGroup/jenkins-spock/blob/master/examples/shared-library/pom.xml#L281-L294

Without that, Groovy is falling back to browsing the filesystem to find your scripts, and those are indeed loaded with a different classloader than content on the classpath.

With that, Groovy finds your scripts on the classpath, just like it found your test classes, and loads them with the same classloader, giving you the expected class identity.

Now, this is indeed a confusing user experience.

Probably, one or more of the following should happen:

  1. There should be a "how to use" wiki in this repo that would've told you about the <testResources> section
  2. jenkins-spock should've failed fast when your scripts were not found on the classpath
  3. jenkins-spock should've printed a warning when it detected that the Groovy engine was going to fall back to loading your script from the filesystem
  4. jenkins-spock should load classpath and filesystem resources with the same classloader, regardless

I'm inclined to prefer items 1 and 2.

I am not a huge fan filesystem-based script resolution in this case; I think the classpath is a neater, more-normalized way to find the resources.