Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions .github/tests/consumer_coverage.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,11 @@ def measure(args):
for file in actual["files"]
}
assert actual_files == files, f"Per-file mismatch: native {files}; DotCov {actual_files}"
# ItsDangerous's upstream reporting config also names tests as a source root.
# Keep the diagnostic and prove file attribution against the native JSON above.
# ItsDangerous's upstream reporting config also names tests as a source root. Every
# measured file exists under exactly one root, so DotCov attributes it without the
# multi-root warning; the per-file comparison above proves that attribution.
warnings = [warning["kind"] for warning in actual.get("warnings", [])]
assert warnings == (["FileIdentityAmbiguous"] if args.language == "python" else []), warnings
assert warnings == [], warnings
# Both real suites have missing lines and branches. No report is edited to manufacture failure.
line_hit, line_total, branch_hit, branch_total = total
assert 0.9 <= line_hit / line_total < 1
Expand Down
2 changes: 2 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ dotcov check coverage.xml --min-line 90 --min-branch 90
Pass the XML file directly to the badge action's `reports` input. This also avoids directory
search defaults: `coverage.xml` does not match `**/*cobertura*.xml`.
DotCov checks line and branch percentages separately; coverage.py's displayed total combines them.
With several coverage.py `source` roots, each file name is relative to the root that contains it.
DotCov uses the root under which the file exists, so run it where the sources are checked out.

CI runs the full suites of two pinned upstream projects on Linux, Windows, and macOS:

