From bb25b8524cbaa2f5c11226140617c994672b3ce5 Mon Sep 17 00:00:00 2001 From: demonfiddler Date: Wed, 11 Nov 2015 17:01:07 +0000 Subject: [PATCH 1/3] [FIXED JENKINS-31258] - Unknown test plug-ins can now specify the path to their reports directory via the Maven project property jenkins..reportsDirectory. Relative paths are resolved against the project's base directory. - Unit test for the above capability. - Fixed a compilation failure that was apparently caused an extraneous (and inconsistent!) Unicode right-apostrophe character instead of a single quote in Messages.properties --- .../java/hudson/maven/reporters/TestMojo.java | 65 +++++++++++++++--- .../hudson/maven/Messages.properties | 2 +- .../java/hudson/maven/MojoInfoBuilder.java | 47 +++++++------ .../hudson/maven/reporters/TestMojoTest.java | 67 +++++++++++++++++++ 4 files changed, 150 insertions(+), 31 deletions(-) diff --git a/src/main/java/hudson/maven/reporters/TestMojo.java b/src/main/java/hudson/maven/reporters/TestMojo.java index 97eb3ca05..596e83747 100644 --- a/src/main/java/hudson/maven/reporters/TestMojo.java +++ b/src/main/java/hudson/maven/reporters/TestMojo.java @@ -13,6 +13,7 @@ import org.apache.maven.project.MavenProject; import org.apache.tools.ant.types.FileSet; import org.codehaus.plexus.component.configurator.ComponentConfigurationException; +import org.codehaus.plexus.component.configurator.expression.ExpressionEvaluationException; import com.google.common.base.Function; import com.google.common.collect.Iterators; @@ -167,21 +168,34 @@ public boolean canRunTests(MojoInfo mojo) { } @CheckForNull public Iterable getReportFiles(MavenProject pom, MojoInfo mojo) throws ComponentConfigurationException { + File reportsDir = getReportsDirectory(pom, mojo); + if (reportsDir != null && reportsDir.exists()) + return getReportFiles(reportsDir, getFileSet(reportsDir)); + + return null; + } + + /** + * Returns the location of test reports created by the specified MOJO. + * @param pom The project model. + * @param mojo The MOJO. + * @return The directory containing the test reports. + * @throws ComponentConfigurationException if unable to retrieve the report directory from the MOJO configuration. + */ + protected File getReportsDirectory(MavenProject pom, MojoInfo mojo) throws ComponentConfigurationException { + // [JENKINS-31258] Allow unknown MOJOs to contribute test results in arbitrary locations by setting a Maven property. + String reportsDirectoryOverride = getReportsDirectoryOverride(mojo); + if (reportsDirectoryOverride != null) + return mojo.expressionEvaluator.alignToBaseDirectory(new File(reportsDirectoryOverride)); + if (this.reportDirectoryConfigKey != null) { File reportsDir = mojo.getConfigurationValue(this.reportDirectoryConfigKey, File.class); - if (reportsDir != null && reportsDir.exists()) { - return getReportFiles(reportsDir, getFileSet(reportsDir)); - } - + if (reportsDir != null) + return reportsDir; } // some plugins just default to this: - File reportsDir = new File(pom.getBuild().getDirectory(), "surefire-reports"); - if (reportsDir.exists()) { - return getReportFiles(reportsDir, getFileSet(reportsDir)); - } - - return null; + return new File(pom.getBuild().getDirectory(), "surefire-reports"); } private Iterable getReportFiles(final File baseDir, FileSet set) { @@ -231,12 +245,41 @@ public static TestMojo lookup(String artifactId, String groupId, String goal) { public static TestMojo lookup(MojoInfo mojo) { TestMojo testMojo = lookup(mojo.pluginName.groupId, mojo.pluginName.artifactId, mojo.getGoal()); + + if (testMojo == null && getReportsDirectoryOverride(mojo) != null) { + testMojo = FALLBACK; + } + if (testMojo != null && testMojo.canRunTests(mojo)) { return testMojo; } + return null; } - + + /** + * For an unknown test-capable MOJO, returns the path to its reports directory as configured by a Maven property. + * @param mojo An unknown MOJO. + * @return the value of the expression ${jenkins.<mojo-execution-id>.reportsDirectory}. + */ + private static String getReportsDirectoryOverride(MojoInfo mojo) { + // Validity of the override property should be constrained to "test" and "integration-test" phases but there seems to be no + // way to check this, as MojoExecution.getLifecyclePhase() (added 2009-05-12) causes tests to throw NoSuchMethodError!!! + // So, we're obliged to assume that the property is configured for a valid test life cycle phase. +// String phase = mojo.mojoExecution.getLifecyclePhase(); +// if ("test".equals(phase) || "integration-test".equals(phase)) { + try { + String reportsDirExpr = "${jenkins." + mojo.mojoExecution.getExecutionId() + ".reportsDirectory}"; + Object result = mojo.expressionEvaluator.evaluate(reportsDirExpr); + if (result != null && !result.equals(reportsDirExpr) && !result.toString().trim().isEmpty()) + return result.toString().trim(); + } catch (ExpressionEvaluationException e) { + // Ignore evaluation exceptions. + } +// } + return null; + } + static class Key { private String artifactId; private String groupId; diff --git a/src/main/resources/hudson/maven/Messages.properties b/src/main/resources/hudson/maven/Messages.properties index c83c1a5db..37b28829a 100644 --- a/src/main/resources/hudson/maven/Messages.properties +++ b/src/main/resources/hudson/maven/Messages.properties @@ -55,7 +55,7 @@ MavenProbeAction.DisplayName=Monitor Maven Process MavenProcessFactory.ClassWorldsNotFound=No classworlds*.jar found in {0} -- Is this a valid maven directory? MavenRedeployer.DisplayName=Deploy to Maven repository -MavenVersionCallable.MavenHomeDoesntExist=Maven Home {0} doesn\u2019t exist +MavenVersionCallable.MavenHomeDoesntExist=Maven Home {0} doesn't exist MavenVersionCallable.MavenHomeIsNotDirectory=Maven Home {0} is not a directory ProcessCache.Reusing=Reusing existing maven process diff --git a/src/test/java/hudson/maven/MojoInfoBuilder.java b/src/test/java/hudson/maven/MojoInfoBuilder.java index be59ee773..87968e1b9 100644 --- a/src/test/java/hudson/maven/MojoInfoBuilder.java +++ b/src/test/java/hudson/maven/MojoInfoBuilder.java @@ -19,6 +19,19 @@ public class MojoInfoBuilder { private String artifactId; private String goalName; private String version = "1.0"; + private String executionId; + private ExpressionEvaluator evaluator = new ExpressionEvaluator() { + @Override + public Object evaluate(String expression) { + return expression; + } + + @Override + public File alignToBaseDirectory(File file) { + return file; + } + }; + private Map configValues = new HashMap(); private long startTime = System.currentTimeMillis(); @@ -34,7 +47,7 @@ private MojoInfoBuilder(String groupId, String artifactId, String goalName) { public MojoInfoBuilder copy() { MojoInfoBuilder copy = new MojoInfoBuilder(this.groupId, this.artifactId, this.goalName) - .version(this.version); + .version(this.version).executionId(this.executionId).evaluator(this.evaluator); copy.configValues.putAll(this.configValues); return copy; } @@ -44,12 +57,22 @@ public MojoInfoBuilder version(String version) { return this; } + public MojoInfoBuilder executionId(String executionId) { + this.executionId = executionId; + return this; + } + + public MojoInfoBuilder evaluator(ExpressionEvaluator evaluator) { + this.evaluator = evaluator; + return this; + } + public MojoInfoBuilder startTime(long startTime) { this.startTime = startTime; return this; } - public MojoInfoBuilder configValue(String key,String value) { + public MojoInfoBuilder configValue(String key, String value) { configValues.put(key, value); return this; } @@ -64,28 +87,14 @@ public MojoInfo build() { mojoDescriptor.setPluginDescriptor(pluginDescriptor); mojoDescriptor.setGoal(goalName); - MojoExecution mojoExecution = new MojoExecution(mojoDescriptor); + MojoExecution mojoExecution = new MojoExecution(mojoDescriptor, executionId); PlexusConfiguration configuration = new DefaultPlexusConfiguration("configuration"); for (Map.Entry e : this.configValues.entrySet()) { - configuration.addChild(e.getKey(),e.getValue()); + configuration.addChild(e.getKey(), e.getValue()); } - ExpressionEvaluator evaluator = new ExpressionEvaluator() { - @Override - public Object evaluate(String expression) { - return expression; - } - - @Override - public File alignToBaseDirectory(File file) { - return file; - } - }; - - MojoInfo info = new MojoInfo(mojoExecution, null, configuration, evaluator, startTime); + MojoInfo info = new MojoInfo(mojoExecution, null, configuration, evaluator, startTime); return info; } - - } diff --git a/src/test/java/hudson/maven/reporters/TestMojoTest.java b/src/test/java/hudson/maven/reporters/TestMojoTest.java index 949510b86..2090dbb29 100644 --- a/src/test/java/hudson/maven/reporters/TestMojoTest.java +++ b/src/test/java/hudson/maven/reporters/TestMojoTest.java @@ -15,9 +15,12 @@ import org.apache.maven.model.Build; import org.apache.maven.project.MavenProject; +import org.codehaus.plexus.component.configurator.expression.ExpressionEvaluationException; +import org.codehaus.plexus.component.configurator.expression.ExpressionEvaluator; import org.codehaus.plexus.component.configurator.ComponentConfigurationException; import org.junit.Test; import org.jvnet.hudson.test.Bug; +import org.jvnet.hudson.test.Issue; public class TestMojoTest { @@ -219,4 +222,68 @@ private static interface ScalatestPluginTest { public void run(MojoInfoBuilder mojoBuilder, MavenProject pom, String testResultsName) throws Exception; } + + @Test + @Issue("JENKINS-31258") + public void testGetReportFilesUnknownTestPlugin() throws Exception { + final String testResultsName = "TEST-some.test.results.xml"; + + final File baseDir = hudson.Util.createTempDir(); + final File targetDir = new File(baseDir, "target"); + final File reportsDir = new File(targetDir, "some-test-reports"); + assertTrue(reportsDir.mkdirs()); + + MojoInfoBuilder builder = MojoInfoBuilder.mojoBuilder("com.some", "unknown-test-capable-plugin", "somegoal") + .evaluator(new ExpressionEvaluator() { + @Override + public Object evaluate(String expression) throws ExpressionEvaluationException { + if ("${jenkins.some-test-execution-id.reportsDirectory}".equals(expression)) + return "target/some-test-reports"; + return expression; + } + + @Override + public File alignToBaseDirectory(File path) { + return new File(baseDir, path.getPath()); + } + }); + + MojoInfo nonReportingMojo = builder.executionId("some-non-test-execution-id").build(); + TestMojo testMojo = TestMojo.lookup(nonReportingMojo); + assertNull("misreported an unknown, unconfigured MOJO as test capable", testMojo); + + MojoInfo reportingMojo = builder.executionId("some-test-execution-id").build(); + testMojo = TestMojo.lookup(reportingMojo); + assertNotNull("failed to recognize correctly configured unknown test capable MOJO", testMojo); + + MavenProject pom = mock(MavenProject.class); + when(pom.getBasedir()).thenReturn(baseDir); + + Build build = mock(Build.class); + when(build.getDirectory()).thenReturn(targetDir.getAbsolutePath()); + when(pom.getBuild()).thenReturn(build); + + assertEquals("test mojo returned incorrect reports directory", new File(baseDir, "target/some-test-reports"), + testMojo.getReportsDirectory(pom, reportingMojo)); + + File testResults = new File(reportsDir, testResultsName); + try { + FileWriter fw = new FileWriter(testResults, false); + fw.write("this is a fake surefire reports output file"); + fw.close(); + + Iterable files = testMojo.getReportFiles(pom, reportingMojo); + assertNotNull("no report files returned", files); + + boolean found = false; + for (File file : files) { + assertEquals(testResultsName, file.getName()); + found = true; + } + assertTrue("report file not found", found); + } finally { + hudson.Util.deleteRecursive(baseDir); + } + } + } From 5945f85d9d12b430315cf2124dde0727197a395f Mon Sep 17 00:00:00 2001 From: demonfiddler Date: Thu, 12 Nov 2015 15:18:36 +0000 Subject: [PATCH 2/3] [FIXED JENKINS-31524] - Handle the case where existing test results have been updated by a subsequent test MOJO execution within the same build. --- .../hudson/maven/reporters/SurefireArchiver.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/main/java/hudson/maven/reporters/SurefireArchiver.java b/src/main/java/hudson/maven/reporters/SurefireArchiver.java index 52c8cf798..2ecb3cf43 100644 --- a/src/main/java/hudson/maven/reporters/SurefireArchiver.java +++ b/src/main/java/hudson/maven/reporters/SurefireArchiver.java @@ -74,10 +74,10 @@ public class SurefireArchiver extends TestFailureDetector { private final AtomicBoolean hasTestFailures = new AtomicBoolean(); /** - * Store result files already parsed, so we don't parse them again, - * if a later running mojo specifies the same reports directory. + * Store result files and modification timestamps already parsed, so we don't parse them again + * if a later running MOJO specifies the same reports directory. */ - private transient ConcurrentMap parsedFiles = new ConcurrentHashMap(); + private transient ConcurrentMap parsedFiles = new ConcurrentHashMap(); @Override public boolean hasTestFailures() { @@ -138,7 +138,7 @@ public boolean postExecute(MavenBuildProxy build, MavenProject pom, MojoInfo moj fileSet = Iterables.filter(fileSet, new Predicate() { @Override public boolean apply(File input) { - return !parsedFiles.containsKey(input); + return !parsedFiles.containsKey(input) || parsedFiles.get(input) < input.lastModified(); } }); @@ -213,7 +213,7 @@ private void markBuildAsSuccess(Throwable mojoError, MavenBuildInformation build */ private void rememberCheckedFiles(Iterable fileSet) { for (File f : fileSet) { - this.parsedFiles.put(f, f); + this.parsedFiles.put(f, f.lastModified()); } } @@ -330,7 +330,7 @@ private TestMojo getTestMojo(MojoInfo mojo) { // I'm not sure if SurefireArchiver is actually ever (de-)serialized, // but just to be sure, set fileSets here protected Object readResolve() { - parsedFiles = new ConcurrentHashMap(); + parsedFiles = new ConcurrentHashMap(); return this; } From 45712ad8938580c1ae1a8c76095917f119d3f0be Mon Sep 17 00:00:00 2001 From: Adrian Price Date: Fri, 15 Jan 2016 17:07:46 +0000 Subject: [PATCH 3/3] Implement Oleg's code review suggestions: - replace extraneous tab characters with spaces - follow Oracle coding standards - remove unnecessary null check - add test for JENKINS-31258 'detect test Mojo identified by Jenkins Maven Property' - add test for JENKINS-31524 'ensure updated existing results are counted' --- .../maven/reporters/SurefireArchiver.java | 2 +- .../java/hudson/maven/reporters/TestMojo.java | 25 +++++++++------- .../SurefireArchiverDetectTestMojosTest.java | 29 +++++++++++++++++++ .../reporters/SurefireArchiverUnitTest.java | 28 ++++++++++++++++++ 4 files changed, 73 insertions(+), 11 deletions(-) diff --git a/src/main/java/hudson/maven/reporters/SurefireArchiver.java b/src/main/java/hudson/maven/reporters/SurefireArchiver.java index 2ecb3cf43..159a0aa1e 100644 --- a/src/main/java/hudson/maven/reporters/SurefireArchiver.java +++ b/src/main/java/hudson/maven/reporters/SurefireArchiver.java @@ -138,7 +138,7 @@ public boolean postExecute(MavenBuildProxy build, MavenProject pom, MojoInfo moj fileSet = Iterables.filter(fileSet, new Predicate() { @Override public boolean apply(File input) { - return !parsedFiles.containsKey(input) || parsedFiles.get(input) < input.lastModified(); + return !parsedFiles.containsKey(input) || parsedFiles.get(input) != input.lastModified(); } }); diff --git a/src/main/java/hudson/maven/reporters/TestMojo.java b/src/main/java/hudson/maven/reporters/TestMojo.java index 596e83747..d5691862f 100644 --- a/src/main/java/hudson/maven/reporters/TestMojo.java +++ b/src/main/java/hudson/maven/reporters/TestMojo.java @@ -9,6 +9,7 @@ import java.util.Iterator; import javax.annotation.CheckForNull; +import javax.annotation.Nonnull; import org.apache.maven.project.MavenProject; import org.apache.tools.ant.types.FileSet; @@ -169,8 +170,9 @@ public boolean canRunTests(MojoInfo mojo) { @CheckForNull public Iterable getReportFiles(MavenProject pom, MojoInfo mojo) throws ComponentConfigurationException { File reportsDir = getReportsDirectory(pom, mojo); - if (reportsDir != null && reportsDir.exists()) + if (reportsDir.exists()) { return getReportFiles(reportsDir, getFileSet(reportsDir)); + } return null; } @@ -182,11 +184,11 @@ public boolean canRunTests(MojoInfo mojo) { * @return The directory containing the test reports. * @throws ComponentConfigurationException if unable to retrieve the report directory from the MOJO configuration. */ - protected File getReportsDirectory(MavenProject pom, MojoInfo mojo) throws ComponentConfigurationException { - // [JENKINS-31258] Allow unknown MOJOs to contribute test results in arbitrary locations by setting a Maven property. - String reportsDirectoryOverride = getReportsDirectoryOverride(mojo); - if (reportsDirectoryOverride != null) - return mojo.expressionEvaluator.alignToBaseDirectory(new File(reportsDirectoryOverride)); + @Nonnull protected File getReportsDirectory(MavenProject pom, MojoInfo mojo) throws ComponentConfigurationException { + // [JENKINS-31258] Allow unknown MOJOs to contribute test results in arbitrary locations by setting a Maven property. + String reportsDirectoryOverride = getReportsDirectoryOverride(mojo); + if (reportsDirectoryOverride != null) + return mojo.expressionEvaluator.alignToBaseDirectory(new File(reportsDirectoryOverride)); if (this.reportDirectoryConfigKey != null) { File reportsDir = mojo.getConfigurationValue(this.reportDirectoryConfigKey, File.class); @@ -247,7 +249,7 @@ public static TestMojo lookup(MojoInfo mojo) { TestMojo testMojo = lookup(mojo.pluginName.groupId, mojo.pluginName.artifactId, mojo.getGoal()); if (testMojo == null && getReportsDirectoryOverride(mojo) != null) { - testMojo = FALLBACK; + testMojo = FALLBACK; } if (testMojo != null && testMojo.canRunTests(mojo)) { @@ -262,10 +264,12 @@ public static TestMojo lookup(MojoInfo mojo) { * @param mojo An unknown MOJO. * @return the value of the expression ${jenkins.<mojo-execution-id>.reportsDirectory}. */ - private static String getReportsDirectoryOverride(MojoInfo mojo) { + @CheckForNull private static String getReportsDirectoryOverride(MojoInfo mojo) { + // TODO: if and when this MOJO ceases to be tested against earlier Maven versions lacking MojoExecution.getLifecyclePhase(), + // Reinstate the following validity check: // Validity of the override property should be constrained to "test" and "integration-test" phases but there seems to be no // way to check this, as MojoExecution.getLifecyclePhase() (added 2009-05-12) causes tests to throw NoSuchMethodError!!! - // So, we're obliged to assume that the property is configured for a valid test life cycle phase. + // Until then, we're obliged to assume that the property is configured for a valid test life cycle phase. // String phase = mojo.mojoExecution.getLifecyclePhase(); // if ("test".equals(phase) || "integration-test".equals(phase)) { try { @@ -274,7 +278,8 @@ private static String getReportsDirectoryOverride(MojoInfo mojo) { if (result != null && !result.equals(reportsDirExpr) && !result.toString().trim().isEmpty()) return result.toString().trim(); } catch (ExpressionEvaluationException e) { - // Ignore evaluation exceptions. + // Note: no access to private MavenProject.logger. + e.printStackTrace(); } // } return null; diff --git a/src/test/java/hudson/maven/reporters/SurefireArchiverDetectTestMojosTest.java b/src/test/java/hudson/maven/reporters/SurefireArchiverDetectTestMojosTest.java index 2ecc28ea9..28bf1aa39 100644 --- a/src/test/java/hudson/maven/reporters/SurefireArchiverDetectTestMojosTest.java +++ b/src/test/java/hudson/maven/reporters/SurefireArchiverDetectTestMojosTest.java @@ -3,11 +3,18 @@ import static hudson.maven.MojoInfoBuilder.mojoBuilder; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; + +import java.io.File; + +import org.codehaus.plexus.component.configurator.expression.ExpressionEvaluationException; +import org.codehaus.plexus.component.configurator.expression.ExpressionEvaluator; + import hudson.maven.MojoInfo; import hudson.maven.MojoInfoBuilder; import org.junit.Before; import org.junit.Test; +import org.jvnet.hudson.test.Issue; /** * Regression test for the detection of test mojos in {@link SurefireArchiver}. @@ -197,6 +204,28 @@ public void shouldDetectAnyMojoWithATestGoal() { assertTrue(this.surefireArchiver.isTestMojo(mojo)); } + @Test + @Issue("JENKINS-31258") + public void shouldDetectAnyMojoWithAJenkinsReportsDirectoryProperty() { + MojoInfoBuilder builder = mojoBuilder("some.weird.internal", "xxx-mojo", "xxx-goal") + .executionId("xxx-execution-id").evaluator(new ExpressionEvaluator() { + @Override + public Object evaluate(String expression) throws ExpressionEvaluationException { + if ("${jenkins.xxx-execution-id.reportsDirectory}".equals(expression)) + return "target/xxx-test-reports"; + return expression; + } + + @Override + public File alignToBaseDirectory(File path) { + return path; + } + }); + + MojoInfo mojo = builder.build(); + assertTrue(this.surefireArchiver.isTestMojo(mojo)); + } + @Test public void shouldNotDetectNonTestGoal() { MojoInfoBuilder builder = mojoBuilder("some.weird.internal","test-mojo", "verify"); diff --git a/src/test/java/hudson/maven/reporters/SurefireArchiverUnitTest.java b/src/test/java/hudson/maven/reporters/SurefireArchiverUnitTest.java index 36665e8a7..76c1fd318 100644 --- a/src/test/java/hudson/maven/reporters/SurefireArchiverUnitTest.java +++ b/src/test/java/hudson/maven/reporters/SurefireArchiverUnitTest.java @@ -36,6 +36,7 @@ import org.junit.Assert; import org.junit.Before; import org.junit.Test; +import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.RandomlyFails; import org.mockito.Matchers; import org.mockito.invocation.InvocationOnMock; @@ -153,6 +154,33 @@ public void testResultsAreNotCountedTwice() throws InterruptedException, IOExcep assertEquals(2658, result.getTotalCount()); } + @Test + @Issue("JENKINS-31524") + public void testUpdatedExistingResultsAreCounted() throws InterruptedException, IOException, URISyntaxException, ComponentConfigurationException { + URL resource = SurefireArchiverUnitTest.class.getResource("/surefire-archiver-test2"); + File reportsDir = new File(resource.toURI().getPath()); + doReturn(reportsDir).when(this.mojoInfo).getConfigurationValue("reportsDirectory", File.class); + + File report1 = new File(reportsDir, "junit-report-1233.xml"); + File report2 = new File(reportsDir, "junit-report-1472.xml"); + long startTime = this.mojoInfo.getStartTime(); + + // Current report files should be counted and stale ones ignored. + report1.setLastModified(startTime); + report2.setLastModified(startTime - (60 * 1000)); + this.archiver.postExecute(buildProxy, null, this.mojoInfo, new NullBuildListener(), null); + SurefireReport action = this.build.getAction(SurefireReport.class); + TestResult result = action.getResult(); + assertEquals(5, result.getTotalCount()); + + // Updated existing report files should now be included. + report2.setLastModified(startTime); + this.archiver.postExecute(buildProxy, null, this.mojoInfo, new NullBuildListener(), null); + action = this.build.getAction(SurefireReport.class); + result = action.getResult(); + assertEquals(2658, result.getTotalCount()); + } + @Test public void testMultiThreaded() throws InterruptedException, IOException, URISyntaxException, ComponentConfigurationException { File reportsDir2 = new File(SurefireArchiverUnitTest.class.getResource("/surefire-archiver-test2").toURI().getPath());