Skip instrumentation for external pdb with no local sources (#538)

MarcoRossignoli · web-flow · commit 19b961130462 · 2019-09-13T14:50:55.000+02:00
Skip instrumentation for external pdb with no local sources
diff --git a/src/coverlet.core/Abstracts/IFileSystem.cs b/src/coverlet.core/Abstracts/IFileSystem.cs
@@ -0,0 +1,7 @@
+﻿namespace Coverlet.Core.Abstracts
+{
+    internal interface IFileSystem
+    {
+        bool Exists(string path);
+    }
+}
diff --git a/src/coverlet.core/Abstracts/IInstrumentationHelper.cs b/src/coverlet.core/Abstracts/IInstrumentationHelper.cs
@@ -12,7 +12,8 @@ public interface IInstrumentationHelper
         bool IsTypeExcluded(string module, string type, string[] excludeFilters);
         bool IsTypeIncluded(string module, string type, string[] includeFilters);
         void RestoreOriginalModule(string module, string identifier);
-        bool EmbeddedPortablePdbHasLocalSource(string module);
+        bool EmbeddedPortablePdbHasLocalSource(string module, out string firstNotFoundDocument);
+        bool PortablePdbHasLocalSource(string module, out string firstNotFoundDocument);
         bool IsLocalMethod(string method);
     }
 }
diff --git a/src/coverlet.core/DependencyInjection.cs b/src/coverlet.core/DependencyInjection.cs
@@ -28,6 +28,7 @@ private static IServiceProvider InitDefaultServices()
             IServiceCollection serviceCollection = new ServiceCollection();
             serviceCollection.AddTransient<IRetryHelper, RetryHelper>();
             serviceCollection.AddTransient<IProcessExitHandler, ProcessExitHandler>();
+            serviceCollection.AddTransient<IFileSystem, FileSystem>();
 
             // We need to keep singleton/static semantics
             serviceCollection.AddSingleton<IInstrumentationHelper, InstrumentationHelper>();
diff --git a/src/coverlet.core/Helpers/FileSystem.cs b/src/coverlet.core/Helpers/FileSystem.cs
@@ -0,0 +1,13 @@
+﻿using Coverlet.Core.Abstracts;
+using System.IO;
+
+namespace Coverlet.Core.Helpers
+{
+    internal class FileSystem : IFileSystem
+    {
+        public bool Exists(string path)
+        {
+            return File.Exists(path);
+        }
+    }
+}
diff --git a/src/coverlet.core/Helpers/InstrumentationHelper.cs b/src/coverlet.core/Helpers/InstrumentationHelper.cs
@@ -9,18 +9,21 @@
 using System.Reflection.PortableExecutable;
 using System.Text.RegularExpressions;
 using Coverlet.Core.Abstracts;
+using Coverlet.Core.Logging;
 
 namespace Coverlet.Core.Helpers
 {
     internal class InstrumentationHelper : IInstrumentationHelper
     {
         private readonly ConcurrentDictionary<string, string> _backupList = new ConcurrentDictionary<string, string>();
         private readonly IRetryHelper _retryHelper;
+        private readonly IFileSystem _fileSystem;
 
-        public InstrumentationHelper(IProcessExitHandler processExitHandler, IRetryHelper retryHelper)
+        public InstrumentationHelper(IProcessExitHandler processExitHandler, IRetryHelper retryHelper, IFileSystem fileSystem)
         {
             processExitHandler.Add((s, e) => RestoreOriginalModules());
             _retryHelper = retryHelper;
+            _fileSystem = fileSystem;
         }
 
         public string[] GetCoverableModules(string module, string[] directories, bool includeTestAssembly)
@@ -93,8 +96,9 @@ public bool HasPdb(string module, out bool embedded)
             }
         }
 
-        public bool EmbeddedPortablePdbHasLocalSource(string module)
+        public bool EmbeddedPortablePdbHasLocalSource(string module, out string firstNotFoundDocument)
         {
+            firstNotFoundDocument = "";
             using (FileStream moduleStream = File.OpenRead(module))
             using (var peReader = new PEReader(moduleStream))
             {
@@ -115,6 +119,7 @@ public bool EmbeddedPortablePdbHasLocalSource(string module)
                                 // Btw check for all possible extension could be weak approach
                                 if (!File.Exists(docName))
                                 {
+                                    firstNotFoundDocument = docName;
                                     return false;
                                 }
                             }
@@ -128,6 +133,41 @@ public bool EmbeddedPortablePdbHasLocalSource(string module)
             return true;
         }
 
