diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f101af1..9c7e222 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,7 +32,7 @@ jobs: run: dotnet pack src/Crap4DotNet.Cli -o ./nupkg - name: Install dotnet-crap - run: dotnet tool install -g Crap4DotNet --add-source ./nupkg + run: dotnet tool install -g Crap4DotNet --configfile nuget.ci.config --prerelease - name: Self-analyze with CRAP metrics continue-on-error: true diff --git a/README.md b/README.md index e0e358f..1c30926 100644 --- a/README.md +++ b/README.md @@ -38,8 +38,12 @@ Requires .NET 8.0 or later. ## Quick Start +Coverage is measured by [Coverlet](https://github.com/coverlet-coverage/coverlet), so every +test project needs a `coverlet.collector` package reference. crap4dotnet reads the Cobertura +XML Coverlet produces and pairs it with the complexity it calculates from your source. + ```bash -# Option A: Let crap4dotnet run tests and generate coverage automatically +# Option A: let crap4dotnet run `dotnet test` for you and pick up the coverage dotnet-crap analyze ./MyApp.sln --run-tests # Option B: Generate coverage yourself, then analyze @@ -65,7 +69,7 @@ dotnet-crap analyze [options] |-----------------|-------------| | `` | Path to `.cs` file, directory, `.csproj`, or `.sln` | | `--coverage ` | Path(s) to Cobertura XML coverage file(s) | -| `--run-tests` | Run `dotnet test` to generate coverage automatically | +| `--run-tests` | Run `dotnet test --collect:"XPlat Code Coverage"` and use the coverage it produces | | `--threshold ` | CRAP threshold (default: 30) | | `--output ` | Write JSON to file instead of stdout | | `--min-crap ` | Only include methods with CRAP >= this value | diff --git a/nuget.ci.config b/nuget.ci.config new file mode 100644 index 0000000..44362f5 --- /dev/null +++ b/nuget.ci.config @@ -0,0 +1,27 @@ + + + + + + + + + + + + + diff --git a/src/Crap4DotNet.Core/Coverage/CoberturaCoverageReader.cs b/src/Crap4DotNet.Core/Coverage/CoberturaCoverageReader.cs index b1939c0..9b15b8a 100644 --- a/src/Crap4DotNet.Core/Coverage/CoberturaCoverageReader.cs +++ b/src/Crap4DotNet.Core/Coverage/CoberturaCoverageReader.cs @@ -48,6 +48,7 @@ private static List ReadFromDocument(XDocument doc) MethodName = methodName, Signature = signature, FileName = filename, + StartLine = FirstLineNumber(method) ?? FirstLineNumber(cls), Coverage = coverage }); } @@ -79,6 +80,25 @@ private static double SelectCoverage(XElement method) return ParseDouble(branchRateAttr.Value); } + /// + /// Lowest source line number recorded under an element, if any. + /// + private static int? FirstLineNumber(XElement element) + { + int? lowest = null; + foreach (var line in element.Descendants("line")) + { + var attr = line.Attribute("number"); + if (attr is null) + continue; + if (int.TryParse(attr.Value, NumberStyles.Integer, CultureInfo.InvariantCulture, out var number) + && (lowest is null || number < lowest)) + lowest = number; + } + + return lowest; + } + private static double ParseDouble(string value) => double.TryParse(value, NumberStyles.Float, CultureInfo.InvariantCulture, out var result) ? result diff --git a/src/Crap4DotNet.Core/Coverage/CoberturaMethodCoverage.cs b/src/Crap4DotNet.Core/Coverage/CoberturaMethodCoverage.cs index a9c7691..71123b5 100644 --- a/src/Crap4DotNet.Core/Coverage/CoberturaMethodCoverage.cs +++ b/src/Crap4DotNet.Core/Coverage/CoberturaMethodCoverage.cs @@ -11,5 +11,13 @@ public sealed record CoberturaMethodCoverage public required string MethodName { get; init; } public required string Signature { get; init; } public string? FileName { get; init; } + + /// + /// First source line this entry describes, where the report provides one. + /// Compiler-generated types keep the positions of the method they were rewritten + /// from, which is what makes overloads separable once the signature is gone. + /// + public int? StartLine { get; init; } + public required double Coverage { get; init; } } diff --git a/src/Crap4DotNet.Core/Matching/CoberturaMethodParser.cs b/src/Crap4DotNet.Core/Matching/CoberturaMethodParser.cs index eb4df5b..d729b55 100644 --- a/src/Crap4DotNet.Core/Matching/CoberturaMethodParser.cs +++ b/src/Crap4DotNet.Core/Matching/CoberturaMethodParser.cs @@ -1,3 +1,4 @@ +using System.Text.RegularExpressions; using Crap4DotNet.Core.Coverage; namespace Crap4DotNet.Core.Matching; @@ -7,7 +8,7 @@ namespace Crap4DotNet.Core.Matching; /// Converts CLR type names to C# keywords, backtick generics to angle brackets, /// nested type separators, and accessor/operator naming conventions. /// -public static class CoberturaMethodParser +public static partial class CoberturaMethodParser { private static readonly Dictionary ClrToCSharpTypes = new(StringComparer.Ordinal) { @@ -64,12 +65,54 @@ public static class CoberturaMethodParser /// public static string ToCanonicalKey(CoberturaMethodCoverage coverage) { + if (TryGetStateMachineOwnerKey(coverage.ClassName, coverage.MethodName, out var ownerKey)) + return ownerKey; + var className = NormalizeClassName(coverage.ClassName); var methodName = NormalizeMethodName(coverage.MethodName, className); var signature = NormalizeSignature(coverage.Signature); return $"{className}.{methodName}{signature}"; } + /// + /// Async and iterator bodies are compiled into a generated state machine type named + /// "Outer/<MethodName>d__N", and coverage is reported against MoveNext on that + /// type rather than against the method in the source. Recover the owning method so + /// the entry can be matched to it. + /// + /// + /// The generated type carries no parameter information, so the key is emitted + /// without a signature and resolves through the name-only fallback pass. + /// + private static bool TryGetStateMachineOwnerKey(string className, string methodName, out string key) + { + key = string.Empty; + + // Only MoveNext holds the rewritten body; the rest of the type is plumbing. + if (!string.Equals(methodName, "MoveNext", StringComparison.Ordinal)) + return false; + + var match = StateMachineClassRegex().Match(className); + if (!match.Success) + return false; + + var owner = NormalizeClassName(match.Groups["owner"].Value); + var method = match.Groups["method"].Value; + + // A generic method's state machine carries the method's arity as a backtick + // suffix, where the source side spells the type parameters out. Both sides + // reduce to the same angle-bracket form. + var arity = match.Groups["arity"].Value; + if (arity.Length > 0) + method += MethodKeyHelper.NormalizeBacktickGenerics(arity); + + key = $"{owner}.{method}"; + return true; + } + + [GeneratedRegex(@"^(?.+)/<(?[^>]+)>d__\d+(?`\d+)?$")] + private static partial Regex StateMachineClassRegex(); + private static string NormalizeClassName(string className) { // Nested type separator: / → . diff --git a/src/Crap4DotNet.Core/Matching/MethodCoverageMatcher.cs b/src/Crap4DotNet.Core/Matching/MethodCoverageMatcher.cs index 0238395..7df1fb2 100644 --- a/src/Crap4DotNet.Core/Matching/MethodCoverageMatcher.cs +++ b/src/Crap4DotNet.Core/Matching/MethodCoverageMatcher.cs @@ -17,6 +17,7 @@ public static MatchResult Match( // Build coverage lookups by normalized key var fullKeyLookup = new Dictionary>(StringComparer.Ordinal); var nameKeyLookup = new Dictionary>(StringComparer.Ordinal); + var erasedKeyLookup = new Dictionary>(StringComparer.Ordinal); foreach (var entry in coverageEntries) { @@ -25,10 +26,32 @@ public static MatchResult Match( var nameKey = MethodKeyHelper.GetNameOnlyKey(fullKey); AddToLookup(nameKeyLookup, nameKey, entry); + + var erasedKey = MethodKeyHelper.EraseMethodGenericArity(nameKey); + if (!string.Equals(erasedKey, nameKey, StringComparison.Ordinal)) + AddToLookup(erasedKeyLookup, erasedKey, entry); + else + AddToLookup(erasedKeyLookup, nameKey, entry); + } + + // Source methods that share a name-only key are the overloads a signature-less + // coverage key cannot tell apart; keep them together so they can be paired by + // source position when that happens. + var sourceGroups = new Dictionary>(StringComparer.Ordinal); + var erasedSourceGroups = new Dictionary>(StringComparer.Ordinal); + foreach (var complexity in complexityResults) + { + var key = MethodKeyHelper.GetNameOnlyKey(RoslynMethodParser.ToCanonicalKey(complexity.Identity)); + AddToGroup(sourceGroups, key, complexity); + + // The erased pass collapses Apply and Apply onto one key, so it needs + // its own grouping rather than reusing the name-only one. + AddToGroup(erasedSourceGroups, MethodKeyHelper.EraseMethodGenericArity(key), complexity); } var matchedFullKeys = new HashSet(StringComparer.Ordinal); var matchedNameKeys = new HashSet(StringComparer.Ordinal); + var matchedErasedKeys = new HashSet(StringComparer.Ordinal); var methods = new List(); var unmatchedNames = new List(); var warnings = new List(); @@ -43,21 +66,41 @@ public static MatchResult Match( matchedFullKeys.Add(fullKey); methods.Add(new MatchedMethod { + // One method can appear in several coverage files. Covered by any + // of them means covered, and picking the first would make the + // result depend on the order the files were passed in. Complexity = complexity, - Coverage = exactMatches[0].Coverage + Coverage = exactMatches.Max(m => m.Coverage) }); continue; } // Pass 2: Fallback to name-only key (without signature) var nameKey = MethodKeyHelper.GetNameOnlyKey(fullKey); - if (nameKeyLookup.TryGetValue(nameKey, out var nameMatches) && nameMatches.Count == 1) + if (nameKeyLookup.TryGetValue(nameKey, out var nameMatches) + && TryResolveForMethod(nameMatches, complexity, sourceGroups[nameKey], out var nameCoverage)) { matchedNameKeys.Add(nameKey); methods.Add(new MatchedMethod { Complexity = complexity, - Coverage = nameMatches[0].Coverage + Coverage = nameCoverage + }); + continue; + } + + // Pass 3: Generic methods carry no arity on the Cobertura side, so fall + // back to a key with the method's arity erased from both sides. + var erasedKey = MethodKeyHelper.EraseMethodGenericArity(nameKey); + if (erasedKeyLookup.TryGetValue(erasedKey, out var erasedMatches) + && TryResolveForMethod( + erasedMatches, complexity, erasedSourceGroups[erasedKey], out var erasedCoverage)) + { + matchedErasedKeys.Add(erasedKey); + methods.Add(new MatchedMethod + { + Complexity = complexity, + Coverage = erasedCoverage }); continue; } @@ -85,6 +128,10 @@ public static MatchResult Match( if (matchedNameKeys.Contains(nameKey)) continue; + // Check if matched by the generic-erased fallback + if (matchedErasedKeys.Contains(MethodKeyHelper.EraseMethodGenericArity(nameKey))) + continue; + orphanedCount += kvp.Value.Count; orphanedNames.AddRange( kvp.Value.Select(c => $"{c.ClassName}.{c.MethodName}")); @@ -138,6 +185,103 @@ public static MatchResult Match( }; } + /// + /// Pick the coverage entry belonging to one specific source method from candidates + /// that share a name-only key. + /// + /// + /// Candidates collide for two unrelated reasons. The same method is reported once + /// per coverage file, because every file describes the whole assembly it loaded; + /// those entries merge to the highest coverage observed. Separately, overloads + /// collide whenever a signature-less key is in play, which is unavoidable for + /// async and iterator methods. Those are told apart by source position, since a + /// generated state machine keeps the positions of the method it was rewritten + /// from. Pairing is positional and only applies when every overload has exactly + /// one entry; anything less certain returns false so the caller declines rather + /// than attributing one overload's coverage to another. + /// + private static bool TryResolveForMethod( + List candidates, + MethodComplexityResult complexity, + List siblings, + out double coverage) + { + coverage = 0.0; + + if (candidates.Count == 0) + return false; + + // Collapse the same entry arriving from several coverage files. + var distinct = candidates + .GroupBy(c => (c.ClassName, c.MethodName, c.Signature, c.StartLine)) + .Select(g => new + { + g.First().FileName, + g.First().StartLine, + Coverage = g.Max(c => c.Coverage) + }) + .ToList(); + + if (distinct.Count == 1 && siblings.Count == 1) + { + coverage = distinct[0].Coverage; + return true; + } + + // More than one real method behind the key: pair by source position, and only + // when the two sides line up exactly. + if (distinct.Count != siblings.Count) + return false; + + if (distinct.Exists(d => d.StartLine is null) + || siblings.Exists(sibling => sibling.Identity.LineNumber is null)) + return false; + + if (!distinct.TrueForAll(d => SameFile(complexity.Identity.FilePath, d.FileName))) + return false; + + var orderedEntries = distinct.OrderBy(d => d.StartLine).ToList(); + var orderedSiblings = siblings.OrderBy(sibling => sibling.Identity.LineNumber).ToList(); + + var index = orderedSiblings.FindIndex(sibling => sibling.Identity == complexity.Identity); + if (index < 0) + return false; + + coverage = orderedEntries[index].Coverage; + return true; + } + + /// + /// Whether a Roslyn file path and a Cobertura filename name the same file. The + /// report path is relative to the project, the Roslyn path is absolute. + /// + private static bool SameFile(string? sourcePath, string? coverageFile) + { + if (string.IsNullOrEmpty(sourcePath) || string.IsNullOrEmpty(coverageFile)) + return false; + + var source = sourcePath.Replace('\\', '/'); + var report = coverageFile.Replace('\\', '/'); + + return source.EndsWith(report, StringComparison.OrdinalIgnoreCase) + || string.Equals( + Path.GetFileName(source), Path.GetFileName(report), StringComparison.OrdinalIgnoreCase); + } + + private static void AddToGroup( + Dictionary> groups, + string key, + MethodComplexityResult complexity) + { + if (!groups.TryGetValue(key, out var group)) + { + group = []; + groups[key] = group; + } + + group.Add(complexity); + } + private static void AddToLookup( Dictionary> lookup, string key, diff --git a/src/Crap4DotNet.Core/Matching/MethodIdentityNormalizer.cs b/src/Crap4DotNet.Core/Matching/MethodIdentityNormalizer.cs index 8123776..3194964 100644 --- a/src/Crap4DotNet.Core/Matching/MethodIdentityNormalizer.cs +++ b/src/Crap4DotNet.Core/Matching/MethodIdentityNormalizer.cs @@ -74,6 +74,26 @@ public static List SplitTypeList(string typeList) return result; } + /// + /// Remove the generic arity marker from the method-name portion of a name-only key. + /// MyApp.Cache<>.Get<> -> MyApp.Cache<>.Get + /// + /// + /// Coverlet writes no arity on a generic method's name, so the arity cannot be + /// recovered from the coverage side. Erasing it from both sides gives them a + /// common key. The declaring type keeps its arity, which both sides do supply. + /// + public static string EraseMethodGenericArity(string nameOnlyKey) + { + var lastDot = nameOnlyKey.LastIndexOf('.'); + if (lastDot < 0) + return nameOnlyKey; + + var methodName = nameOnlyKey[(lastDot + 1)..]; + var angle = methodName.IndexOf('<'); + return angle < 0 ? nameOnlyKey : string.Concat(nameOnlyKey.AsSpan(0, lastDot + 1), methodName.AsSpan(0, angle)); + } + /// /// Convert CLR backtick generic arity notation to angle bracket notation. /// Cache`1 → Cache<>, Dictionary`2 → Dictionary<,> diff --git a/tests/Crap4DotNet.Core.Tests/Matching/MethodCoverageMatcherTests.cs b/tests/Crap4DotNet.Core.Tests/Matching/MethodCoverageMatcherTests.cs index 1479db4..1385cfc 100644 --- a/tests/Crap4DotNet.Core.Tests/Matching/MethodCoverageMatcherTests.cs +++ b/tests/Crap4DotNet.Core.Tests/Matching/MethodCoverageMatcherTests.cs @@ -14,7 +14,9 @@ private static MethodComplexityResult MakeComplexity( string className = "Service", string ns = "MyApp", string signature = "()", - int complexity = 5) => + int complexity = 5, + string filePath = "Test.cs", + int lineNumber = 1) => new() { Identity = new MethodIdentity @@ -24,8 +26,8 @@ private static MethodComplexityResult MakeComplexity( MethodName = methodName, Signature = signature, FullName = $"{ns}.{className}.{methodName}{signature}", - FilePath = "Test.cs", - LineNumber = 1 + FilePath = filePath, + LineNumber = lineNumber }, Complexity = complexity }; @@ -34,12 +36,16 @@ private static CoberturaMethodCoverage MakeCoverage( string methodName, string className = "MyApp.Service", string signature = "()", - double coverage = 0.8) => + double coverage = 0.8, + string? fileName = null, + int? startLine = null) => new() { ClassName = className, MethodName = methodName, Signature = signature, + FileName = fileName, + StartLine = startLine, Coverage = coverage }; @@ -390,4 +396,165 @@ public void ResultsPreserveComplexityOrder() result.Methods.Select(m => m.Coverage) .Should().Equal(0.3, 0.1, 0.2); } + + [Fact] + public void AsyncMethod_CoverageOnCompilerGeneratedStateMachine_IsAttributedToSourceMethod() + { + // The C# compiler rewrites an async body into a state machine class named + // "d__N", so Coverlet reports the coverage against MoveNext on + // that generated type rather than against the method the developer wrote. + var complexity = new[] { MakeComplexity("UpdateAsync", signature: "(int)") }; + var coverage = new[] + { + MakeCoverage( + "MoveNext", + className: "MyApp.Service/d__5", + coverage: 1.0) + }; + + var result = MethodCoverageMatcher.Match(complexity, coverage); + + result.Methods.Single().Coverage.Should().Be(1.0); + } + + [Fact] + public void GenericAsyncMethod_StateMachineArity_IsAttributedToSourceMethod() + { + // A generic async method's state machine carries the method's generic arity as + // a backtick suffix ("d__9`1"), while the source side spells the type + // parameters out ("Get"). Both have to reach the same canonical form. + var complexity = new[] { MakeComplexity("Get", signature: "(int)") }; + var coverage = new[] + { + MakeCoverage( + "MoveNext", + className: "MyApp.Service/d__9`1", + coverage: 1.0) + }; + + var result = MethodCoverageMatcher.Match(complexity, coverage); + + result.Methods.Single().Coverage.Should().Be(1.0); + } + [Fact] + public void GenericMethod_CoverageEntryCarriesNoArity_IsStillMatched() + { + // A non-async generic method has no state machine, and Coverlet writes no + // arity on the method name, so the entry is a bare "Apply". The source side + // normalizes type parameters to "Apply<>". The arity cannot be recovered from + // the coverage entry, so matching has to erase it from the source key instead. + var complexity = new[] { MakeComplexity("Apply", signature: "(int)") }; + var coverage = new[] { MakeCoverage("Apply", signature: "(System.Int32)", coverage: 1.0) }; + + var result = MethodCoverageMatcher.Match(complexity, coverage); + + result.Methods.Single().Coverage.Should().Be(1.0); + } + + [Fact] + public void SameMethodInTwoCoverageFiles_TakesTheHigherCoverage() + { + // A solution with several test projects emits one coverage file each, and a + // project that merely references the assembly under analysis reports its + // classes at zero. Which file was passed first must not decide the answer. + var complexity = new[] { MakeComplexity("Run") }; + var coverage = new[] + { + MakeCoverage("Run", coverage: 0.0), + MakeCoverage("Run", coverage: 1.0) + }; + + var result = MethodCoverageMatcher.Match(complexity, coverage); + + result.Methods.Single().Coverage.Should().Be(1.0); + } + + [Fact] + public void TwoAsyncOverloads_AreNotMergedIntoASingleCoverageValue() + { + // Two async overloads compile to two state machines differing only by ordinal, + // and recovery has to drop the signature to match them at all, so both reduce + // to the same name. Nothing in the coverage file says which ordinal belongs to + // which overload, so attributing one overload's coverage to the other would + // report an untested overload as covered. Declining is the safe answer. + var complexity = new[] + { + MakeComplexity("Create", signature: "(Expense)"), + MakeComplexity("Create", signature: "(ExpenseDetails)") + }; + var coverage = new[] + { + MakeCoverage("MoveNext", className: "MyApp.Service/d__5", coverage: 0.0), + MakeCoverage("MoveNext", className: "MyApp.Service/d__6", coverage: 1.0) + }; + + var result = MethodCoverageMatcher.Match(complexity, coverage); + + result.Methods.Should().OnlyContain(m => m.Coverage == 0.0); + } + + [Fact] + public void TwoAsyncOverloads_AreDistinguishedBySourcePosition() + { + // The generated state machine keeps the original source positions, so the + // overload each one belongs to can be recovered from where its lines sit even + // though the signature is gone. + var complexity = new[] + { + MakeComplexity("Create", signature: "(Expense)", + filePath: "ExpenseCreator.cs", lineNumber: 26), + MakeComplexity("Create", signature: "(ExpenseDetails)", + filePath: "ExpenseCreator.cs", lineNumber: 63) + }; + var coverage = new[] + { + MakeCoverage("MoveNext", className: "MyApp.Service/d__5", + coverage: 0.83, fileName: "ExpenseCreator.cs", startLine: 27), + MakeCoverage("MoveNext", className: "MyApp.Service/d__6", + coverage: 1.0, fileName: "ExpenseCreator.cs", startLine: 64) + }; + + var result = MethodCoverageMatcher.Match(complexity, coverage); + + result.Methods.Single(m => m.Complexity.Identity.Signature == "(Expense)") + .Coverage.Should().Be(0.83); + result.Methods.Single(m => m.Complexity.Identity.Signature == "(ExpenseDetails)") + .Coverage.Should().Be(1.0); + } + + [Fact] + public void AsyncMethodInTwoCoverageFiles_IsStillMatchedByTheNameFallback() + { + // A recovered state machine key carries no signature, so it can only resolve + // through the name-only pass. With one coverage file per test project the same + // method arrives twice, and the pass must not mistake that for two overloads + // and drop the method to zero. + var complexity = new[] { MakeComplexity("Run", signature: "(int)") }; + var coverage = new[] + { + MakeCoverage("MoveNext", className: "MyApp.Service/d__1", coverage: 0.0), + MakeCoverage("MoveNext", className: "MyApp.Service/d__1", coverage: 1.0) + }; + + var result = MethodCoverageMatcher.Match(complexity, coverage); + + result.Methods.Single().Coverage.Should().Be(1.0); + } + + [Fact] + public void GenericMethodInTwoCoverageFiles_IsStillMatchedByTheErasedFallback() + { + // A generic method resolves only through the arity-erased pass, which needs + // the same tolerance for one method arriving from several coverage files. + var complexity = new[] { MakeComplexity("Apply", signature: "(int)") }; + var coverage = new[] + { + MakeCoverage("Apply", signature: "(System.Int32)", coverage: 0.0), + MakeCoverage("Apply", signature: "(System.Int32)", coverage: 1.0) + }; + + var result = MethodCoverageMatcher.Match(complexity, coverage); + + result.Methods.Single().Coverage.Should().Be(1.0); + } }