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-65757] only use pluginFirst classloader when you have no other option (and never for guice!) #151

Merged
merged 7 commits into from
Jun 17, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
176 changes: 3 additions & 173 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ THE SOFTWARE.
<parent>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>plugin</artifactId>
<version>4.18</version>
<version>4.19</version>
</parent>

<artifactId>jacoco</artifactId>
Expand All @@ -44,8 +44,8 @@ THE SOFTWARE.
<jacoco.version>0.8.7</jacoco.version>
<!-- see https://www.jenkins.io/changelog-stable/ for changelog of the LTS releases -->
<jenkins.version>2.263.4</jenkins.version>
<maven.version>3.8.1</maven.version>
<powermock.version>2.0.7</powermock.version>
<!-- until the plugin-pom picks up a newer version -->
<jenkins-test-harness.version>1589.vc23fca066d5c</jenkins-test-harness.version>
Copy link
Member

Choose a reason for hiding this comment

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

<!-- Do not fail if tests are run without argLine -->
<argLine />
</properties>
Expand Down Expand Up @@ -113,117 +113,6 @@ THE SOFTWARE.
</repository>
</distributionManagement>

<!-- enforce versions of transitive dependencies to match RequireUpperBoundDeps rule -->
<dependencyManagement>
<dependencies>
<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
<version>2.8.0</version>
</dependency>
<dependency>
<groupId>org.apache.ant</groupId>
<artifactId>ant</artifactId>
<version>1.10.9</version>
</dependency>
<dependency>
<groupId>org.apache.commons</groupId>
<artifactId>commons-lang3</artifactId>
<version>3.12.0</version>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpclient</artifactId>
<version>4.5.13</version>
</dependency>
<dependency>
<groupId>org.apache.maven</groupId>
<artifactId>maven-core</artifactId>
<version>${maven.version}</version>
</dependency>
<dependency>
<groupId>org.apache.maven</groupId>
<artifactId>maven-embedder</artifactId>
<version>${maven.version}</version>
</dependency>
<dependency>
<groupId>org.apache.maven</groupId>
<artifactId>maven-repository-metadata</artifactId>
<version>${maven.version}</version>
</dependency>
<dependency>
<groupId>org.apache.maven.reporting</groupId>
<artifactId>maven-reporting-api</artifactId>
<version>3.0</version>
</dependency>
<dependency>
<groupId>org.apache.maven.doxia</groupId>
<artifactId>doxia-sink-api</artifactId>
<version>1.9.1</version>
</dependency>
<dependency>
<groupId>org.apache.maven.doxia</groupId>
<artifactId>doxia-site-renderer</artifactId>
<version>1.7.4</version>
</dependency>
<dependency>
<groupId>org.apache.maven.reporting</groupId>
<artifactId>maven-reporting-impl</artifactId>
<version>3.0.0</version>
</dependency>
<dependency>
<groupId>org.apache.maven.shared</groupId>
<artifactId>maven-shared-utils</artifactId>
<version>3.2.1</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-velocity</artifactId>
<version>1.2</version>
</dependency>
<dependency>
<groupId>org.apache.maven</groupId>
<artifactId>maven-aether-provider</artifactId>
<version>3.3.9</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-classworlds</artifactId>
<version>2.6.0</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-component-annotations</artifactId>
<version>2.1.0</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-container-default</artifactId>
<version>1.7.1</version>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-interpolation</artifactId>
<version>1.24</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.main</groupId>
<artifactId>maven-plugin</artifactId>
<version>3.7</version>
</dependency>
<dependency>
<groupId>com.google.inject</groupId>
<artifactId>guice</artifactId>
<version>4.2.1</version>
</dependency>
<dependency>
<groupId>com.google.guava</groupId>
<artifactId>guava</artifactId>
<version>30.1-jre</version>
</dependency>
</dependencies>
</dependencyManagement>

