Relativize LCOV paths against a root, match coverage roots at segments

CHANGELOG.md

@@ -12,7 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - An environment activation that has not finished is now reported with a warning naming the test process and how long it has been waiting, repeating every `activation_progress_seconds` (a new `TestItemController` keyword, default 120). Activation covers the test process's own precompilation, so a slow one is legitimate and is not interrupted — but a run that stalls there no longer goes silent between the `Activating` status and whatever eventually gives up.
 - The JSON-RPC protocol gained a client → controller `shutdown` notification for a graceful shutdown: every run is cancelled, every test process is terminated (force-killed if it does not exit within the grace period) and the controller exits once they are gone.
 - `write_junit_xml(io_or_path, ::TestrunResult; root)` writes a test run as JUnit XML, the one report format every CI system ingests. Test items are grouped into one `<testsuite>` per source file and one `<testcase>` per (item × run profile); captured output goes to `<system-out>` with ANSI escape sequences stripped, and performance statistics become `<properties>`. It is a pure function of a `TestrunResult`, so a result file written by one process can be converted by another.
-- `write_lcov(io_or_path, ::TestrunResult)` writes a run's merged coverage in LCOV info format, so consumers no longer have to reach into the vendored `CoverageTools`.
+- `write_lcov(io_or_path, ::TestrunResult; root)` writes a run's merged coverage in LCOV info format, so consumers no longer have to reach into the vendored `CoverageTools`. `root` is a directory path or `file:` URI that the `SF:` paths are relativized against, mirroring `write_junit_xml` — coverage services match `SF:` paths against paths in the repository, and the absolute paths of a CI runner match nothing at all, which is one way a fully covered package comes out at 0%. A file outside `root` keeps its absolute path rather than a `..`-heavy one, the same choice the JUnit writer makes, and paths always use `/` separators so a Windows leg and a Linux leg of the same matrix contribute the same file names to a merged report.
 - `TestrunResultTestitemProfile` gained a `perf` field and `TestrunResult` a `coverage` field. Both are optional and both are read tolerantly: a result file written before these existed still parses, which matters because `julia-report-ci-results` merges files produced by every leg of a CI matrix and those legs are not necessarily on the same version.
 - `TestItemDetail` gained `option_skip`, carrying the `@testitem` `skip` kwarg — either a literal `Bool` or the source text of an expression, which is evaluated in the test process rather than the controller so it sees the test process's Julia version and platform.
 - The terminal test item callbacks take an optional trailing argument: performance statistics for `passed`/`failed`/`errored`, and a reason for `skipped`. Callbacks written against the previous signatures are unaffected — the controller falls back to the arity the callback accepts.
@@ -33,6 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- Coverage root URIs are now matched at whole path segments. A root is a folder URI and `filepath2uri` never leaves a trailing slash, so the prefix test also accepted siblings whose names merely started with it — a root of `<workspace>/Foo` collected coverage for `<workspace>/Foo2` as well.
 - Hang diagnostics include a CPU profile again when the controller is embedded in a host that pins `JULIA_LOAD_PATH` — `Pkg.test` being the one that matters, which is why this only ever showed up on CI. The watchdog used to load `Profile` at runtime, on the grounds that the test process's pinned environment did not list it, and it did so by name; a by-name lookup goes through the load path and only finds a stdlib while `@stdlib` is on it. Inheriting a load path of just the test environment, the lookup failed and every dump silently degraded to "No CPU profile: the Profile stdlib is not loadable in this test process." `Profile` is now a declared dependency of `TestItemServer` like every other stdlib it uses, so it resolves through the pinned environment's manifest and the load path is irrelevant.
 - Activating a test environment no longer refreshes the user's package registry, which was both an unrequested side effect and a race. A test process runs tests in an environment the host has already set up, but `Pkg.develop` and the `Pkg.resolve` inside `TestEnv.activate` would auto-update the General registry on the way through. When several test processes do that at once — they belong to different controllers, so the controller's precompile gate, which only serializes the processes of one controller, does not cover it — one process's open handle makes another's `unlink` of `registries/General.tar.gz` fail with `EBUSY` on Windows, and the whole activation fails: every test item assigned to that process is reported as errored with the `IOError` as its message. Only the automatic update is suppressed; a package missing from the depot still gets installed. The trade-off is that a dependency newer than the local registry no longer triggers a refresh during resolve.
 - Errors that the controller was written to recover from no longer take the whole controller down with them, and the recovery they were meant to trigger now actually runs. When VS Code launches the controller it installs a logger that turns any `@error` into a crash report and then exits the process, so the five `@error` sites written as "log this and carry on" — failing to activate an environment, to configure a run, to send items to a process, to steal items from one, and to run Revise — were killing the controller instead, and the `TestProcessIOErrorMsg` each of them queues on the very next line never ran. Those sites now log at `@warn` and let the reactor recover. `_send_steal!` had no recovery to begin with: a failed steal left its items assigned to nobody, so `remaining_work` never emptied and the run hung; it now reports the same IO error its siblings do.

