From 76baf7e656e899c399ff8ff2953e3bdf2d15f39a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20M=C3=BCller?= Date: Tue, 18 May 2021 11:42:02 +0200 Subject: [PATCH 1/5] Skip fsharp construct with `unknown` source --- Documentation/Changelog.md | 1 + coverlet.sln | 7 +++++++ .../Helpers/InstrumentationHelper.cs | 20 ++++++++++++++----- .../Instrumentation/InstrumenterTests.cs | 18 +++++++++++++++++ .../coverlet.core.tests.csproj | 1 + .../Library.fs | 4 ++++ ...coverlet.tests.projectsample.fsharp.fsproj | 12 +++++++++++ 7 files changed, 58 insertions(+), 5 deletions(-) create mode 100644 test/coverlet.tests.projectsample.fsharp/Library.fs create mode 100644 test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj diff --git a/Documentation/Changelog.md b/Documentation/Changelog.md index 490162318..e72424527 100644 --- a/Documentation/Changelog.md +++ b/Documentation/Changelog.md @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +-Fix F# projects with `unkown` source [#1145](https://github.com/coverlet-coverage/coverlet/issues/1145) -Fix partially covered throw statement [#1144](https://github.com/coverlet-coverage/coverlet/pull/1144) -Fix coverage threshold not failing when no coverage [#1115](https://github.com/coverlet-coverage/coverlet/pull/1115) -Fix partially covered `await foreach` statement [#1107](https://github.com/coverlet-coverage/coverlet/pull/1107) by https://github.com/alexthornton1 diff --git a/coverlet.sln b/coverlet.sln index 17d712674..40dfd8966 100644 --- a/coverlet.sln +++ b/coverlet.sln @@ -53,6 +53,8 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Build", "Build", "{9A8B19D4 test\Directory.Build.targets = test\Directory.Build.targets EndProjectSection EndProject +Project("{F2A71F9B-5D33-465A-A702-920D77279786}") = "coverlet.tests.projectsample.fsharp", "test\coverlet.tests.projectsample.fsharp\coverlet.tests.projectsample.fsharp.fsproj", "{1CBF6966-2A67-4D2C-8598-D174B83072F4}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -115,6 +117,10 @@ Global {F8199E19-FA9A-4559-9101-CAD7028121B4}.Debug|Any CPU.Build.0 = Debug|Any CPU {F8199E19-FA9A-4559-9101-CAD7028121B4}.Release|Any CPU.ActiveCfg = Release|Any CPU {F8199E19-FA9A-4559-9101-CAD7028121B4}.Release|Any CPU.Build.0 = Release|Any CPU + {1CBF6966-2A67-4D2C-8598-D174B83072F4}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {1CBF6966-2A67-4D2C-8598-D174B83072F4}.Debug|Any CPU.Build.0 = Debug|Any CPU + {1CBF6966-2A67-4D2C-8598-D174B83072F4}.Release|Any CPU.ActiveCfg = Release|Any CPU + {1CBF6966-2A67-4D2C-8598-D174B83072F4}.Release|Any CPU.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE @@ -135,6 +141,7 @@ Global {5FF404AD-7C0B-465A-A1E9-558CDC642B0C} = {2FEBDE1B-83E3-445B-B9F8-5644B0E0E134} {F8199E19-FA9A-4559-9101-CAD7028121B4} = {2FEBDE1B-83E3-445B-B9F8-5644B0E0E134} {9A8B19D4-4A24-4217-AEFE-159B68F029A1} = {2FEBDE1B-83E3-445B-B9F8-5644B0E0E134} + {1CBF6966-2A67-4D2C-8598-D174B83072F4} = {2FEBDE1B-83E3-445B-B9F8-5644B0E0E134} EndGlobalSection GlobalSection(ExtensibilityGlobals) = postSolution SolutionGuid = {9CA57C02-97B0-4C38-A027-EA61E8741F10} diff --git a/src/coverlet.core/Helpers/InstrumentationHelper.cs b/src/coverlet.core/Helpers/InstrumentationHelper.cs index d48036317..69b6d5cde 100644 --- a/src/coverlet.core/Helpers/InstrumentationHelper.cs +++ b/src/coverlet.core/Helpers/InstrumentationHelper.cs @@ -121,12 +121,13 @@ public bool EmbeddedPortablePdbHasLocalSource(string module, out string firstNot { Document document = metadataReader.GetDocument(docHandle); string docName = _sourceRootTranslator.ResolveFilePath(metadataReader.GetString(document.Name)); - + Guid languageGuid = metadataReader.GetGuid(document.Language); // 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 - // We exlude from the check the autogenerated source file(i.e. source generators) - if (!_fileSystem.Exists(docName) && !docName.EndsWith(".g.cs")) + // We exclude from the check the autogenerated source file(i.e. source generators) + // We exclude special F# construct https://github.com/coverlet-coverage/coverlet/issues/1145 + if (!_fileSystem.Exists(docName) && !docName.EndsWith(".g.cs") && !IsUnknownModuleInFSharpAssembly(languageGuid, docName)) { firstNotFoundDocument = docName; return false; @@ -165,16 +166,18 @@ public bool PortablePdbHasLocalSource(string module, out string firstNotFoundDoc _logger.LogWarning($"{nameof(BadImageFormatException)} during MetadataReaderProvider.FromPortablePdbStream in InstrumentationHelper.PortablePdbHasLocalSource, unable to check if module has got local source."); return true; } + foreach (DocumentHandle docHandle in metadataReader.Documents) { Document document = metadataReader.GetDocument(docHandle); string docName = _sourceRootTranslator.ResolveFilePath(metadataReader.GetString(document.Name)); - + Guid languageGuid = metadataReader.GetGuid(document.Language); // 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 // We exlude from the check the autogenerated source file(i.e. source generators) - if (!_fileSystem.Exists(docName) && !docName.EndsWith(".g.cs")) + // We exclude special F# construct https://github.com/coverlet-coverage/coverlet/issues/1145 + if (!_fileSystem.Exists(docName) && !docName.EndsWith(".g.cs") && !IsUnknownModuleInFSharpAssembly(languageGuid, docName)) { firstNotFoundDocument = docName; return false; @@ -443,5 +446,12 @@ private bool IsAssembly(string filePath) return false; } } + + private bool IsUnknownModuleInFSharpAssembly(Guid languageGuid, string docName) + { + // https://github.com/dotnet/runtime/blob/main/docs/design/specs/PortablePdb-Metadata.md#document-table-0x30 + return languageGuid.Equals(new Guid("ab4f38c9-b6e6-43ba-be3b-58080b2ccce3")) + && docName.EndsWith("unknown"); + } } } \ No newline at end of file diff --git a/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs b/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs index 53219b293..dcdbc9499 100644 --- a/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs +++ b/test/coverlet.core.tests/Instrumentation/InstrumenterTests.cs @@ -513,6 +513,24 @@ public void TestInstrument_MissingModule() loggerMock.Verify(l => l.LogWarning(It.IsAny())); } + [Fact] + public void CanInstrumentFSharpAssemblyWithAnonymousRecord() + { + var loggerMock = new Mock(); + + string sample = Directory.GetFiles(Directory.GetCurrentDirectory(), "coverlet.tests.projectsample.fsharp.dll").First(); + InstrumentationHelper instrumentationHelper = + new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), new FileSystem(), new Mock().Object, + new SourceRootTranslator(sample, new Mock().Object, new FileSystem())); + + var instrumenter = new Instrumenter(sample, "_coverlet_tests_projectsample_fsharp", new CoverageParameters(), loggerMock.Object, instrumentationHelper, + new FileSystem(), new SourceRootTranslator(sample, loggerMock.Object, new FileSystem()), new CecilSymbolHelper()); + + Assert.True(instrumentationHelper.HasPdb(sample, out bool embedded)); + Assert.False(embedded); + Assert.True(instrumenter.CanInstrument()); + } + [Theory] [InlineData("NotAMatch", new string[] { }, false)] [InlineData("ExcludeFromCoverageAttribute", new string[] { }, true)] diff --git a/test/coverlet.core.tests/coverlet.core.tests.csproj b/test/coverlet.core.tests/coverlet.core.tests.csproj index 2a7adf9aa..b7cd119b3 100644 --- a/test/coverlet.core.tests/coverlet.core.tests.csproj +++ b/test/coverlet.core.tests/coverlet.core.tests.csproj @@ -24,6 +24,7 @@ + diff --git a/test/coverlet.tests.projectsample.fsharp/Library.fs b/test/coverlet.tests.projectsample.fsharp/Library.fs new file mode 100644 index 000000000..36ea79c12 --- /dev/null +++ b/test/coverlet.tests.projectsample.fsharp/Library.fs @@ -0,0 +1,4 @@ +namespace coverlet.tests.projectsample.fsharp + +module TestModule = + type Type1 = Option1 | Option2 of {| x: string |} diff --git a/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj b/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj new file mode 100644 index 000000000..3bc351866 --- /dev/null +++ b/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj @@ -0,0 +1,12 @@ + + + + net5.0 + true + + + + + + + From 66418e67a90c786e3c9b3f1522aeac6c600d4790 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20M=C3=BCller?= Date: Tue, 18 May 2021 12:50:28 +0200 Subject: [PATCH 2/5] added test sdk --- .../coverlet.tests.projectsample.fsharp.fsproj | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj b/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj index 3bc351866..b02240f2b 100644 --- a/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj +++ b/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj @@ -9,4 +9,8 @@ + + + + From b23ceaeda3735824e96f9769142068bef346aaf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20M=C3=BCller?= Date: Tue, 18 May 2021 16:43:38 +0200 Subject: [PATCH 3/5] removed the test sdk and added exclusion to build.props of the tests --- test/Directory.Build.props | 10 +++++++--- .../coverlet.tests.projectsample.fsharp.fsproj | 4 ---- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/test/Directory.Build.props b/test/Directory.Build.props index 670a745aa..839366f78 100644 --- a/test/Directory.Build.props +++ b/test/Directory.Build.props @@ -1,7 +1,11 @@ - - true - + + + + true + + + \ No newline at end of file diff --git a/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj b/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj index b02240f2b..3bc351866 100644 --- a/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj +++ b/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj @@ -9,8 +9,4 @@ - - - - From 99c37b1f39cb2f9909477dc9d46ecc897e6e51f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20M=C3=BCller?= Date: Wed, 19 May 2021 14:52:16 +0200 Subject: [PATCH 4/5] refactoring --- .../Helpers/InstrumentationHelper.cs | 60 ++++++++++--------- .../Helpers/InstrumentationHelperTests.cs | 23 ++++++- 2 files changed, 54 insertions(+), 29 deletions(-) diff --git a/src/coverlet.core/Helpers/InstrumentationHelper.cs b/src/coverlet.core/Helpers/InstrumentationHelper.cs index 69b6d5cde..8a6827621 100644 --- a/src/coverlet.core/Helpers/InstrumentationHelper.cs +++ b/src/coverlet.core/Helpers/InstrumentationHelper.cs @@ -117,21 +117,13 @@ public bool EmbeddedPortablePdbHasLocalSource(string module, out string firstNot using (MetadataReaderProvider embeddedMetadataProvider = peReader.ReadEmbeddedPortablePdbDebugDirectoryData(entry)) { MetadataReader metadataReader = embeddedMetadataProvider.GetMetadataReader(); - foreach (DocumentHandle docHandle in metadataReader.Documents) + + var matchingResult = MatchDocumentsWithSources(metadataReader); + + if (!matchingResult.allDocumentsMatch) { - Document document = metadataReader.GetDocument(docHandle); - string docName = _sourceRootTranslator.ResolveFilePath(metadataReader.GetString(document.Name)); - Guid languageGuid = metadataReader.GetGuid(document.Language); - // 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 - // We exclude from the check the autogenerated source file(i.e. source generators) - // We exclude special F# construct https://github.com/coverlet-coverage/coverlet/issues/1145 - if (!_fileSystem.Exists(docName) && !docName.EndsWith(".g.cs") && !IsUnknownModuleInFSharpAssembly(languageGuid, docName)) - { - firstNotFoundDocument = docName; - return false; - } + firstNotFoundDocument = matchingResult.notFoundDocument; + return false; } } } @@ -167,21 +159,12 @@ public bool PortablePdbHasLocalSource(string module, out string firstNotFoundDoc return true; } - foreach (DocumentHandle docHandle in metadataReader.Documents) + var matchingResult = MatchDocumentsWithSources(metadataReader); + + if (!matchingResult.allDocumentsMatch) { - Document document = metadataReader.GetDocument(docHandle); - string docName = _sourceRootTranslator.ResolveFilePath(metadataReader.GetString(document.Name)); - Guid languageGuid = metadataReader.GetGuid(document.Language); - // 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 - // We exlude from the check the autogenerated source file(i.e. source generators) - // We exclude special F# construct https://github.com/coverlet-coverage/coverlet/issues/1145 - if (!_fileSystem.Exists(docName) && !docName.EndsWith(".g.cs") && !IsUnknownModuleInFSharpAssembly(languageGuid, docName)) - { - firstNotFoundDocument = docName; - return false; - } + firstNotFoundDocument = matchingResult.notFoundDocument; + return false; } } } @@ -190,6 +173,27 @@ public bool PortablePdbHasLocalSource(string module, out string firstNotFoundDoc return true; } + private (bool allDocumentsMatch, string notFoundDocument) MatchDocumentsWithSources(MetadataReader metadataReader) + { + foreach (DocumentHandle docHandle in metadataReader.Documents) + { + Document document = metadataReader.GetDocument(docHandle); + string docName = _sourceRootTranslator.ResolveFilePath(metadataReader.GetString(document.Name)); + Guid languageGuid = metadataReader.GetGuid(document.Language); + // 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 + // We exlude from the check the autogenerated source file(i.e. source generators) + // We exclude special F# construct https://github.com/coverlet-coverage/coverlet/issues/1145 + if (!_fileSystem.Exists(docName) && !docName.EndsWith(".g.cs") && + !IsUnknownModuleInFSharpAssembly(languageGuid, docName)) + { + return (false, docName); + } + } + return (true, string.Empty); + } + public void BackupOriginalModule(string module, string identifier) { var backupPath = GetBackupPath(module, identifier); diff --git a/test/coverlet.core.tests/Helpers/InstrumentationHelperTests.cs b/test/coverlet.core.tests/Helpers/InstrumentationHelperTests.cs index a97e8d89a..2830f0857 100644 --- a/test/coverlet.core.tests/Helpers/InstrumentationHelperTests.cs +++ b/test/coverlet.core.tests/Helpers/InstrumentationHelperTests.cs @@ -3,6 +3,7 @@ using Xunit; using System.Collections.Generic; using System.Linq; +using Castle.Core.Internal; using Moq; using Coverlet.Core.Abstractions; @@ -10,7 +11,7 @@ namespace Coverlet.Core.Helpers.Tests { public class InstrumentationHelperTests { - private readonly InstrumentationHelper _instrumentationHelper = + private InstrumentationHelper _instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), new FileSystem(), new Mock().Object, new SourceRootTranslator(typeof(InstrumentationHelperTests).Assembly.Location, new Mock().Object, new FileSystem())); [Fact] @@ -29,6 +30,26 @@ public void TestGetDependenciesWithTestAssembly() Assert.True(Array.Exists(modules, m => m == module)); } + [Fact] + public void EmbeddedPortablePDPHasLocalSource_DocumentDoesNotExist_ReturnsFalse() + { + var fileSystem = new Mock {CallBase = true}; + fileSystem.Setup(x => x.Exists(It.IsAny())).Returns(false); + + _instrumentationHelper = + new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), fileSystem.Object, new Mock().Object, new SourceRootTranslator(typeof(InstrumentationHelperTests).Assembly.Location, new Mock().Object, new FileSystem())); + + Assert.False(_instrumentationHelper.PortablePdbHasLocalSource(typeof(InstrumentationHelperTests).Assembly.Location, out string notFoundDocument)); + Assert.False(notFoundDocument.IsNullOrEmpty()); + } + + [Fact] + public void EmbeddedPortablePDPHasLocalSource_AllDocumentsExist_ReturnsTrue() + { + Assert.True(_instrumentationHelper.PortablePdbHasLocalSource(typeof(InstrumentationHelperTests).Assembly.Location, out string notFoundDocument)); + Assert.True(notFoundDocument.IsNullOrEmpty()); + } + [Fact] public void TestHasPdb() { From 072e5390d8d00ba98e947e587af683433ebc0beb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20M=C3=BCller?= Date: Sat, 29 May 2021 02:22:43 +0200 Subject: [PATCH 5/5] code review --- test/Directory.Build.props | 10 +++------- .../Helpers/InstrumentationHelperTests.cs | 6 +++--- .../coverlet.tests.projectsample.fsharp.fsproj | 1 + 3 files changed, 7 insertions(+), 10 deletions(-) diff --git a/test/Directory.Build.props b/test/Directory.Build.props index 839366f78..670a745aa 100644 --- a/test/Directory.Build.props +++ b/test/Directory.Build.props @@ -1,11 +1,7 @@ - - - - true - - - + + true + \ No newline at end of file diff --git a/test/coverlet.core.tests/Helpers/InstrumentationHelperTests.cs b/test/coverlet.core.tests/Helpers/InstrumentationHelperTests.cs index 2830f0857..745009c72 100644 --- a/test/coverlet.core.tests/Helpers/InstrumentationHelperTests.cs +++ b/test/coverlet.core.tests/Helpers/InstrumentationHelperTests.cs @@ -11,7 +11,7 @@ namespace Coverlet.Core.Helpers.Tests { public class InstrumentationHelperTests { - private InstrumentationHelper _instrumentationHelper = + private readonly InstrumentationHelper _instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), new FileSystem(), new Mock().Object, new SourceRootTranslator(typeof(InstrumentationHelperTests).Assembly.Location, new Mock().Object, new FileSystem())); [Fact] @@ -36,10 +36,10 @@ public void EmbeddedPortablePDPHasLocalSource_DocumentDoesNotExist_ReturnsFalse( var fileSystem = new Mock {CallBase = true}; fileSystem.Setup(x => x.Exists(It.IsAny())).Returns(false); - _instrumentationHelper = + InstrumentationHelper instrumentationHelper = new InstrumentationHelper(new ProcessExitHandler(), new RetryHelper(), fileSystem.Object, new Mock().Object, new SourceRootTranslator(typeof(InstrumentationHelperTests).Assembly.Location, new Mock().Object, new FileSystem())); - Assert.False(_instrumentationHelper.PortablePdbHasLocalSource(typeof(InstrumentationHelperTests).Assembly.Location, out string notFoundDocument)); + Assert.False(instrumentationHelper.PortablePdbHasLocalSource(typeof(InstrumentationHelperTests).Assembly.Location, out string notFoundDocument)); Assert.False(notFoundDocument.IsNullOrEmpty()); } diff --git a/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj b/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj index 3bc351866..7a7d09a09 100644 --- a/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj +++ b/test/coverlet.tests.projectsample.fsharp/coverlet.tests.projectsample.fsharp.fsproj @@ -3,6 +3,7 @@ net5.0 true + false