+        public bool PortablePdbHasLocalSource(string module, out string firstNotFoundDocument)
+        {
+            firstNotFoundDocument = "";
+            using (var moduleStream = File.OpenRead(module))
+            using (var peReader = new PEReader(moduleStream))
+            {
+                foreach (var entry in peReader.ReadDebugDirectory())
+                {
+                    if (entry.Type == DebugDirectoryEntryType.CodeView)
+                    {
+                        var codeViewData = peReader.ReadCodeViewDebugDirectoryData(entry);
+                        using FileStream pdbStream = new FileStream(codeViewData.Path, FileMode.Open);
+                        using MetadataReaderProvider metadataReaderProvider = MetadataReaderProvider.FromPortablePdbStream(pdbStream);
+                        MetadataReader metadataReader = metadataReaderProvider.GetMetadataReader();
+                        foreach (DocumentHandle docHandle in metadataReader.Documents)
+                        {
+                            Document document = metadataReader.GetDocument(docHandle);
+                            string docName = metadataReader.GetString(document.Name);
+
+                            // We verify all docs and return false if not all are present in local
+                            // We could have false negative if doc is not a source
+                            // Btw check for all possible extension could be weak approach
+                            if (!_fileSystem.Exists(docName))
+                            {
+                                firstNotFoundDocument = docName;
+                                return false;
+                            }
+                        }
+                    }
+                }
+            }
+
+            return true;
+        }
+
         public void BackupOriginalModule(string module, string identifier)
         {
             var backupPath = GetBackupPath(module, identifier);
diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs
@@ -40,13 +40,13 @@ internal class Instrumenter
         private List<string> _excludedSourceFiles;
 
         public Instrumenter(
-            string module, 
-            string identifier, 
-            string[] excludeFilters, 
-            string[] includeFilters, 
-            string[] excludedFiles, 
-            string[] excludedAttributes, 
-            bool singleHit, 
+            string module,
+            string identifier,
+            string[] excludeFilters,
+            string[] includeFilters,
+            string[] excludedFiles,
+            string[] excludedAttributes,
+            bool singleHit,
             ILogger logger,
             IInstrumentationHelper instrumentationHelper)
         {
@@ -70,19 +70,27 @@ public bool CanInstrument()
                 {
                     if (embeddedPdb)
                     {
-                        if (_instrumentationHelper.EmbeddedPortablePdbHasLocalSource(_module))
+                        if (_instrumentationHelper.EmbeddedPortablePdbHasLocalSource(_module, out string firstNotFoundDocument))
                         {
                             return true;
                         }
                         else
                         {
-                            _logger.LogWarning($"Unable to instrument module: {_module}, embedded pdb without local source files");
+                            _logger.LogWarning($"Unable to instrument module: {_module}, embedded pdb without local source files, [{firstNotFoundDocument}]");
                             return false;
                         }
                     }
                     else
                     {
-                        return true;
+                        if (_instrumentationHelper.PortablePdbHasLocalSource(_module, out string firstNotFoundDocument))
+                        {
+                            return true;
+                        }
+                        else
+                        {
+                            _logger.LogWarning($"Unable to instrument module: {_module}, pdb without local source files, [{firstNotFoundDocument}]");
+                            return false;
+                        }
                     }
                 }
                 else
diff --git a/src/coverlet.core/Properties/AssemblyInfo.cs b/src/coverlet.core/Properties/AssemblyInfo.cs
@@ -4,4 +4,6 @@
 "2e5bb58a827a3c46c2f959318327ad30d1b52e918321ffbd847fb21565b8576d2a3a24562a93e8" +
 "6c77a298b564a0f1b98f63d7a1441a3a8bcc206da3ed09d5dacc76e122a109a9d3ac608e21a054" +
 "d667a2bae98510a1f0f653c0e6f58f42b4b3934f6012f5ec4a09b3dfd3e14d437ede1424bdb722" +
-"aead64ad")]
+"aead64ad")]
+// Needed to mock internal type https://github.com/Moq/moq4/wiki/Quickstart#advanced-features
+[assembly: System.Runtime.CompilerServices.InternalsVisibleTo("DynamicProxyGenAssembly2, PublicKey=0024000004800000940000000602000000240000525341310004000001000100c547cac37abd99c8db225ef2f6c8a3602f3b3606cc9891605d02baa56104f4cfc0734aa39b93bf7852f7d9266654753cc297e7d2edfe0bac1cdcf9f717241550e0a7b191195b7667bb4f64bcb8e2121380fd1d9d46ad2d92d2d15605093924cceaf74c4861eff62abf69b9291ed0a340e113be11e6a7d3113e92484cf7045cc7")]
diff --git a/test/coverlet.core.tests/CoverageTests.cs b/test/coverlet.core.tests/CoverageTests.cs
@@ -14,7 +14,7 @@ namespace Coverlet.Core.Tests
 {
     public class CoverageTests
     {
-        private readonly InstrumentationHelper _instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper());
+        private readonly InstrumentationHelper _instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), new FileSystem());
         private readonly Mock<ILogger> _mockLogger = new Mock<ILogger>();
 
         [Fact]
