diff --git a/src/main/java/com/uber/jenkins/phabricator/coverage/CoberturaXMLParser.java b/src/main/java/com/uber/jenkins/phabricator/coverage/CoberturaXMLParser.java index 00c69c6f..b6244c65 100644 --- a/src/main/java/com/uber/jenkins/phabricator/coverage/CoberturaXMLParser.java +++ b/src/main/java/com/uber/jenkins/phabricator/coverage/CoberturaXMLParser.java @@ -118,7 +118,9 @@ private Map> parse(Map> coverageDat NodeList classes = entry.getKey(); List sourceDirs = entry.getValue(); - // Loop over all files in the coverage report + // Collect all filenames in coverage report + List fileNames = new ArrayList(); + List childNodes = new ArrayList(); for (int i = 0; i < classes.getLength(); i++) { Node classNode = classes.item(i); String fileName = classNode.getAttributes().getNamedItem(NODE_FILENAME).getTextContent(); @@ -126,17 +128,24 @@ private Map> parse(Map> coverageDat if (includeFileNames != null && !includeFileNames.contains(FilenameUtils.getName(fileName))) { continue; } + fileNames.add(fileName); + childNodes.add(classNode.getChildNodes()); + } + + // Make multiple guesses on which of the `sourceDirs` contains files in question + Map detectedSourceRoots = new PathResolver(workspace, sourceDirs).choose(fileNames); - // Make a guess on which of the `sourceDirs` contains the file in question - String detectedSourceRoot = new PathResolver(workspace, sourceDirs).choose(fileName); - fileName = join(detectedSourceRoot, fileName); + // Loop over all files which are needed for coverage report + for (int i = 0; i < fileNames.size(); i++) { + String fileName = fileNames.get(i); + fileName = join(detectedSourceRoots.get(fileName), fileName); SortedMap hitCounts = internalCounts.get(fileName); if (hitCounts == null) { hitCounts = new TreeMap(); } - NodeList children = classNode.getChildNodes(); + NodeList children = childNodes.get(i); for (int j = 0; j < children.getLength(); j++) { Node child = children.item(j); diff --git a/src/main/java/com/uber/jenkins/phabricator/coverage/PathResolver.java b/src/main/java/com/uber/jenkins/phabricator/coverage/PathResolver.java index bcd109c7..d1c37479 100644 --- a/src/main/java/com/uber/jenkins/phabricator/coverage/PathResolver.java +++ b/src/main/java/com/uber/jenkins/phabricator/coverage/PathResolver.java @@ -21,9 +21,15 @@ package com.uber.jenkins.phabricator.coverage; import hudson.FilePath; +import hudson.remoting.VirtualChannel; +import jenkins.MasterToSlaveFileCallable; import java.io.IOException; +import java.io.File; +import java.util.Collections; +import java.util.HashMap; import java.util.List; +import java.util.Map; public class PathResolver { private final FilePath root; @@ -35,26 +41,49 @@ public PathResolver(FilePath root, List candidates) { } /** - * Using the workspace's root FilePath and a file that is presumed to exist on the node running the tests, + * Using the workspace's root FilePath and files that is presumed to exist on the node running the tests, * recurse over the `sources` provided by Cobertura and look for a combination where the file exists. * * This is a heuristic, and not perfect, to overcome changes to Python's coverage.py module which introduced * additional `source` directories in version 4.0.3 + * + * Returns map where key is filename and value - sourceDirectory */ - public String choose(String filename) { - for (String sourceDir : candidates) { - FilePath candidate = new FilePath(root, sourceDir); - candidate = new FilePath(candidate, filename); - try { - if (candidate.exists()) { - return sourceDir; + + public Map choose(List filenames) { + try { + if (candidates.size() > 0) { + return root.act(new PathResolverChooseMultiCallable(candidates, filenames)); + } + } catch (IOException e) { + e.printStackTrace(); + } catch (InterruptedException e) { + e.printStackTrace(); + } + return Collections.emptyMap(); + } + + private static final class PathResolverChooseMultiCallable extends MasterToSlaveFileCallable> { + private final List candidates; + private final List filenames; + + private PathResolverChooseMultiCallable(List candidates, List filenames) { + this.candidates = candidates; + this.filenames = filenames; + } + + public Map invoke(File f, VirtualChannel channel) { + Map res = new HashMap(filenames.size()); + for (String filename : filenames) { + for (String sourceDir : candidates) { + File candidate = new File(f, sourceDir); + candidate = new File(candidate, filename); + if (candidate.exists()) { + res.put(filename, sourceDir); + } } - } catch (IOException e) { - e.printStackTrace(); - } catch (InterruptedException e) { - e.printStackTrace(); } + return res; } - return null; } } diff --git a/src/test/java/com/uber/jenkins/phabricator/coverage/CoberturaCoverageProviderTest.java b/src/test/java/com/uber/jenkins/phabricator/coverage/CoberturaCoverageProviderTest.java index 2baaca1c..e838862f 100644 --- a/src/test/java/com/uber/jenkins/phabricator/coverage/CoberturaCoverageProviderTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/coverage/CoberturaCoverageProviderTest.java @@ -19,6 +19,7 @@ import java.nio.file.Paths; import java.util.List; import java.util.Map; +import java.util.concurrent.TimeUnit; import javax.xml.parsers.ParserConfigurationException; @@ -106,9 +107,9 @@ public void testGetMetricsWithBuildActionResult() throws Exception { @Test public void testGetMetricsWithoutBuildActionResult() throws Exception { - FreeStyleBuild build = getBuild(); + FreeStyleBuild build = getExecutedBuild(); Path testCoverageFile = Paths.get(getClass().getResource(TEST_COVERAGE_FILE).toURI()); - FileUtils.copyFile(testCoverageFile.toFile(), new File(build.getRootDir(), "coverage.xml")); + FileUtils.copyFile(testCoverageFile.toFile(), new File(build.getWorkspace().getRemote(), "coverage.xml")); provider.setBuild(build); assertTrue(provider.hasCoverage()); @@ -119,33 +120,30 @@ public void testGetMetricsWithoutBuildActionResult() throws Exception { @Test public void testGetMetricsWithoutBuildActionResultDeletesFilesFromMasterAfter() throws Exception { - FreeStyleBuild build = getBuild(); + FreeStyleProject project = j.createFreeStyleProject(); + FreeStyleBuild remoteBuild = project.scheduleBuild2(0).get(100, TimeUnit.MINUTES); Path testCoverageFile = Paths.get(getClass().getResource(TEST_COVERAGE_FILE).toURI()); - File workspaceCopy = new File(build.getRootDir(), "coverage.xml"); - FileUtils.copyFile(testCoverageFile.toFile(), workspaceCopy); - provider.setBuild(build); + File cov = new File(remoteBuild.getWorkspace().getRemote(), "coverage.xml"); + FileUtils.copyFile(testCoverageFile.toFile(), cov); + provider.setBuild(remoteBuild); assertTrue(provider.hasCoverage()); CodeCoverageMetrics metrics = provider.getMetrics(); assertNotNull(metrics); - assertFalse(workspaceCopy.exists()); + assertFalse(new File(project.getLastBuild().getRootDir(), "coverage.xml").exists()); } @Test public void testGetLineCoverageNull() throws Exception { - FreeStyleProject project = j.createFreeStyleProject(); - FreeStyleBuild build = new FreeStyleBuild(project); - provider.setBuild(build); + provider.setBuild(getExecutedBuild()); assertNull(provider.readLineCoverage()); } @Test public void testGetLineCoverageWithFile() throws Exception { - FreeStyleProject project = j.createFreeStyleProject(); - FreeStyleBuild build = new FreeStyleBuild(project); + FreeStyleBuild build = getExecutedBuild(); - File coverageFile = new File(build.getRootDir(), "coverage0.xml"); - assertTrue(build.getRootDir().mkdirs()); + File coverageFile = new File(build.getWorkspace().getRemote(), "coverage0.xml"); InputStream in = getClass().getResourceAsStream("go-torch-coverage.xml"); OutputStream out = new BufferedOutputStream(new FileOutputStream(coverageFile)); IOUtils.copy(in, out); @@ -189,4 +187,8 @@ private CoverageResult getMockResult() { private FreeStyleBuild getBuild() throws IOException { return new FreeStyleBuild(j.createFreeStyleProject()); } + + private FreeStyleBuild getExecutedBuild() throws Exception { + return j.createFreeStyleProject().scheduleBuild2(0).get(100, TimeUnit.MINUTES); + } } diff --git a/src/test/java/com/uber/jenkins/phabricator/coverage/PathResolverTest.java b/src/test/java/com/uber/jenkins/phabricator/coverage/PathResolverTest.java index e6a85f40..0ed4a513 100644 --- a/src/test/java/com/uber/jenkins/phabricator/coverage/PathResolverTest.java +++ b/src/test/java/com/uber/jenkins/phabricator/coverage/PathResolverTest.java @@ -20,7 +20,9 @@ package com.uber.jenkins.phabricator.coverage; +import com.google.common.collect.ImmutableMap; import com.google.common.io.Files; + import hudson.FilePath; import org.junit.After; import org.junit.Before; @@ -36,11 +38,15 @@ @RunWith(Parameterized.class) public class PathResolverTest { public List candidates; + public List filenames; + public Map expectedResult; private Stack cleanupPaths; - public PathResolverTest(List candidates) { + public PathResolverTest(List candidates, List filenames, Map expectedResult) { this.candidates = candidates; + this.filenames = filenames; + this.expectedResult = expectedResult; } @Test @@ -62,12 +68,10 @@ public void testChoose() throws Exception { cleanupPaths.push(file); } - String chosen = new PathResolver(new FilePath(tmpDir), dirs).choose("dir/file"); + Map chosen = new PathResolver(new FilePath(tmpDir), dirs).choose(filenames); - if (candidates.isEmpty()) { - assertNull(chosen); - } else { - assertEquals("workspace/", chosen); + for (String filename : filenames) { + assertEquals(expectedResult.get(filename), chosen.get(filename)); } } @@ -86,9 +90,10 @@ public void tearDown() { @Parameterized.Parameters public static Collection data() { Collection result = new ArrayList(); - result.add(new Object[] { Collections.emptyList() }); - result.add(new Object[] { Arrays.asList("workspace/", "workspace/dir/", "workspace/dir/file") }); - result.add(new Object[] { Arrays.asList("workspace/", "workspace/dir/", "workspace/dir/dir/", "workspace/dir/file") }); + result.add(new Object[] { Collections.emptyList(), Arrays.asList("dir/file"), ImmutableMap.of()}); + result.add(new Object[] { Arrays.asList("workspace/", "workspace/dir/", "workspace/dir/file"), Arrays.asList("dir/file"), ImmutableMap.of("dir/file", "workspace/") }); + result.add(new Object[] { Arrays.asList("workspace/", "workspace/dir/", "workspace/dir/dir/", "workspace/dir/file"), Arrays.asList("dir/file"), ImmutableMap.of("dir/file", "workspace/")}); + result.add(new Object[] { Arrays.asList("workspace/", "workspace/dir/", "workspace/dir/dir/", "workspace/dir/file", "workspace2/dir/", "workspace2/dir/file2"), Arrays.asList("dir/file", "file2"), ImmutableMap.of("dir/file", "workspace/", "file2", "workspace2/dir/") }); return result; } }