From 9c81d89abfda2a8d6a2e51f1d1086dd842b04f90 Mon Sep 17 00:00:00 2001 From: Simon Potter Date: Mon, 30 Apr 2018 19:41:00 -0400 Subject: [PATCH 1/7] Add coverage for properties and nested types. --- .../Attributes/ExcludeFromCoverage.cs | 4 +- .../Instrumentation/Instrumenter.cs | 96 +++++++++++++++---- 2 files changed, 81 insertions(+), 19 deletions(-) diff --git a/src/coverlet.core/Attributes/ExcludeFromCoverage.cs b/src/coverlet.core/Attributes/ExcludeFromCoverage.cs index 9a09ac128..e2ac0e523 100644 --- a/src/coverlet.core/Attributes/ExcludeFromCoverage.cs +++ b/src/coverlet.core/Attributes/ExcludeFromCoverage.cs @@ -2,6 +2,6 @@ namespace Coverlet.Core.Attributes { - [AttributeUsage(AttributeTargets.Method | AttributeTargets.Constructor)] - public class ExcludeFromCoverageAttribute : Attribute { } + [AttributeUsage(AttributeTargets.Property | AttributeTargets.Method | AttributeTargets.Constructor)] + public sealed class ExcludeFromCoverageAttribute : Attribute { } } \ No newline at end of file diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index c64f38ed2..a42c4426e 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -1,12 +1,15 @@ +using System; +using System.Collections.Generic; using System.IO; using System.Linq; +using System.Reflection; +using Coverlet.Core.Attributes; using Coverlet.Core.Helpers; using Mono.Cecil; using Mono.Cecil.Cil; using Mono.Cecil.Rocks; -using System.Collections.Generic; namespace Coverlet.Core.Instrumentation { @@ -14,7 +17,8 @@ internal class Instrumenter { private readonly string _module; private readonly string _identifier; - private IEnumerable _excludedFiles; + private readonly IEnumerable _excludedFiles; + private readonly static Lazy _markExecutedMethodLoader = new Lazy(GetMarkExecutedMethod); private InstrumenterResult _result; public Instrumenter(string module, string identifier, IEnumerable excludedFiles = null) @@ -57,18 +61,7 @@ private void InstrumentModule() { foreach (var type in module.GetTypes()) { - if (type.CustomAttributes.Any(a => a.AttributeType.Name == "ExcludeFromCoverageAttribute" || a.AttributeType.Name == "ExcludeFromCoverage")) - continue; - - foreach (var method in type.Methods) - { - var sourceFile = method.DebugInformation.SequencePoints.Select(s => s.Document.Url).FirstOrDefault(); - if (!string.IsNullOrEmpty(sourceFile) && _excludedFiles.Contains(sourceFile)) { - continue; - } - if (!method.CustomAttributes.Any(a => a.AttributeType.Name == "ExcludeFromCoverageAttribute" || a.AttributeType.Name == "ExcludeFromCoverage")) - InstrumentMethod(method); - } + InstrumentType(type); } module.Write(stream); @@ -76,9 +69,54 @@ private void InstrumentModule() } } + private void InstrumentType(TypeDefinition type) + { + if (type.CustomAttributes.Any(IsExcludeAttribute)) + return; + + foreach (var method in type.Methods) + { + if (!method.CustomAttributes.Any(IsExcludeAttribute)) + InstrumentMethod(method); + } + + foreach (var property in type.Properties) + { + if (!property.CustomAttributes.Any(IsExcludeAttribute)) + InstrumentProperty(property); + } + + if (type.HasNestedTypes) + { + foreach (var nestedType in type.NestedTypes) + InstrumentType(nestedType); + } + } + + private void InstrumentProperty(PropertyDefinition property) + { + if (property.GetMethod != null && !property.GetMethod.IsAbstract) + { + InstrumentMethod(property.GetMethod); + } + + if (property.SetMethod != null && !property.SetMethod.IsAbstract) + { + InstrumentMethod(property.SetMethod); + } + } + private void InstrumentMethod(MethodDefinition method) { - if (!method.HasBody) + var sourceFile = method.DebugInformation.SequencePoints.Select(s => s.Document.Url).FirstOrDefault(); + if (!string.IsNullOrEmpty(sourceFile) && _excludedFiles.Contains(sourceFile)) + return; + + var methodBody = GetMethodBody(method); + if (methodBody == null) + return; + + if (method.IsNative) return; InstrumentIL(method); @@ -95,7 +133,7 @@ private void InstrumentIL(MethodDefinition method) { var instruction = processor.Body.Instructions[index]; var sequencePoint = method.DebugInformation.GetSequencePoint(instruction); - if (sequencePoint == null || sequencePoint.StartLine == 16707566) + if (sequencePoint == null || sequencePoint.StartLine == HiddenSequencePoint) { index++; continue; @@ -135,7 +173,7 @@ private Instruction AddInstrumentationCode(MethodDefinition method, ILProcessor var pathInstr = Instruction.Create(OpCodes.Ldstr, _result.HitsFilePath); var markInstr = Instruction.Create(OpCodes.Ldstr, marker); - var callInstr = Instruction.Create(OpCodes.Call, processor.Body.Method.Module.ImportReference(typeof(CoverageTracker).GetMethod("MarkExecuted"))); + var callInstr = Instruction.Create(OpCodes.Call, processor.Body.Method.Module.ImportReference(_markExecutedMethodLoader.Value)); processor.InsertBefore(instruction, callInstr); processor.InsertBefore(callInstr, markInstr); @@ -198,5 +236,29 @@ private static void ReplaceExceptionHandlerBoundary(ExceptionHandler handler, In if (handler.TryStart == oldTarget) handler.TryStart = newTarget; } + + private static bool IsExcludeAttribute(CustomAttribute customAttribute) + { + return customAttribute.AttributeType.Name == nameof(ExcludeFromCoverageAttribute) || customAttribute.AttributeType.Name == "ExcludeFromCoverage"; + } + + private static Mono.Cecil.Cil.MethodBody GetMethodBody(MethodDefinition method) + { + try + { + return method.HasBody ? method.Body : null; + } + catch + { + return null; + } + } + + private static MethodInfo GetMarkExecutedMethod() + { + return typeof(CoverageTracker).GetMethod(nameof(CoverageTracker.MarkExecuted)); + } + + private const int HiddenSequencePoint = 0xFEEFEE; } } \ No newline at end of file From 292b0c58c78b867167dde271c7e3689d28fccd92 Mon Sep 17 00:00:00 2001 From: Simon Potter Date: Mon, 30 Apr 2018 19:44:14 -0400 Subject: [PATCH 2/7] Ensure dependencies are valid .NET assemblies. Minor refactorings elsewhere: * Inline functions to local functions. * Extract method for obtaining backup module path. * Avoid hard-coding coverlet assembly name. --- .../Helpers/InstrumentationHelper.cs | 75 ++++++++++++------- 1 file changed, 46 insertions(+), 29 deletions(-) diff --git a/src/coverlet.core/Helpers/InstrumentationHelper.cs b/src/coverlet.core/Helpers/InstrumentationHelper.cs index c36baca51..0da666369 100644 --- a/src/coverlet.core/Helpers/InstrumentationHelper.cs +++ b/src/coverlet.core/Helpers/InstrumentationHelper.cs @@ -2,10 +2,10 @@ using System.Collections.Generic; using System.IO; using System.Linq; +using System.Reflection; using System.Reflection.PortableExecutable; -using Microsoft.Extensions.FileSystemGlobbing; -using Coverlet.Core.Instrumentation; +using Microsoft.Extensions.FileSystemGlobbing; using Microsoft.Extensions.FileSystemGlobbing.Abstractions; namespace Coverlet.Core.Helpers @@ -15,7 +15,7 @@ internal static class InstrumentationHelper public static string[] GetDependencies(string module) { IEnumerable modules = Directory.GetFiles(Path.GetDirectoryName(module), "*.dll"); - modules = modules.Where(a => Path.GetFileName(a) != Path.GetFileName(module)); + modules = modules.Where(a => IsAssembly(a) && Path.GetFileName(a) != Path.GetFileName(module)); return modules.ToArray(); } @@ -41,39 +41,35 @@ public static bool HasPdb(string module) public static void CopyCoverletDependency(string module) { var directory = Path.GetDirectoryName(module); - if (Path.GetFileNameWithoutExtension(module) == "coverlet.core") - return; + var moduleFileName = Path.GetFileName(module); var assembly = typeof(Coverage).Assembly; string name = Path.GetFileName(assembly.Location); + if (name == moduleFileName) + return; + File.Copy(assembly.Location, Path.Combine(directory, name), true); } public static void BackupOriginalModule(string module, string identifier) { - var backupPath = Path.Combine( - Path.GetTempPath(), - Path.GetFileNameWithoutExtension(module) + "_" + identifier + ".dll" - ); - + var backupPath = GetBackupPath(module, identifier); File.Copy(module, backupPath); } public static void RestoreOriginalModule(string module, string identifier) { - var backupPath = Path.Combine( - Path.GetTempPath(), - Path.GetFileNameWithoutExtension(module) + "_" + identifier + ".dll" - ); + var backupPath = GetBackupPath(module, identifier); // Restore the original module - retry up to 10 times, since the destination file could be locked // See: https://github.com/tonerdo/coverlet/issues/25 var currentSleep = 6; - Func retryStrategy = () => { + TimeSpan retryStrategy() + { var sleep = TimeSpan.FromMilliseconds(currentSleep); currentSleep *= 2; return sleep; - }; + } RetryHelper.Retry(() => { File.Copy(backupPath, module, true); @@ -86,12 +82,12 @@ public static IEnumerable ReadHitsFile(string path) // Retry hitting the hits file - retry up to 10 times, since the file could be locked // See: https://github.com/tonerdo/coverlet/issues/25 var currentSleep = 6; - Func retryStrategy = () => + TimeSpan retryStrategy() { var sleep = TimeSpan.FromMilliseconds(currentSleep); currentSleep *= 2; return sleep; - }; + } return RetryHelper.Do(() => File.ReadLines(path), retryStrategy, 10); } @@ -101,23 +97,24 @@ public static void DeleteHitsFile(string path) // Retry hitting the hits file - retry up to 10 times, since the file could be locked // See: https://github.com/tonerdo/coverlet/issues/25 var currentSleep = 6; - Func retryStrategy = () => { + TimeSpan retryStrategy() + { var sleep = TimeSpan.FromMilliseconds(currentSleep); currentSleep *= 2; return sleep; - }; + } RetryHelper.Retry(() => File.Delete(path), retryStrategy, 10); } - - public static IEnumerable GetExcludedFiles(IEnumerable excludeRules, - string parentDir = null) + + public static IEnumerable GetExcludedFiles(IEnumerable excludeRules, + string parentDir = null) { const string RELATIVE_KEY = nameof(RELATIVE_KEY); parentDir = string.IsNullOrWhiteSpace(parentDir)? Directory.GetCurrentDirectory() : parentDir; - + if (excludeRules == null || !excludeRules.Any()) return Enumerable.Empty(); - + var matcherDict = new Dictionary(){ {RELATIVE_KEY, new Matcher()}}; foreach (var excludeRule in excludeRules) { @@ -125,14 +122,13 @@ public static IEnumerable GetExcludedFiles(IEnumerable excludeRu var root = Path.GetPathRoot(excludeRule); if (!matcherDict.ContainsKey(root)) { matcherDict.Add(root, new Matcher()); - } + } matcherDict[root].AddInclude(excludeRule.Substring(root.Length)); } else { matcherDict[RELATIVE_KEY].AddInclude(excludeRule); } - } - + var files = new List(); foreach(var entry in matcherDict) { @@ -144,9 +140,30 @@ public static IEnumerable GetExcludedFiles(IEnumerable excludeRu .Select(f => Path.GetFullPath(Path.Combine(directoryInfo.ToString(), f.Path))); files.AddRange(currentFiles); } - + return files.Distinct(); } + + private static bool IsAssembly(string filePath) + { + try + { + AssemblyName.GetAssemblyName(filePath); + return true; + } + catch + { + return false; + } + } + + private static string GetBackupPath(string module, string identifier) + { + return Path.Combine( + Path.GetTempPath(), + Path.GetFileNameWithoutExtension(module) + "_" + identifier + ".dll" + ); + } } } From aeafe1bd3d91b730c493a0284afe69b367c76786 Mon Sep 17 00:00:00 2001 From: Simon Potter Date: Mon, 30 Apr 2018 19:44:58 -0400 Subject: [PATCH 3/7] Add namespace to RetryHelper. --- src/coverlet.core/Helpers/RetryHelper.cs | 87 ++++++++++++------------ 1 file changed, 45 insertions(+), 42 deletions(-) diff --git a/src/coverlet.core/Helpers/RetryHelper.cs b/src/coverlet.core/Helpers/RetryHelper.cs index 59d5e4750..1ee9e4390 100644 --- a/src/coverlet.core/Helpers/RetryHelper.cs +++ b/src/coverlet.core/Helpers/RetryHelper.cs @@ -2,56 +2,59 @@ using System.Collections.Generic; using System.Threading; -// A slightly amended version of the code found here: https://stackoverflow.com/a/1563234/186184 -// This code allows for varying backoff strategies through the use of Func. -public static class RetryHelper +namespace Coverlet.Core { - /// - /// Retry a void method. - /// - /// The action to perform - /// A function returning a Timespan defining the backoff strategy to use. - /// The maximum number of retries before bailing out. Defaults to 3. - public static void Retry( - Action action, - Func backoffStrategy, - int maxAttemptCount = 3) + // A slightly amended version of the code found here: https://stackoverflow.com/a/1563234/186184 + // This code allows for varying backoff strategies through the use of Func. + public static class RetryHelper { - Do(() => + /// + /// Retry a void method. + /// + /// The action to perform + /// A function returning a Timespan defining the backoff strategy to use. + /// The maximum number of retries before bailing out. Defaults to 3. + public static void Retry( + Action action, + Func backoffStrategy, + int maxAttemptCount = 3) { - action(); - return null; - }, backoffStrategy, maxAttemptCount); - } - - /// - /// Retry a method returning type T. - /// - /// The action to perform - /// A function returning a Timespan defining the backoff strategy to use. - /// The maximum number of retries before bailing out. Defaults to 3. - public static T Do( - Func action, - Func backoffStrategy, - int maxAttemptCount = 3) - { - var exceptions = new List(); + Do(() => + { + action(); + return null; + }, backoffStrategy, maxAttemptCount); + } - for (int attempted = 0; attempted < maxAttemptCount; attempted++) + /// + /// Retry a method returning type T. + /// + /// The type of the object to return + /// The action to perform + /// A function returning a Timespan defining the backoff strategy to use. + /// The maximum number of retries before bailing out. Defaults to 3. + public static T Do( + Func action, + Func backoffStrategy, + int maxAttemptCount = 3) { - try + var exceptions = new List(); + + for (int attempted = 0; attempted < maxAttemptCount; attempted++) { - if (attempted > 0) + try { - Thread.Sleep(backoffStrategy()); + if (attempted > 0) + { + Thread.Sleep(backoffStrategy()); + } + return action(); + } + catch (Exception ex) + { + exceptions.Add(ex); } - return action(); - } - catch (Exception ex) - { - exceptions.Add(ex); } + throw new AggregateException(exceptions); } - throw new AggregateException(exceptions); } -} \ No newline at end of file From f694acf5778cdf179de932f98c4966148ca15799 Mon Sep 17 00:00:00 2001 From: Simon Potter Date: Mon, 30 Apr 2018 22:30:36 -0400 Subject: [PATCH 4/7] Fix typo where last brace was removed. --- src/coverlet.core/Helpers/RetryHelper.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coverlet.core/Helpers/RetryHelper.cs b/src/coverlet.core/Helpers/RetryHelper.cs index 1ee9e4390..fed49230a 100644 --- a/src/coverlet.core/Helpers/RetryHelper.cs +++ b/src/coverlet.core/Helpers/RetryHelper.cs @@ -58,3 +58,4 @@ public static T Do( throw new AggregateException(exceptions); } } +} \ No newline at end of file From 7c8f5d0494ff40fb70abbc22d7ded20040b63faf Mon Sep 17 00:00:00 2001 From: Simon Potter Date: Tue, 1 May 2018 15:22:08 -0400 Subject: [PATCH 5/7] Use Mono.Cecil's IsHidden property instead of checking ourselves. --- src/coverlet.core/Instrumentation/Instrumenter.cs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index a42c4426e..a2e0e55ee 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -133,7 +133,7 @@ private void InstrumentIL(MethodDefinition method) { var instruction = processor.Body.Instructions[index]; var sequencePoint = method.DebugInformation.GetSequencePoint(instruction); - if (sequencePoint == null || sequencePoint.StartLine == HiddenSequencePoint) + if (sequencePoint == null || sequencePoint.IsHidden) { index++; continue; @@ -258,7 +258,5 @@ private static MethodInfo GetMarkExecutedMethod() { return typeof(CoverageTracker).GetMethod(nameof(CoverageTracker.MarkExecuted)); } - - private const int HiddenSequencePoint = 0xFEEFEE; } } \ No newline at end of file From ab5ea14bb11347d645f7628fa1010effc859ea3d Mon Sep 17 00:00:00 2001 From: Simon Potter Date: Tue, 1 May 2018 15:29:28 -0400 Subject: [PATCH 6/7] Extract method for creating a retry strategy. --- .../Helpers/InstrumentationHelper.cs | 36 ++++++++----------- 1 file changed, 15 insertions(+), 21 deletions(-) diff --git a/src/coverlet.core/Helpers/InstrumentationHelper.cs b/src/coverlet.core/Helpers/InstrumentationHelper.cs index 0da666369..96b140f03 100644 --- a/src/coverlet.core/Helpers/InstrumentationHelper.cs +++ b/src/coverlet.core/Helpers/InstrumentationHelper.cs @@ -63,13 +63,7 @@ public static void RestoreOriginalModule(string module, string identifier) // Restore the original module - retry up to 10 times, since the destination file could be locked // See: https://github.com/tonerdo/coverlet/issues/25 - var currentSleep = 6; - TimeSpan retryStrategy() - { - var sleep = TimeSpan.FromMilliseconds(currentSleep); - currentSleep *= 2; - return sleep; - } + var retryStrategy = CreateRetryStrategy(); RetryHelper.Retry(() => { File.Copy(backupPath, module, true); @@ -81,13 +75,7 @@ public static IEnumerable ReadHitsFile(string path) { // Retry hitting the hits file - retry up to 10 times, since the file could be locked // See: https://github.com/tonerdo/coverlet/issues/25 - var currentSleep = 6; - TimeSpan retryStrategy() - { - var sleep = TimeSpan.FromMilliseconds(currentSleep); - currentSleep *= 2; - return sleep; - } + var retryStrategy = CreateRetryStrategy(); return RetryHelper.Do(() => File.ReadLines(path), retryStrategy, 10); } @@ -96,13 +84,7 @@ public static void DeleteHitsFile(string path) { // Retry hitting the hits file - retry up to 10 times, since the file could be locked // See: https://github.com/tonerdo/coverlet/issues/25 - var currentSleep = 6; - TimeSpan retryStrategy() - { - var sleep = TimeSpan.FromMilliseconds(currentSleep); - currentSleep *= 2; - return sleep; - } + var retryStrategy = CreateRetryStrategy(); RetryHelper.Retry(() => File.Delete(path), retryStrategy, 10); } @@ -164,6 +146,18 @@ private static string GetBackupPath(string module, string identifier) Path.GetFileNameWithoutExtension(module) + "_" + identifier + ".dll" ); } + + private static Func CreateRetryStrategy(int initialSleepSeconds = 6) + { + TimeSpan retryStrategy() + { + var sleep = TimeSpan.FromMilliseconds(initialSleepSeconds); + initialSleepSeconds *= 2; + return sleep; + } + + return retryStrategy; + } } } From a94f010ee3e7784eb063405a36926aa001c14ef3 Mon Sep 17 00:00:00 2001 From: Simon Potter Date: Tue, 1 May 2018 15:55:22 -0400 Subject: [PATCH 7/7] Remove unnecessary type and property instrumentation. --- .../Instrumentation/Instrumenter.cs | 25 ------------------- 1 file changed, 25 deletions(-) diff --git a/src/coverlet.core/Instrumentation/Instrumenter.cs b/src/coverlet.core/Instrumentation/Instrumenter.cs index a2e0e55ee..1387546d0 100644 --- a/src/coverlet.core/Instrumentation/Instrumenter.cs +++ b/src/coverlet.core/Instrumentation/Instrumenter.cs @@ -79,31 +79,6 @@ private void InstrumentType(TypeDefinition type) if (!method.CustomAttributes.Any(IsExcludeAttribute)) InstrumentMethod(method); } - - foreach (var property in type.Properties) - { - if (!property.CustomAttributes.Any(IsExcludeAttribute)) - InstrumentProperty(property); - } - - if (type.HasNestedTypes) - { - foreach (var nestedType in type.NestedTypes) - InstrumentType(nestedType); - } - } - - private void InstrumentProperty(PropertyDefinition property) - { - if (property.GetMethod != null && !property.GetMethod.IsAbstract) - { - InstrumentMethod(property.GetMethod); - } - - if (property.SetMethod != null && !property.SetMethod.IsAbstract) - { - InstrumentMethod(property.SetMethod); - } } private void InstrumentMethod(MethodDefinition method)