diff --git a/test/coverlet.core.tests/Helpers/InstrumentationHelperTests.cs b/test/coverlet.core.tests/Helpers/InstrumentationHelperTests.cs
@@ -8,7 +8,7 @@ namespace Coverlet.Core.Helpers.Tests
 {
     public class InstrumentationHelperTests
     {
-        private readonly InstrumentationHelper _instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper());
+        private readonly InstrumentationHelper _instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), new FileSystem());
 
         [Fact]
         public void TestGetDependencies()
diff --git a/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs b/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs
@@ -5,6 +5,7 @@
 using System.Reflection;
 using System.Runtime.InteropServices;
 
+using Coverlet.Core.Abstracts;
 using Coverlet.Core.Helpers;
 using Coverlet.Core.Logging;
 using Coverlet.Core.Samples.Tests;
@@ -20,7 +21,7 @@ namespace Coverlet.Core.Instrumentation.Tests
 {
     public class InstrumenterTests
     {
-        private readonly InstrumentationHelper _instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper());
+        private readonly InstrumentationHelper _instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), new FileSystem());
         private readonly Mock<ILogger> _mockLogger = new Mock<ILogger>();
 
         [Fact(Skip = "To be used only validating System.Private.CoreLib instrumentation")]
@@ -363,6 +364,25 @@ public void SkipEmbeddedPpdbWithoutLocalSource()
             loggerMock.VerifyNoOtherCalls();
         }
 
+        [Fact]
+        public void SkipPpdbWithoutLocalSource()
+        {
+            Mock<IFileSystem> mockFileSystem = new Mock<IFileSystem>();
+            mockFileSystem.Setup(fs => fs.Exists(It.IsAny<string>())).Returns((string path) =>
+             {
+                 return Path.GetExtension(path) != ".cs";
+             });
+            InstrumentationHelper instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), mockFileSystem.Object);
+
+            string coverletLib = Directory.GetFiles(Directory.GetCurrentDirectory(), "coverlet.core.dll").First();
+            var loggerMock = new Mock<ILogger>();
+            Instrumenter instrumenter = new Instrumenter(coverletLib, "_corelib_instrumented", Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), Array.Empty<string>(), false, loggerMock.Object, instrumentationHelper);
+            Assert.True(_instrumentationHelper.HasPdb(coverletLib, out bool embedded));
+            Assert.False(embedded);
+            Assert.False(instrumenter.CanInstrument());
+            loggerMock.Verify(l => l.LogWarning(It.IsAny<string>()));
+        }
+
         [Fact]
         public void TestInstrument_MissingModule()
         {
diff --git a/test/coverlet.core.tests/InstrumenterHelper.cs b/test/coverlet.core.tests/InstrumenterHelper.cs
@@ -191,7 +191,7 @@ public static CoverageResult GetCoverageResult(string filePath)
             using (var result = new FileStream(filePath, FileMode.Open))
             {
                 CoveragePrepareResult coveragePrepareResultLoaded = CoveragePrepareResult.Deserialize(result);
-                Coverage coverage = new Coverage(coveragePrepareResultLoaded, new Mock<ILogger>().Object, new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper()));
+                Coverage coverage = new Coverage(coveragePrepareResultLoaded, new Mock<ILogger>().Object, new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), new FileSystem()));
                 return coverage.GetCoverageResult();
             }
         }