src/junit.jl

@@ -62,19 +62,21 @@ end
 _clean(s::AbstractString) = _xml_escape(_strip_ansi(s))
 
 """
-    _relative_path(uri, root)
+    _report_path(uri, root)::Union{Nothing,String}
 
 The file path of `uri` relative to `root`, with `/` separators so a Windows run and a
-Linux run of the same suite produce the same `classname`. Falls back to the URI itself
-when the path cannot be relativized (a non-`file:` URI, or a different Windows drive).
+Linux run of the same suite produce the same path. Returns `nothing` when `uri` is not a
+`file:` URI, leaving it to the caller to decide what an unusable entry becomes.
+
+Shared with the LCOV writer, which needs the same relativization for its `SF:` lines.
 """
-function _relative_path(uri::AbstractString, root::Union{Nothing,AbstractString})
+function _report_path(uri::AbstractString, root::Union{Nothing,AbstractString})
     path = try
         uri2filepath(uri)
     catch
         nothing
     end
-    path === nothing && return uri
+    path === nothing && return nothing
 
     if root !== nothing
         root_path = startswith(root, "file:") ? something(uri2filepath(root), root) : root
@@ -101,6 +103,15 @@ function _relative_path(uri::AbstractString, root::Union{Nothing,AbstractString}
     return replace(path, '\\' => '/')
 end
 
+"""
+    _relative_path(uri, root)
+
+As [`_report_path`](@ref), but falling back to the URI itself when the path cannot be
+derived — a `classname` has to say *something*.
+"""
+_relative_path(uri::AbstractString, root::Union{Nothing,AbstractString}) =
+    something(_report_path(uri, root), String(uri))
+
 # The discovery id, as recorded by whoever produced the result.
 #
 # This used to be reconstructed from `(uri, root)` and the name. That was wrong twice over:

src/lcov.jl

@@ -7,42 +7,48 @@ reaching into the vendored copy and having to know how our URIs map onto file pa
 """
 
 """
-    write_lcov(io_or_path, result::TestrunResult)
+    write_lcov(io_or_path, result::TestrunResult; root=nothing)
 
 Write the merged coverage of `result` in LCOV info format, the format `genhtml`,
 Codecov, Coveralls and friends consume.
 
 Returns `false` and writes nothing when the run collected no coverage — that is the normal
 outcome for a run that was not started in coverage mode, not an error.
 
-File URIs are converted back to absolute file paths, since LCOV consumers expect paths.
+`root` is a directory path or `file:` URI to relativize the `SF:` paths against; without
+it they are absolute. Coverage services match `SF:` paths against paths in the repository,
+and the absolute paths of a CI runner match nothing at all — which is one way a fully
+covered package comes out at 0%. A file outside `root` keeps its absolute path rather than
+a `..`-heavy one, the same choice [`write_junit_xml`](@ref) makes.
+
+Paths always use `/` separators, so a Windows leg and a Linux leg of the same matrix
+contribute the same file names to a merged report.
+
 Entries whose URI is not a `file:` URI are skipped.
 """
-function write_lcov(io::IO, result::TestrunResult)
-    fcs = _to_coverage_tools(result)
+function write_lcov(io::IO, result::TestrunResult; root::Union{Nothing,AbstractString}=nothing)
+    fcs = _to_coverage_tools(result, root)
     fcs === nothing && return false
     CoverageTools.LCOV.write(io, fcs)
     return true
 end
 
-function write_lcov(path::AbstractString, result::TestrunResult)
-    fcs = _to_coverage_tools(result)
+function write_lcov(path::AbstractString, result::TestrunResult; root::Union{Nothing,AbstractString}=nothing)
+    fcs = _to_coverage_tools(result, root)
     fcs === nothing && return false
     CoverageTools.LCOV.writefile(path, fcs)
     return true
 end
 
-function _to_coverage_tools(result::TestrunResult)
+function _to_coverage_tools(result::TestrunResult, root::Union{Nothing,AbstractString})
     result.coverage === nothing && return nothing
     isempty(result.coverage) && return nothing
 
     fcs = CoverageTools.FileCoverage[]
     for fc in result.coverage
-        filename = try
-            uri2filepath(fc.uri)
-        catch
-            nothing
-        end
+        # Shared with the JUnit writer: same relativization, same `/` separators, same
+        # refusal to walk out of the root with `..`.
+        filename = _report_path(fc.uri, root)
         filename === nothing && continue
         push!(fcs, CoverageTools.FileCoverage(filename, "", fc.coverage))
     end

test/test_coverage.jl

@@ -190,6 +190,37 @@ end
     end
 end
 
+@testitem "Coverage roots match whole path segments" setup=[TestHelpers] begin
+    if VERSION < v"1.11"
+        @test_skip "Coverage mode requires Julia 1.11+"
+    else
+        using TestItemControllers: filepath2uri
+
+        pkg_path = joinpath(TestHelpers.TESTDATA_DIR, "BasicPackage")
+        discovered = TestHelpers.discover_test_items(pkg_path)
+
+        passing_items = filter(i -> i.label == "add works", discovered.items)
+        @test length(passing_items) == 1
+
+        # A root is a folder URI with no trailing slash, so a plain prefix test also
+        # accepted anything whose name merely started with it: this truncated root used to
+        # collect the whole of `src`, the same way a root of `<workspace>/Foo` collected
+        # `<workspace>/Foo2`.
+        truncated_root = String(chop(filepath2uri(joinpath(pkg_path, "src"))))
+
+        result = TestHelpers.run_testrun(
+            passing_items, discovered.setups, discovered;
+            mode="Coverage",
+            coverage_root_uris=[truncated_root],
+            timeout=600
+        )
+
+        passed_events = filter(e -> e.event == :passed, result.events)
+        @test length(passed_events) == 1
+        @test result.coverage === nothing
+    end
+end
+
 @testitem "Coverage is identical across repeated runs" setup=[TestHelpers] begin
     if VERSION < v"1.11"
         @test_skip "Coverage mode requires Julia 1.11+"

test/test_junit.jl

@@ -287,3 +287,96 @@ end
         @test !occursin(replace(pkg, "\\" => "/"), xml)
     end
 end
+
+@testitem "LCOV export relativizes against a root" begin
+    using TestItemControllers: write_lcov, filepath2uri
+    using TestItemControllers.Results
+
+    # Coverage services match `SF:` paths against paths in the repository. The absolute
+    # paths of a CI runner match nothing at all, which is one way a fully covered package
+    # gets reported as 0%.
+    mktempdir() do dir
+        dir = realpath(dir)
+        pkg = joinpath(dir, "pkg")
+        mkpath(joinpath(pkg, "src"))
+        uri = string(filepath2uri(joinpath(pkg, "src", "f.jl")))
+
+        result = TestrunResult(
+            TestrunResultDefinitionError[],
+            TestrunResultTestitem[],
+            Dict{String,String}(),
+            [TestrunResultFileCoverage(uri, Union{Nothing,Int}[nothing, 3, 0])],
+        )
+
+        io = IOBuffer()
+        @test write_lcov(io, result; root=pkg) == true
+        lcov = String(take!(io))
+
+        @test occursin("SF:src/f.jl", lcov)
+        @test !occursin(replace(pkg, "\\" => "/"), lcov)
+
+        # ...and a relative root is resolved against the working directory, which `relpath`
+        # does not do on its own.
+        io = IOBuffer()
+        cd(dir) do
+            write_lcov(io, result; root="pkg")
+        end
+        @test occursin("SF:src/f.jl", String(take!(io)))
+    end
+end
+
+@testitem "LCOV export keeps files outside the root absolute" begin
+    using TestItemControllers: write_lcov, filepath2uri
+    using TestItemControllers.Results
+
+    # A `..`-heavy path means nothing to a coverage service, and dropping the record would
+    # make a stray file look like a coverage regression rather than a stray file.
+    mktempdir() do dir
+        dir = realpath(dir)
+        mkpath(joinpath(dir, "pkg"))
+        mkpath(joinpath(dir, "elsewhere"))
+        uri = string(filepath2uri(joinpath(dir, "elsewhere", "f.jl")))
+
+        result = TestrunResult(
+            TestrunResultDefinitionError[],
+            TestrunResultTestitem[],
+            Dict{String,String}(),
+            [TestrunResultFileCoverage(uri, Union{Nothing,Int}[1])],
+        )
+
+        io = IOBuffer()
+        @test write_lcov(io, result; root=joinpath(dir, "pkg")) == true
+        lcov = String(take!(io))
+
+        @test !occursin("SF:..", lcov)
+        # Lowercased because `filepath2uri` lowercases the Windows drive letter on the way
+        # in, and the round trip does not restore its case.
+        @test occursin(lowercase(replace(joinpath(dir, "elsewhere", "f.jl"), "\\" => "/")),
+            lowercase(lcov))
+    end
+end
+
+@testitem "LCOV export uses forward slashes and skips non-file URIs" begin
+    using TestItemControllers: write_lcov
+    using TestItemControllers.Results
+
+    # `uri2filepath` hands back a backslashed path on Windows, which no LCOV consumer
+    # recognizes — a Windows leg used to contribute nothing to a merged report.
+    result = TestrunResult(
+        TestrunResultDefinitionError[],
+        TestrunResultTestitem[],
+        Dict{String,String}(),
+        [
+            TestrunResultFileCoverage("untitled:Untitled-1", Union{Nothing,Int}[1]),
+            TestrunResultFileCoverage("file:///c%3A/pkg/src/f.jl", Union{Nothing,Int}[nothing, 3]),
+        ],
+    )
+
+    io = IOBuffer()
+    @test write_lcov(io, result) == true
+    lcov = String(take!(io))
+
+    @test !occursin("\\", lcov)
+    @test !occursin("Untitled-1", lcov)
+    @test occursin("SF:c:/pkg/src/f.jl", lcov)
+end

testprocess/TestItemServer/src/TestItemServer.jl

@@ -309,14 +309,21 @@
             rm(lcov_filename)
         end
 
-        # `roots === nothing` means the run put no restriction on which files to report —
-        # the CLI never sends `coverageRootUris`. Feeding that to `any` throws a
-        # `MethodError`, which the caller's `catch` turns into a spurious "errored" result
-        # for whatever test item happened to be running, so the restriction has to be
-        # skipped explicitly rather than left to short-circuit on an empty result.
+        # `roots === nothing` means the run put no restriction on which files to report,
+        # which is what a client that sends no `coverageRootUris` gets. Feeding that to
+        # `any` throws a `MethodError`, which the caller's `catch` turns into a spurious
+        # "errored" result for whatever test item happened to be running, so the
+        # restriction has to be skipped explicitly rather than left to short-circuit on an
+        # empty result.
         filter!(cov_info) do i
             isabspath(i.filename) || return false
-            roots !== nothing && !any(j -> startswith(filepath2uri(i.filename), j), roots) && return false
+            if roots !== nothing
+                uri = filepath2uri(i.filename)
+                # A root is a folder URI and `filepath2uri` never leaves a trailing slash,
+                # so a bare prefix test also accepts a sibling whose name merely starts
+                # with it — a root of `<workspace>/Foo` collecting `<workspace>/Foo2`.
+                any(j -> startswith(uri, endswith(j, "/") ? j : j * "/"), roots) || return false
+            end
             return isfile(i.filename)
         end
 
@@ -644,7 +651,7 @@
         setup_details = state.test_setups[(params.packageUri, Symbol(i))]
 
         if setup_details.kind==:module && !setup_details.evaled
             mod = Core.eval(Main.Testsetups, :(module $(Symbol(i)) end))
 
             code = string('\n'^(setup_details.line-1), ' '^(setup_details.column-1), setup_details.code)