Fix deterministic build+source link bug (#895)

MarcoRossignoli · web-flow · commit 0c85839c9474 · 2020-07-11T17:11:30.000+02:00
Fix deterministic build+source link bug
diff --git a/Documentation/Changelog.md b/Documentation/Changelog.md
@@ -7,7 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## [Unreleased]
 
 ### Fixed
--Attribute exclusion does not work if attribute name does not end with "Attribute" [883](https://github.com/coverlet-coverage/coverlet/issues/883) by https://github.com/bddckr
+-Attribute exclusion does not work if attribute name does not end with "Attribute" [884](https://github.com/coverlet-coverage/coverlet/pull/884) by https://github.com/bddckr  
+-Fix deterministic build+source link bug [895](https://github.com/coverlet-coverage/coverlet/pull/895)
 
 ## Release date 2020-05-30
 ### Packages  
diff --git a/src/coverlet.core/Abstractions/ISourceRootTranslator.cs b/src/coverlet.core/Abstractions/ISourceRootTranslator.cs
@@ -1,7 +1,11 @@
-﻿namespace Coverlet.Core.Abstractions
+﻿using System.Collections.Generic;
+using Coverlet.Core.Helpers;
+
+namespace Coverlet.Core.Abstractions
 {
     internal interface ISourceRootTranslator
     {
         string ResolveFilePath(string originalFileName);
+        IReadOnlyList<SourceRootMapping> ResolvePathRoot(string pathRoot);
     }
 }
diff --git a/src/coverlet.core/Coverage.cs b/src/coverlet.core/Coverage.cs
@@ -3,6 +3,7 @@
 using System.IO;
 using System.Linq;
 using Coverlet.Core.Abstractions;
+using Coverlet.Core.Helpers;
 using Coverlet.Core.Instrumentation;
 
 using Newtonsoft.Json;
@@ -77,7 +78,11 @@ public Coverage(string module,
             _results = new List<InstrumenterResult>();
         }
 
-        public Coverage(CoveragePrepareResult prepareResult, ILogger logger, IInstrumentationHelper instrumentationHelper, IFileSystem fileSystem)
+        public Coverage(CoveragePrepareResult prepareResult,
+                        ILogger logger,
+                        IInstrumentationHelper instrumentationHelper,
+                        IFileSystem fileSystem,
+                        ISourceRootTranslator sourceRootTranslator)
         {
             _identifier = prepareResult.Identifier;
             _module = prepareResult.Module;
@@ -87,6 +92,7 @@ public Coverage(CoveragePrepareResult prepareResult, ILogger logger, IInstrument
             _logger = logger;
             _instrumentationHelper = instrumentationHelper;
             _fileSystem = fileSystem;
+            _sourceRootTranslator = sourceRootTranslator;
         }
 
         public CoveragePrepareResult PrepareModules()
@@ -429,29 +435,33 @@ private string GetSourceLinkUrl(Dictionary<string, string> sourceLinkDocuments,
                 string key = sourceLinkDocument.Key;
                 if (Path.GetFileName(key) != "*") continue;
 
-                string directoryDocument = Path.GetDirectoryName(document);
-                string sourceLinkRoot = Path.GetDirectoryName(key);
-                string relativePath = "";
-
-                // if document is on repo root we skip relative path calculation
-                if (directoryDocument != sourceLinkRoot)
+                IReadOnlyList<SourceRootMapping> rootMapping = _sourceRootTranslator.ResolvePathRoot(key.Substring(0, key.Length - 1));
+                foreach (string keyMapping in rootMapping is null ? new List<string>() { key } : new List<string>(rootMapping.Select(m => m.OriginalPath)))
                 {
-                    if (!directoryDocument.StartsWith(sourceLinkRoot + Path.DirectorySeparatorChar))
-                        continue;
+                    string directoryDocument = Path.GetDirectoryName(document);
+                    string sourceLinkRoot = Path.GetDirectoryName(keyMapping);
+                    string relativePath = "";
 
-                    relativePath = directoryDocument.Substring(sourceLinkRoot.Length + 1);
-                }
+                    // if document is on repo root we skip relative path calculation
+                    if (directoryDocument != sourceLinkRoot)
+                    {
+                        if (!directoryDocument.StartsWith(sourceLinkRoot + Path.DirectorySeparatorChar))
+                            continue;
 
-                if (relativePathOfBestMatch.Length == 0)
-                {
-                    keyWithBestMatch = sourceLinkDocument.Key;
-                    relativePathOfBestMatch = relativePath;
-                }
+                        relativePath = directoryDocument.Substring(sourceLinkRoot.Length + 1);
+                    }
 
-                if (relativePath.Length < relativePathOfBestMatch.Length)
-                {
-                    keyWithBestMatch = sourceLinkDocument.Key;
-                    relativePathOfBestMatch = relativePath;
+                    if (relativePathOfBestMatch.Length == 0)
+                    {
+                        keyWithBestMatch = sourceLinkDocument.Key;
+                        relativePathOfBestMatch = relativePath;
+                    }
+
+                    if (relativePath.Length < relativePathOfBestMatch.Length)
+                    {
+                        keyWithBestMatch = sourceLinkDocument.Key;
+                        relativePathOfBestMatch = relativePath;
+                    }
                 }
             }
 
diff --git a/src/coverlet.core/Helpers/SourceRootTranslator.cs b/src/coverlet.core/Helpers/SourceRootTranslator.cs
@@ -19,7 +19,7 @@ internal class SourceRootTranslator : ISourceRootTranslator
         private readonly IFileSystem _fileSystem;
         private readonly Dictionary<string, List<SourceRootMapping>> _sourceRootMapping;
         private const string MappingFileName = "CoverletSourceRootsMapping";
-        private Dictionary<string, string> _resolutionCache;
+        private Dictionary<string, string> _resolutionCacheFiles;
 
         public SourceRootTranslator(ILogger logger, IFileSystem fileSystem)
         {
@@ -76,11 +76,16 @@ private Dictionary<string, List<SourceRootMapping>> LoadSourceRootMapping(string
             return mapping;
         }
 
+        public IReadOnlyList<SourceRootMapping> ResolvePathRoot(string pathRoot)
+        {
+            return _sourceRootMapping.TryGetValue(pathRoot, out List<SourceRootMapping> sourceRootMapping) ? sourceRootMapping.AsReadOnly() : null;
+        }
+
         public string ResolveFilePath(string originalFileName)
         {
-            if (_resolutionCache != null && _resolutionCache.ContainsKey(originalFileName))
+            if (_resolutionCacheFiles != null && _resolutionCacheFiles.ContainsKey(originalFileName))
             {
-                return _resolutionCache[originalFileName];
+                return _resolutionCacheFiles[originalFileName];
             }
 
             foreach (KeyValuePair<string, List<SourceRootMapping>> mapping in _sourceRootMapping)
@@ -92,7 +97,7 @@ public string ResolveFilePath(string originalFileName)
                         string pathToCheck;
                         if (_fileSystem.Exists(pathToCheck = Path.GetFullPath(originalFileName.Replace(mapping.Key, srm.OriginalPath))))
                         {
-                            (_resolutionCache ??= new Dictionary<string, string>()).Add(originalFileName, pathToCheck);
+                            (_resolutionCacheFiles ??= new Dictionary<string, string>()).Add(originalFileName, pathToCheck);
                             _logger.LogVerbose($"Mapping resolved: '{originalFileName}' -> '{pathToCheck}'");
                             return pathToCheck;
                         }
diff --git a/src/coverlet.msbuild.tasks/CoverageResultTask.cs b/src/coverlet.msbuild.tasks/CoverageResultTask.cs
@@ -96,7 +96,7 @@ public override bool Execute()
                     // Task.Log is teared down after a task and thus the new MSBuildLogger must be passed to the InstrumentationHelper
                     // https://github.com/microsoft/msbuild/issues/5153
                     instrumentationHelper.SetLogger(_logger);
-                    coverage = new Coverage(CoveragePrepareResult.Deserialize(instrumenterStateStream), this._logger, ServiceProvider.GetService<IInstrumentationHelper>(), fileSystem);
+                    coverage = new Coverage(CoveragePrepareResult.Deserialize(instrumenterStateStream), this._logger, ServiceProvider.GetService<IInstrumentationHelper>(), fileSystem, ServiceProvider.GetService<ISourceRootTranslator>());
                 }
 
                 try
diff --git a/test/coverlet.core.tests/Coverage/InstrumenterHelper.cs b/test/coverlet.core.tests/Coverage/InstrumenterHelper.cs
@@ -63,7 +63,7 @@ public static CoverageResult GetCoverageResult(string filePath)
             });
             _processWideContainer.GetRequiredService<IInstrumentationHelper>().SetLogger(logger.Object);
             CoveragePrepareResult coveragePrepareResultLoaded = CoveragePrepareResult.Deserialize(result);
-            Coverage coverage = new Coverage(coveragePrepareResultLoaded, logger.Object, _processWideContainer.GetService<IInstrumentationHelper>(), new FileSystem());
+            Coverage coverage = new Coverage(coveragePrepareResultLoaded, logger.Object, _processWideContainer.GetService<IInstrumentationHelper>(), new FileSystem(), new SourceRootTranslator(new Mock<ILogger>().Object, new FileSystem()));
             return coverage.GetCoverageResult();
         }
 
diff --git a/test/coverlet.core.tests/Helpers/SourceRootTranslatorTests.cs b/test/coverlet.core.tests/Helpers/SourceRootTranslatorTests.cs
@@ -28,6 +28,23 @@ public void Translate_Success()
             Assert.Equal(@"C:\git\coverlet\src\coverlet.core\obj\Debug\netstandard2.0\coverlet.core.pdb", translator.ResolveFilePath(fileToTranslate));
         }
 
+        [ConditionalFact]
+        [SkipOnOS(OS.Linux, "Windows path format only")]
+        [SkipOnOS(OS.MacOS, "Windows path format only")]
+        public void TranslatePathRoot_Success()
+        {
+            Mock<ILogger> logger = new Mock<ILogger>();
+            Mock<IFileSystem> fileSystem = new Mock<IFileSystem>();
+            fileSystem.Setup(f => f.Exists(It.IsAny<string>())).Returns((string p) =>
+            {
+                if (p == "testLib.dll" || p == @"C:\git\coverlet\src\coverlet.core\obj\Debug\netstandard2.0\coverlet.core.pdb" || p == "CoverletSourceRootsMapping") return true;
+                return false;
+            });
+            fileSystem.Setup(f => f.ReadAllLines(It.IsAny<string>())).Returns(File.ReadAllLines(@"TestAssets/CoverletSourceRootsMappingTest"));
+            SourceRootTranslator translator = new SourceRootTranslator("testLib.dll", logger.Object, fileSystem.Object);
+            Assert.Equal(@"C:\git\coverlet\", translator.ResolvePathRoot("/_/")[0].OriginalPath);
+        }
+
         [Fact]
         public void Translate_EmptyFile()
         {
diff --git a/test/coverlet.integration.tests/BaseTest.cs b/test/coverlet.integration.tests/BaseTest.cs
@@ -200,7 +200,7 @@ private protected void AddCoverletCollectosRef(string projectPath)
             xml.Save(csproj);
         }
 
-        private protected string AddCollectorRunsettingsFile(string projectPath, string includeFilter = "[coverletsamplelib.integration.template]*DeepThought")
+        private protected string AddCollectorRunsettingsFile(string projectPath, string includeFilter = "[coverletsamplelib.integration.template]*DeepThought", bool sourceLink = false)
         {
             string runSettings =
 $@"<?xml version=""1.0"" encoding=""utf-8"" ?>
@@ -211,6 +211,7 @@ private protected string AddCollectorRunsettingsFile(string projectPath, string
            <Configuration>
             <Format>json,cobertura</Format>
             <Include>{includeFilter}</Include>
+            <UseSourceLink>{(sourceLink ? "true" : "false")}</UseSourceLink>
             <!-- We need to include test assembly because test and code to cover are in same template project -->
             <IncludeTestAssembly>true</IncludeTestAssembly>
         </Configuration>
diff --git a/test/coverlet.integration.tests/DeterministicBuild.cs b/test/coverlet.integration.tests/DeterministicBuild.cs
@@ -66,7 +66,7 @@ public void Msbuild()
             string sourceRootMappingFilePath = Path.Combine(_testProjectPath, "bin", _buildConfiguration, _testProjectTfm!, "CoverletSourceRootsMapping");
             Assert.True(File.Exists(sourceRootMappingFilePath), sourceRootMappingFilePath);
             Assert.True(!string.IsNullOrEmpty(File.ReadAllText(sourceRootMappingFilePath)), "Empty CoverletSourceRootsMapping file");
-            Assert.Equal(2, File.ReadAllLines(sourceRootMappingFilePath).Length);
+            Assert.Contains("=/_/", File.ReadAllText(sourceRootMappingFilePath));
 
             DotnetCli($"test -c {_buildConfiguration} --no-build /p:CollectCoverage=true /p:Include=\"[coverletsample.integration.determisticbuild]*DeepThought\" /p:IncludeTestAssembly=true", out standardOutput, out _, _testProjectPath);
             Assert.Contains("Test Run Successful.", standardOutput);
@@ -80,6 +80,30 @@ public void Msbuild()
             RunCommand("git", "clean -fdx", out _, out _, _testProjectPath);
         }
 
+        [Fact]
+        public void Msbuild_SourceLink()
+        {
+            CreateDeterministicTestPropsFile();
+            DotnetCli($"build -c {_buildConfiguration} /p:DeterministicSourcePaths=true", out string standardOutput, out string _, _testProjectPath);
+            Assert.Contains("Build succeeded.", standardOutput);
+            string sourceRootMappingFilePath = Path.Combine(_testProjectPath, "bin", _buildConfiguration, _testProjectTfm!, "CoverletSourceRootsMapping");
+            Assert.True(File.Exists(sourceRootMappingFilePath), sourceRootMappingFilePath);
+            Assert.True(!string.IsNullOrEmpty(File.ReadAllText(sourceRootMappingFilePath)), "Empty CoverletSourceRootsMapping file");
+            Assert.Contains("=/_/", File.ReadAllText(sourceRootMappingFilePath));
+
+            DotnetCli($"test -c {_buildConfiguration} --no-build /p:CollectCoverage=true /p:UseSourceLink=true /p:Include=\"[coverletsample.integration.determisticbuild]*DeepThought\" /p:IncludeTestAssembly=true", out standardOutput, out _, _testProjectPath);
+            Assert.Contains("Test Run Successful.", standardOutput);
+            Assert.Contains("| coverletsample.integration.determisticbuild | 100% | 100%   | 100%   |", standardOutput);
+            Assert.True(File.Exists(Path.Combine(_testProjectPath, "coverage.json")));
+            Assert.Contains("raw.githubusercontent.com", File.ReadAllText(Path.Combine(_testProjectPath, "coverage.json")));
+            AssertCoverage(standardOutput);
+
+            // Process exits hang on clean seem that process doesn't close, maybe some mbuild node reuse? btw manually tested
+            // DotnetCli("clean", out standardOutput, out standardError, _fixture.TestProjectPath);
+            // Assert.False(File.Exists(sourceRootMappingFilePath));
+            RunCommand("git", "clean -fdx", out _, out _, _testProjectPath);
+        }
+
         [Fact]
         public void Collectors()
         {
@@ -89,7 +113,7 @@ public void Collectors()
             string sourceRootMappingFilePath = Path.Combine(_testProjectPath, "bin", GetAssemblyBuildConfiguration().ToString(), _testProjectTfm!, "CoverletSourceRootsMapping");
             Assert.True(File.Exists(sourceRootMappingFilePath), sourceRootMappingFilePath);
             Assert.NotEmpty(File.ReadAllText(sourceRootMappingFilePath));
-            Assert.Equal(2, File.ReadAllLines(sourceRootMappingFilePath).Length);
+            Assert.Contains("=/_/", File.ReadAllText(sourceRootMappingFilePath));
 
             string runSettingsPath = AddCollectorRunsettingsFile(_testProjectPath, "[coverletsample.integration.determisticbuild]*DeepThought");
             Assert.True(DotnetCli($"test -c {_buildConfiguration} --no-build \"{_testProjectPath}\" --collect:\"XPlat Code Coverage\" --settings \"{runSettingsPath}\" --diag:{Path.Combine(_testProjectPath, "log.txt")}", out standardOutput, out _), standardOutput);
@@ -108,6 +132,35 @@ public void Collectors()
             RunCommand("git", "clean -fdx", out _, out _, _testProjectPath);
         }
 
+        [Fact]
+        public void Collectors_SourceLink()
+        {
+            CreateDeterministicTestPropsFile();
+            DotnetCli($"build -c {_buildConfiguration} /p:DeterministicSourcePaths=true", out string standardOutput, out string _, _testProjectPath);
+            Assert.Contains("Build succeeded.", standardOutput);
+            string sourceRootMappingFilePath = Path.Combine(_testProjectPath, "bin", GetAssemblyBuildConfiguration().ToString(), _testProjectTfm!, "CoverletSourceRootsMapping");
+            Assert.True(File.Exists(sourceRootMappingFilePath), sourceRootMappingFilePath);
+            Assert.NotEmpty(File.ReadAllText(sourceRootMappingFilePath));
+            Assert.Contains("=/_/", File.ReadAllText(sourceRootMappingFilePath));
+
+            string runSettingsPath = AddCollectorRunsettingsFile(_testProjectPath, "[coverletsample.integration.determisticbuild]*DeepThought", sourceLink: true);
+            Assert.True(DotnetCli($"test -c {_buildConfiguration} --no-build \"{_testProjectPath}\" --collect:\"XPlat Code Coverage\" --settings \"{runSettingsPath}\" --diag:{Path.Combine(_testProjectPath, "log.txt")}", out standardOutput, out _), standardOutput);
+            Assert.Contains("Test Run Successful.", standardOutput);
+            AssertCoverage(standardOutput);
+            Assert.Contains("raw.githubusercontent.com", File.ReadAllText(Directory.GetFiles(_testProjectPath, "coverage.cobertura.xml", SearchOption.AllDirectories).Single()));
+
+            // Check out/in process collectors injection
+            string dataCollectorLogContent = File.ReadAllText(Directory.GetFiles(_testProjectPath, "log.datacollector.*.txt").Single());
+            Assert.Contains("[coverlet]Initializing CoverletCoverageDataCollector with configuration:", dataCollectorLogContent);
+            Assert.Contains("[coverlet]Initialize CoverletInProcDataCollector", File.ReadAllText(Directory.GetFiles(_testProjectPath, "log.host.*.txt").Single()));
+            Assert.Contains("[coverlet]Mapping resolved", dataCollectorLogContent);
+
+            // Process exits hang on clean seem that process doesn't close, maybe some mbuild node reuse? btw manually tested
+            // DotnetCli("clean", out standardOutput, out standardError, _fixture.TestProjectPath);
+            // Assert.False(File.Exists(sourceRootMappingFilePath));
+            RunCommand("git", "clean -fdx", out _, out _, _testProjectPath);
+        }
+
         public void Dispose()
         {
             File.Delete(Path.Combine(_testProjectPath, PropsFileName));

Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,11 @@`
`1`		`-namespace Coverlet.Core.Abstractions`
	`1`	`+using System.Collections.Generic;`
	`2`	`+using Coverlet.Core.Helpers;`
	`3`	`+`
	`4`	`+namespace Coverlet.Core.Abstractions`
`2`	`5`	`{`
`3`	`6`	`internal interface ISourceRootTranslator`
`4`	`7`	`{`
`5`	`8`	`string ResolveFilePath(string originalFileName);`
	`9`	`+ IReadOnlyList<SourceRootMapping> ResolvePathRoot(string pathRoot);`
`6`	`10`	`}`
`7`	`11`	`}`
Original file line number	Diff line number	Diff line change
`@@ -19,7 +19,7 @@ internal class SourceRootTranslator : ISourceRootTranslator`
`19`	`19`	`private readonly IFileSystem _fileSystem;`
`20`	`20`	`private readonly Dictionary<string, List<SourceRootMapping>> _sourceRootMapping;`
`21`	`21`	`private const string MappingFileName = "CoverletSourceRootsMapping";`
`22`		`- private Dictionary<string, string> _resolutionCache;`
	`22`	`+ private Dictionary<string, string> _resolutionCacheFiles;`
`23`	`23`
`24`	`24`	`public SourceRootTranslator(ILogger logger, IFileSystem fileSystem)`
`25`	`25`	`{`
`@@ -76,11 +76,16 @@ private Dictionary<string, List<SourceRootMapping>> LoadSourceRootMapping(string`
`76`	`76`	`return mapping;`
`77`	`77`	`}`
`78`	`78`
	`79`	`+ public IReadOnlyList<SourceRootMapping> ResolvePathRoot(string pathRoot)`
	`80`	`+ {`
	`81`	`+ return _sourceRootMapping.TryGetValue(pathRoot, out List<SourceRootMapping> sourceRootMapping) ? sourceRootMapping.AsReadOnly() : null;`
	`82`	`+ }`
	`83`	`+`
`79`	`84`	`public string ResolveFilePath(string originalFileName)`
`80`	`85`	`{`
`81`		`- if (_resolutionCache != null && _resolutionCache.ContainsKey(originalFileName))`
	`86`	`+ if (_resolutionCacheFiles != null && _resolutionCacheFiles.ContainsKey(originalFileName))`
`82`	`87`	`{`
`83`		`- return _resolutionCache[originalFileName];`
	`88`	`+ return _resolutionCacheFiles[originalFileName];`
`84`	`89`	`}`
`85`	`90`
`86`	`91`	`foreach (KeyValuePair<string, List<SourceRootMapping>> mapping in _sourceRootMapping)`
`@@ -92,7 +97,7 @@ public string ResolveFilePath(string originalFileName)`
`92`	`97`	`string pathToCheck;`
`93`	`98`	`if (_fileSystem.Exists(pathToCheck = Path.GetFullPath(originalFileName.Replace(mapping.Key, srm.OriginalPath))))`
`94`	`99`	`{`
`95`		`- (_resolutionCache ??= new Dictionary<string, string>()).Add(originalFileName, pathToCheck);`
	`100`	`+ (_resolutionCacheFiles ??= new Dictionary<string, string>()).Add(originalFileName, pathToCheck);`
`96`	`101`	`_logger.LogVerbose($"Mapping resolved: '{originalFileName}' -> '{pathToCheck}'");`
`97`	`102`	`return pathToCheck;`
`98`	`103`	`}`
Original file line number	Diff line number	Diff line change
`@@ -96,7 +96,7 @@ public override bool Execute()`
`96`	`96`	`// Task.Log is teared down after a task and thus the new MSBuildLogger must be passed to the InstrumentationHelper`
`97`	`97`	`// https://github.com/microsoft/msbuild/issues/5153`
`98`	`98`	`instrumentationHelper.SetLogger(_logger);`
`99`		`- coverage = new Coverage(CoveragePrepareResult.Deserialize(instrumenterStateStream), this._logger, ServiceProvider.GetService<IInstrumentationHelper>(), fileSystem);`
	`99`	`+ coverage = new Coverage(CoveragePrepareResult.Deserialize(instrumenterStateStream), this._logger, ServiceProvider.GetService<IInstrumentationHelper>(), fileSystem, ServiceProvider.GetService<ISourceRootTranslator>());`
`100`	`100`	`}`
`101`	`101`
`102`	`102`	`try`
Original file line number	Diff line number	Diff line change
`@@ -63,7 +63,7 @@ public static CoverageResult GetCoverageResult(string filePath)`
`63`	`63`	`});`
`64`	`64`	`_processWideContainer.GetRequiredService<IInstrumentationHelper>().SetLogger(logger.Object);`
`65`	`65`	`CoveragePrepareResult coveragePrepareResultLoaded = CoveragePrepareResult.Deserialize(result);`
`66`		`- Coverage coverage = new Coverage(coveragePrepareResultLoaded, logger.Object, _processWideContainer.GetService<IInstrumentationHelper>(), new FileSystem());`
	`66`	`+ Coverage coverage = new Coverage(coveragePrepareResultLoaded, logger.Object, _processWideContainer.GetService<IInstrumentationHelper>(), new FileSystem(), new SourceRootTranslator(new Mock<ILogger>().Object, new FileSystem()));`
`67`	`67`	`return coverage.GetCoverageResult();`
`68`	`68`	`}`
`69`	`69`