Expand Down
78 changes: 51 additions & 27 deletions src/DotCov/CoberturaParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,9 @@ private static async ValueTask VisitAsync(
/// Capture one <c>&lt;source&gt;</c> root. No-op spellings (".", "./") collapse to the ""
/// sentinel, which <see cref="ResolveFileKey"/> reads as "leave relative filenames
/// unprefixed". The sentinel is recorded, not discarded: a no-op declared alongside real
/// roots is a second resolution convention, so it counts toward the multi-root warning and
/// keeps its document-order slot (the FIRST declared root wins resolution). Respellings of
/// one root are deduplicated so they cannot fake that multiplicity.
/// roots is a second resolution convention, so it is a candidate root like any other and
/// keeps its document-order slot (the FIRST declared root wins when the disk cannot
/// decide). Respellings of one root are deduplicated so they cannot fake that multiplicity.
/// <para>
/// The root is the element's own character content: every text and CDATA child,
/// concatenated. A comment or processing instruction splits that text without ending it,
Expand All @@ -213,17 +213,7 @@ private static bool IsOwnText(XmlReader reader, int elementDepth) =>
private static void RecordSource(string value, DocumentContext document)
{
var root = PathIdentity.NormalizeRoot(value);
if (document.SourceRoots.Contains(root)) return; // List.Contains is Ordinal for strings

document.SourceRoots.Add(root);
if (document.SourceRoots.Count is 2)
document.Warn(new CoverageWarning(
CoverageWarningKind.FileIdentityAmbiguous,
"",
0,
$"multiple <source> roots - {(document.SourceRoots[0].Length is 0
? "leaving relative filenames unprefixed (the first declared root is a no-op)"
: $"resolving relative filenames against the first ('{document.SourceRoots[0]}')")}; files may not be attributable to a unique root"));
if (!document.SourceRoots.Contains(root)) document.SourceRoots.Add(root); // List.Contains is Ordinal for strings
}

/// <summary>
Expand Down Expand Up @@ -294,14 +284,14 @@ public void Visit(XmlReader sub)

/// <summary>
/// The file identity key of a <c>&lt;class filename&gt;</c>: separators normalized to '/'
/// (Windows coverlet writes '\'), the document's first <c>&lt;source&gt;</c> root applied
/// (Windows coverlet writes '\'), the document's <c>&lt;source&gt;</c> root applied
/// to relative names, and a leading drive letter uppercased. Both aggregations key on this
/// string, so method entries join against <see cref="FileCoverage.Path"/> on the same key.
/// Returns null for a class without a filename, which carries no attributable data.
/// </summary>
private static string? DecodeFileName(XmlReader reader, DocumentContext document) =>
reader.GetAttribute("filename") is { } filename
? ResolveFileKey(filename.Replace('\\', '/'), document.SourceRoots)
? ResolveFileKey(filename.Replace('\\', '/'), document)
: null;

/// <summary>
Expand Down Expand Up @@ -353,22 +343,20 @@ private static bool TryDecodeLine(XmlReader reader, string file, DocumentContext
}

/// <summary>
/// Prepend the report's first <c>&lt;source&gt;</c> root to a relative filename. Cobertura
/// Prepend the report's <c>&lt;source&gt;</c> root to a relative filename. Cobertura
/// emitters (coverage.py, gcovr, cover2cover) write filenames relative to a source root;
/// discarding it would make two different files that share a relative name collide on one key.
/// The rooted check is manual (leading '/' or a drive-letter prefix) because
/// <c>Path.IsPathRooted("C:/x")</c> is false on Linux and reports cross machines. The root
/// is applied unconditionally, never only on collision — a conditional prefix would make a
/// file's identity unstable across runs. A first-declared no-op root (the "" sentinel)
/// resolves as "no prefix": first-wins applies to the DECLARED order.
/// <c>Path.IsPathRooted("C:/x")</c> is false on Linux and reports cross machines. A single
/// root is applied unconditionally, never only on collision — a conditional prefix would make
/// a file's identity unstable across runs. Several roots follow <see cref="ResolveRoot"/>.
/// A no-op root (the "" sentinel) resolves as "no prefix".
/// </summary>
private static string ResolveFileKey(string filename, List<string> sourceRoots)
private static string ResolveFileKey(string filename, DocumentContext document)
{
if (sourceRoots.Count > 0 && sourceRoots[0].Length > 0 && !IsRooted(filename))
{
var root = sourceRoots[0];
filename = root.EndsWith('/') ? root + filename : $"{root}/{filename}";
}
var roots = document.SourceRoots;
if (roots.Count > 0 && !IsRooted(filename))
filename = JoinRoot(roots.Count is 1 ? roots[0] : ResolveRoot(filename, document), filename);

// Uppercase a leading drive letter so `c:\x\A.cs` and `C:/x/A.cs` — the same file
// emitted by different Windows toolchains — produce one Ordinal key. Key normalization,
Expand All @@ -379,6 +367,42 @@ private static string ResolveFileKey(string filename, List<string> sourceRoots)
return filename;
}

/// <summary>
/// Pick the root for a relative filename in a document that declares several. coverage.py
/// writes each file relative to the source root that contains it, so the one root under which
/// the file exists on disk is the file's own. When it exists under none (a report read away
/// from its sources) or under several, the first declared root applies and the document
/// carries one report-scoped warning.
/// </summary>
private static string ResolveRoot(string filename, DocumentContext document)
{
string? match = null;
foreach (var root in document.SourceRoots)
{
if (!File.Exists(JoinRoot(root, filename))) continue;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Reject report-supplied network roots before probing

On Windows, a Cobertura document with multiple roots can supply \\host\share (normalized to //host/share) and a relative class filename, causing both Parse and ParseAsync to call File.Exists on an attacker-controlled UNC path. This can initiate outbound SMB access, potentially use ambient authentication, and stall parsing while the network request times out; cancellation cannot interrupt the synchronous probe. Avoid probing network roots by default, or require an explicitly trusted/caller-provided source resolver.

Useful? React with 👍 / 👎.

if (match is not null) return FirstRootWithWarning(document);
match = root;
}

return match ?? FirstRootWithWarning(document);
}

private static string FirstRootWithWarning(DocumentContext document)
{
var first = document.SourceRoots[0];
document.Warn(new CoverageWarning(
CoverageWarningKind.FileIdentityAmbiguous,
"",
0,
$"multiple <source> roots - {(first.Length is 0
? "leaving relative filenames unprefixed (the first declared root is a no-op)"
: $"resolving relative filenames against the first ('{first}')")}; files may not be attributable to a unique root"));
return first;
}

private static string JoinRoot(string root, string filename) =>
root.Length is 0 ? filename : root.EndsWith('/') ? root + filename : $"{root}/{filename}";

private static bool IsRooted(string path) =>
path.StartsWith('/') ||
(path.Length >= 2 && char.IsAsciiLetter(path[0]) && path[1] == ':');
Expand Down
6 changes: 3 additions & 3 deletions src/DotCov/PathIdentity.cs
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@ internal static string NormalizeRoot(string root)
/// Whether two reports' declared roots genuinely differ — the single predicate behind
/// both the merge's ambiguity scan and the diff's unconditional name-pairing arm, so
/// the two can never disagree about what counts as a convention change. Compares
/// normalized SEQUENCES, not sets: root order is semantic (relative filenames resolve
/// against the FIRST root), so the same roots reordered can key a relative file
/// differently and must count as differing. Root-less on both sides is "same" —
/// normalized SEQUENCES, not sets: root order is semantic (a relative filename falls back to
/// the FIRST root when the disk cannot decide), so the same roots reordered can key a
/// relative file differently and must count as differing. Root-less on both sides is "same" —
/// hand-built reports never trip the cross-convention machinery.
/// </summary>
internal static bool RootsDiffer(IReadOnlyList<string> a, IReadOnlyList<string> b)
Expand Down
57 changes: 54 additions & 3 deletions tests/DotCov.Tests/ParserSourceTests.cs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System.Text;
using DotCov.Tests.Infrastructure;

namespace DotCov.Tests;

Expand All @@ -10,15 +11,18 @@ private static CoverageReport ParseXml(string xml)
return CoberturaParser.Parse(stream);
}

/// <summary>One-class document with optional <c>&lt;source&gt;</c> roots (raw, unescaped).</summary>
private static string Doc(string sources, string filename) =>
/// <summary>One class per filename, with optional <c>&lt;source&gt;</c> roots (raw, unescaped).</summary>
private static string Doc(string sources, params string[] filenames) =>
$"""
<?xml version="1.0"?>
<coverage>{sources}<packages><package><classes>
<class name="X" filename="{filename}"><lines><line number="1" hits="1" branch="false" /></lines></class>
{string.Concat(filenames.Select(f => $"""<class name="X" filename="{f}"><lines><line number="1" hits="1" branch="false" /></lines></class>"""))}
</classes></package></packages></coverage>
""";

/// <summary>A workspace directory as a declared root: forward slashes, like the parser's keys.</summary>
private static string RootOf(TempWorkspace ws, string relative) => ws.CreateDirectory(relative).Replace('\\', '/');

[Test]
public async Task Parse_EmptySourceElement_DoesNotSwallowTheFollowingRoot()
{
Expand Down Expand Up @@ -50,4 +54,51 @@ public async Task Parse_MultipleRoots_WarningIsReportScopedWithEmptyFile()
await Assert.That(w.Line).IsEqualTo(0);
await Assert.That(w.Detail).Contains("'/a'");
}

[Test]
public async Task Parse_MultipleRoots_EachFileResolvesAgainstTheRootThatHasIt()
{
// coverage.py with `source = ["src", "tests"]` names each file relative to its own root.
using var ws = TempWorkspace.Create("dotcov-source-roots-");
var src = RootOf(ws, "src");
var tests = RootOf(ws, "tests");
ws.Write("src/pkg/app.py", "");
ws.Write("tests/test_app.py", "");

var report = ParseXml(Doc($"<sources><source>{src}</source><source>{tests}</source></sources>", "pkg/app.py", "test_app.py"));

await Assert.That(report.Files.Select(f => f.Path).Order(StringComparer.Ordinal))
.IsEquivalentTo(new[] { $"{src}/pkg/app.py", $"{tests}/test_app.py" }.Order(StringComparer.Ordinal));
await Assert.That(report.Warnings).IsEmpty();
}

[Test]
public async Task Parse_MultipleRoots_FileUnderEveryRoot_UsesTheFirstAndWarns()
{
using var ws = TempWorkspace.Create("dotcov-source-roots-");
var a = RootOf(ws, "a");
var b = RootOf(ws, "b");
ws.Write("a/x.py", "");
ws.Write("b/x.py", "");

var report = ParseXml(Doc($"<sources><source>{a}</source><source>{b}</source></sources>", "x.py"));

await Assert.That(report.Files.Single().Path).IsEqualTo($"{a}/x.py");
var w = await Assert.That(report.Warnings).HasSingleItem();
await Assert.That(w.Kind).IsEqualTo(CoverageWarningKind.FileIdentityAmbiguous);
await Assert.That(w.Detail).Contains($"'{a}'");
}

[Test]
public async Task Parse_NoOpRootBesideARealRoot_UsesTheRootThatHasTheFile()
{
using var ws = TempWorkspace.Create("dotcov-source-roots-");
var src = RootOf(ws, "src");
ws.Write("src/pkg/app.py", "");

var report = ParseXml(Doc($"<sources><source>.</source><source>{src}</source></sources>", "pkg/app.py"));

await Assert.That(report.Files.Single().Path).IsEqualTo($"{src}/pkg/app.py");
await Assert.That(report.Warnings).IsEmpty();
}
}
Loading