<dependencies>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
Expand All @@ -245,7 +134,6 @@ THE SOFTWARE.
<dependency>
<groupId>org.powermock</groupId>
<artifactId>powermock-module-junit4</artifactId>
<version>${powermock.version}</version>
<scope>test</scope>
</dependency>
<dependency>
Expand All @@ -257,70 +145,13 @@ THE SOFTWARE.
<dependency>
<groupId>org.hamcrest</groupId>
<artifactId>hamcrest-core</artifactId>
<version>2.2</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.jacoco</groupId>
<artifactId>jacoco-maven-plugin</artifactId>
<version>${jacoco.version}</version>
<!-- Exclude a few unused transitive dependencies of things that are not needed here to avoid clashes with other plugins -->
<exclusions>
<exclusion>
<groupId>org.apache.maven.doxia</groupId>
<artifactId>*</artifactId>
</exclusion>
<exclusion>
<groupId>org.codehaus.plexus</groupId>
<artifactId>*</artifactId>
</exclusion>
<exclusion>
<groupId>org.sonatype.plexus</groupId>
<artifactId>*</artifactId>
</exclusion>
</exclusions>
</dependency>
<dependency>
<groupId>org.apache.maven</groupId>
<artifactId>maven-plugin-api</artifactId>
<version>${maven.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.maven</groupId>
<artifactId>maven-artifact</artifactId>
<version>${maven.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.maven</groupId>
<artifactId>maven-model</artifactId>
<version>${maven.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.apache.maven</groupId>
<artifactId>maven-settings</artifactId>
<version>${maven.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.codehaus.plexus</groupId>
<artifactId>plexus-utils</artifactId>
<version>3.2.1</version>
</dependency>
<!-- Somehow we need org.w3c.dom.ElementTraversal, but do not get it from any of the dependencies
do not update to 2.x as it will not contain the class any more! -->
<dependency>
<groupId>xml-apis</groupId>
<artifactId>xml-apis</artifactId>
<version>1.4.01</version>
</dependency>
<dependency>
<groupId>org.apache.httpcomponents</groupId>
<artifactId>httpcore</artifactId>
<version>4.4.13</version>
</dependency>
<dependency>
<groupId>org.jenkins-ci.plugins</groupId>
<artifactId>structs</artifactId>
Expand Down Expand Up @@ -530,7 +361,6 @@ THE SOFTWARE.
</execution>
</executions>
<configuration>
<pluginFirstClassLoader>true</pluginFirstClassLoader>
Copy link
Member Author

Choose a reason for hiding this comment

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

we do not need this, we are safe with maskClasses now after removing other conflicting libraries

<maskClasses>org.objectweb.asm.</maskClasses>
</configuration>
</plugin>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import org.jacoco.core.data.ExecutionDataReader;
import org.jacoco.core.data.ExecutionDataStore;
import org.jacoco.core.data.SessionInfoStore;
import org.jacoco.maven.FileFilter;
Copy link
Member Author

Choose a reason for hiding this comment

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

this caused a whole slew of the maven dependencies and for no real reason.



public class ExecutionFileLoader implements Serializable {
Expand Down Expand Up @@ -122,9 +121,8 @@ private IBundleCoverage analyzeStructure() throws IOException {
excludes = ITEM_ZERO;
Copy link
Member Author

@jtnord jtnord Jun 7, 2021

Choose a reason for hiding this comment

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

so this is the default exclude - but I have no idea why. the default in maven in the empty String, but here we use "{0}" 🤷

}

final FileFilter fileFilter = new FileFilter(Arrays.asList(includes), Arrays.asList(excludes));
try {
final List<File> filesToAnalyze = FileUtils.getFiles(classDirectory, fileFilter.getIncludes(), fileFilter.getExcludes());
final List<File> filesToAnalyze = FileUtils.getFiles(classDirectory, String.join(",", includes), String.join(",", excludes));
for (final File file : filesToAnalyze) {
analyzer.analyzeAll(file);
}
Expand Down
108 changes: 108 additions & 0 deletions src/test/java/hudson/plugins/jacoco/e2e/E2ETest.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
package hudson.plugins.jacoco.e2e;

import java.util.ArrayList;
import java.util.List;

import org.hamcrest.Description;
import org.hamcrest.TypeSafeDiagnosingMatcher;
import org.junit.Rule;
import org.junit.Test;
import org.jvnet.hudson.test.RealJenkinsRule;

import hudson.Functions;
import hudson.model.Descriptor;
import hudson.model.FreeStyleBuild;
import hudson.model.FreeStyleProject;
import hudson.plugins.jacoco.JacocoBuildAction;
import hudson.plugins.jacoco.JacocoPublisher;
import hudson.plugins.jacoco.model.Coverage;
import hudson.tasks.BatchFile;
import hudson.tasks.Builder;
import hudson.tasks.Publisher;
import hudson.tasks.Shell;
import hudson.util.DescribableList;

import static org.hamcrest.CoreMatchers.*;
import static org.hamcrest.MatcherAssert.assertThat;

import static hudson.plugins.jacoco.e2e.E2ETest.CoverageMatcher.withCoverage;

public class E2ETest {

@Rule
public RealJenkinsRule rjr = new RealJenkinsRule();
Copy link
Member Author

Choose a reason for hiding this comment

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


@Test
public void simpleTest() throws Throwable {
rjr.then(r -> {
FreeStyleProject project = r.createFreeStyleProject();
project.getBuildersList().addAll(createJacocoProjectBuilders());
project.getPublishersList().add(new JacocoPublisher());

FreeStyleBuild build = r.buildAndAssertSuccess(project);

assertThat("plugin collected data", build.getLog(), containsString("Collecting JaCoCo coverage data"));

JacocoBuildAction action = build.getAction(JacocoBuildAction.class);
assertThat("Build has the Jacoco Action", action, notNullValue());

assertThat("incorrect branch coverage reported", action.getBranchCoverage(), withCoverage(0, 621, 621));
assertThat("incorrect class coverage reported", action.getClassCoverage(), withCoverage(7, 59, 66));
assertThat("incorrect complexity coverage reported", action.getComplexityScore(), withCoverage(19, 835, 854));
assertThat("incorrect instruction coverage reported", action.getInstructionCoverage(), withCoverage(229, 9013, 9242));
assertThat("incorrect line coverage reported", action.getLineCoverage(), withCoverage(53, 1860, 1913));
}
);
}

private static List<Builder> createJacocoProjectBuilders() {
String[] commands = { "git clone --branch jacoco-3.2.0 https://github.com/jenkinsci/jacoco-plugin.git .",
Copy link
Member Author

Choose a reason for hiding this comment

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

whilst running maven inside a test is not the best, we are using this project as the test (and a tag) so we at least have most of the dependencies already local.

It will diverge over time, so we may want to try and keep this somewhat in sync

Copy link
Member

Choose a reason for hiding this comment

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

So binaries "git" and "mvn" need to be available on the CI node for this to work, right?

Copy link
Member Author

@jtnord jtnord Jun 8, 2021

Choose a reason for hiding this comment

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

yes, but maven already is as it is what runs the build that runs this test :)

as for git the CI could have cloned using jgit rather than the git CLI so it may not be available, but ci.jenkins.io is not setup like that.
A developer could work exclusively with IntelliJ or Eclipse (again jgit) but I am not convinced that is likely.

Copy link
Member

Choose a reason for hiding this comment

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

For a more reproducible environment:

  • store a small test project in src/test/resources/sampleproj/
  • use dependency:copy to unpack a specific release of Maven into target/test-classes/

but still use the Java in path I guess.

"mvn -P enable-jacoco -B -Dtest=ClassReportTest test" };
List<Builder> builders = new ArrayList<>();
if (Functions.isWindows()) {
for (String command : commands) {
builders.add(new BatchFile(command));
}
} else {
for (String command : commands) {
builders.add(new Shell(command));
}
}
return builders;
}

public static class CoverageMatcher extends TypeSafeDiagnosingMatcher<Coverage> {

private final int covered;
private final int missed;
private final int total;

private CoverageMatcher(int covered, int missed, int total) {
this.covered = covered;
this.missed = missed;
this.total = total;
}
@Override
public void describeTo(Description description) {
description.appendText(" with covered="+ covered);
description.appendText(" and missed="+ missed);
description.appendText(" and total="+ total);

}

@Override
protected boolean matchesSafely(Coverage coverage, Description mismatchDescription) {
mismatchDescription.appendText(" with covered="+ coverage.getCovered());
mismatchDescription.appendText(" and missed="+ coverage.getMissed());
mismatchDescription.appendText(" and total="+ coverage.getTotal());

return coverage.getCovered() == covered &&
coverage.getMissed() == missed &&
coverage.getTotal() == total;
}

public static CoverageMatcher withCoverage(int covered, int missed, int total) {
return new CoverageMatcher(covered, missed, total);
}
}
}