@@ -202,6 +202,7 @@ async public static Task<CoveragePrepareResult> Run<T>(Func<dynamic, Task> callM
             DependencyInjection.Set(new ServiceCollection()
             .AddTransient<IRetryHelper, CustomRetryHelper>()
             .AddTransient<IProcessExitHandler, CustomProcessExitHandler>()
+            .AddTransient<IFileSystem, FileSystem>()
             .AddSingleton<IInstrumentationHelper, InstrumentationHelper>()
             .BuildServiceProvider());
 

Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,8 @@ public interface IInstrumentationHelper`
`12`	`12`	`bool IsTypeExcluded(string module, string type, string[] excludeFilters);`
`13`	`13`	`bool IsTypeIncluded(string module, string type, string[] includeFilters);`
`14`	`14`	`void RestoreOriginalModule(string module, string identifier);`
`15`		`- bool EmbeddedPortablePdbHasLocalSource(string module);`
	`15`	`+ bool EmbeddedPortablePdbHasLocalSource(string module, out string firstNotFoundDocument);`
	`16`	`+ bool PortablePdbHasLocalSource(string module, out string firstNotFoundDocument);`
`16`	`17`	`bool IsLocalMethod(string method);`
`17`	`18`	`}`
`18`	`19`	`}`
Original file line number	Diff line number	Diff line change
`@@ -40,13 +40,13 @@ internal class Instrumenter`
`40`	`40`	`private List<string> _excludedSourceFiles;`
`41`	`41`
`42`	`42`	`public Instrumenter(`
`43`		`- string module,`
`44`		`- string identifier,`
`45`		`- string[] excludeFilters,`
`46`		`- string[] includeFilters,`
`47`		`- string[] excludedFiles,`
`48`		`- string[] excludedAttributes,`
`49`		`- bool singleHit,`
	`43`	`+ string module,`
	`44`	`+ string identifier,`
	`45`	`+ string[] excludeFilters,`
	`46`	`+ string[] includeFilters,`
	`47`	`+ string[] excludedFiles,`
	`48`	`+ string[] excludedAttributes,`
	`49`	`+ bool singleHit,`
`50`	`50`	`ILogger logger,`
`51`	`51`	`IInstrumentationHelper instrumentationHelper)`
`52`	`52`	`{`
`@@ -70,19 +70,27 @@ public bool CanInstrument()`
`70`	`70`	`{`
`71`	`71`	`if (embeddedPdb)`
`72`	`72`	`{`
`73`		`- if (_instrumentationHelper.EmbeddedPortablePdbHasLocalSource(_module))`
	`73`	`+ if (_instrumentationHelper.EmbeddedPortablePdbHasLocalSource(_module, out string firstNotFoundDocument))`
`74`	`74`	`{`
`75`	`75`	`return true;`
`76`	`76`	`}`
`77`	`77`	`else`
`78`	`78`	`{`
`79`		`- _logger.LogWarning($"Unable to instrument module: {_module}, embedded pdb without local source files");`
	`79`	`+ _logger.LogWarning($"Unable to instrument module: {_module}, embedded pdb without local source files, [{firstNotFoundDocument}]");`
`80`	`80`	`return false;`
`81`	`81`	`}`
`82`	`82`	`}`
`83`	`83`	`else`
`84`	`84`	`{`
`85`		`- return true;`
	`85`	`+ if (_instrumentationHelper.PortablePdbHasLocalSource(_module, out string firstNotFoundDocument))`
	`86`	`+ {`
	`87`	`+ return true;`
	`88`	`+ }`
	`89`	`+ else`
	`90`	`+ {`
	`91`	`+ _logger.LogWarning($"Unable to instrument module: {_module}, pdb without local source files, [{firstNotFoundDocument}]");`
	`92`	`+ return false;`
	`93`	`+ }`
`86`	`94`	`}`
`87`	`95`	`}`
`88`	`96`	`else`
Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,7 @@ namespace Coverlet.Core.Tests`
`14`	`14`	`{`
`15`	`15`	`public class CoverageTests`
`16`	`16`	`{`
`17`		`- private readonly InstrumentationHelper _instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper());`
	`17`	`+ private readonly InstrumentationHelper _instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), new FileSystem());`
`18`	`18`	`private readonly Mock<ILogger> _mockLogger = new Mock<ILogger>();`
`19`	`19`
`20`	`20`	`[Fact]`
Original file line number	Diff line number	Diff line change
`@@ -8,7 +8,7 @@ namespace Coverlet.Core.Helpers.Tests`
`8`	`8`	`{`
`9`	`9`	`public class InstrumentationHelperTests`
`10`	`10`	`{`
`11`		`- private readonly InstrumentationHelper _instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper());`
	`11`	`+ private readonly InstrumentationHelper _instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), new FileSystem());`
`12`	`12`
`13`	`13`	`[Fact]`
`14`	`14`	`public void TestGetDependencies()`