A unit that cannot record its own error must not take the shard with it

Project.toml

@@ -1,6 +1,6 @@
 name = "TestShards"
 uuid = "acceef1d-f5e0-4fe4-a546-818dc56ce7b2"
-version = "0.3.41"
+version = "0.3.42"
 authors = ["sota shimozono <shimozono-sota631@g.ecc.u-tokyo.ac.jp>"]
 
 [deps]

src/provider.jl

@@ -122,10 +122,21 @@ _duration(ts) =
         0.0
     end
 
+# Julia 1.13 gave `DefaultTestSet` a `results_lock` and takes it on every `record`. Take it on
+# the read too, and copy rather than iterate under it, since `_section` recurses. A unit that
+# left a task running — which `@shard` forbids — would otherwise have this walk a vector being
+# pushed to.
+_results_snapshot(ts::Test.DefaultTestSet) =
+    if hasfield(Test.DefaultTestSet, :results_lock)
+        @lock ts.results_lock copy(ts.results)
+    else
+        ts.results
+    end
+
 function _section(ctx::ShardContext, ts::Test.DefaultTestSet)
     npass = nfail = nerror = nbroken = 0
     kids = Section[]
-    for r in ts.results
+    for r in _results_snapshot(ts)
         if r isa Test.DefaultTestSet
             s = _section(ctx, r)
             push!(kids, s)

src/run.jl

@@ -31,6 +31,47 @@ function _key(ctx::ShardContext, path::AbstractString)
     return replace(relpath(abspath(path), ctx.root), '\\' => '/')
 end
 
+# Record a unit's escaping error on its testset, and say whether that worked.
+#
+# `_run` catches so the shard survives a unit that throws. Recording must not undo that: a
+# provider's testset type with no `Test.record` for `Test.Error` makes the record call itself
+# throw, and before this that MethodError escaped `_run` — the unit went unrecorded, every unit
+# after it never ran, and the failure surfaced as a TestShards internal rather than as the unit
+# that could not load.
+#
+# Tested by doing it, not by `hasmethod`: a catch-all `record(ts, ::Any)` that throws for
+# `Test.Error` satisfies `hasmethod` and still fails here.
+function _record_unit_error(ts, key, err)
+    try
+        Test.record(
+            ts,
+            # XXX: `Base.catch_stack` is internal; `Base.current_exceptions()` is
+            # the public spelling — see `docs/src/testset-internals.md`.
+            Test.Error(
+                :nontest_error, Expr(:tuple), err, Base.catch_stack(), LineNumberNode(0)
+            ),
+        )
+        return true
+    catch record_err
+        @error "TestShards: $(key) threw, and its testset could not record the error. The \
+                unit is counted as errored and the shard continues." unit_error = err record_err
+        return false
+    end
+end
+
+function _plus_one_error(s::Section)
+    return Section(
+        s.name,
+        s.duration,
+        s.npass,
+        s.nfail,
+        s.nerror + 1,
+        s.nbroken,
+        s.evidence,
+        s.sections,
+    )
+end
+
 """
     _run(ctx, key, body)
 
@@ -50,28 +91,25 @@ function _run(ctx::ShardContext, key::AbstractString, body)
 
     ts = _unit_testset(key)
     t0 = time()
+    recorded = true
     _with_testset(ts) do
         try
             body()
         catch err
             # An error escaping the unit (a load error, say) is recorded as the unit's error
             # rather than aborting the shard, so the remaining units still run and still get
             # recorded.
-            Test.record(
-                ts,
-                # XXX: `Base.catch_stack` is internal; `Base.current_exceptions()` is
-                # the public spelling — see `docs/src/testset-internals.md`.
-                Test.Error(
-                    :nontest_error, Expr(:tuple), err, Base.catch_stack(), LineNumberNode(0)
-                ),
-            )
+            recorded = _record_unit_error(ts, key, err)
         end
     end
     # A tool that attaches a finished testset to its parent can only do it now that the parent is
     # current again — which is the whole reason a hand-entered testset needs this second step.
     _unit_close(ts)
     dt = time() - t0
     sec = unit_fold(ctx, ts)
+    # The unit failed; its testset just could not say so. Count it anyway, or the shard
+    # reports green for a unit that threw.
+    recorded || (sec = _plus_one_error(sec))
     push!(
         ctx.records,
         UnitRecord(

test/core/helpers.jl

@@ -9,7 +9,7 @@ using TestShards
 export make_suite, run_suite, unit_keys
 export shared_suite, whole_units, partition_check, shard_runs_clean
 export bare_context, shard_window, lcov_trace
-export StubSet, stub_fold, with_provider
+export StubSet, PartialSet, stub_fold, partial_fold, with_provider
 
 const PROJ = dirname(Base.active_project())
 
@@ -223,6 +223,21 @@ mutable struct StubSet <: Test.AbstractTestSet
 end
 StubSet(desc::AbstractString) = StubSet(String(desc), 0, 0, 0, 0, false)
 
+"""
+A provider testset that records `Pass`/`Fail`/`Broken` but NOT `Error` — the shape a provider
+author naturally ends up with, since only a THROWING unit reaches the error branch.
+"""
+mutable struct PartialSet <: Test.AbstractTestSet
+    description::String
+    n::Int
+end
+PartialSet(desc::AbstractString) = PartialSet(String(desc), 0)
+Test.record(ts::PartialSet, r::Test.Pass) = (ts.n += 1; r)
+Test.record(ts::PartialSet, r::Test.Fail) = (ts.n += 1; r)
+Test.record(ts::PartialSet, r::Test.Broken) = (ts.n += 1; r)
+Test.finish(ts::PartialSet) = ts
+partial_fold(ts::PartialSet) = (; npass=ts.n, nfail=0, nerror=0, nbroken=0, sections=())
+
 function Test.record(ts::StubSet, res)
     res isa Test.Pass && (ts.npass += 1)
     res isa Test.Fail && (ts.nfail += 1)

test/core/test_provider.jl

@@ -5,7 +5,7 @@ isdefined(Main, :TSHelpers) || include(joinpath(@__DIR__, "helpers.jl"))
 using .TSHelpers
 
 # The unit-testset provider seam, exercised with a STUB provider — no other package involved. The
-# contract is what matters here: the type a unit runs in, the second step a hand-popped testset
+# contract is what matters here: the type a unit runs in, the second step a hand-entered testset
 # needs, and the fold that keeps the counts right when the testset is not ours. The real consumer
 # (Pinax) is tested end to end in `test_pinax.jl`.
 
@@ -120,3 +120,22 @@ end
         TestShards.UNIT_PROVIDER[] = saved
     end
 end
+
+@testset "a provider that cannot record an Error does not truncate the shard" begin
+    # The only way to reach `_run`'s Error branch is a unit whose body THROWS — not an
+    # ordinary `@test` failure — so a provider author testing assertions has no reason to
+    # cover it. Before, the resulting `MethodError` escaped `_run`: the unit went
+    # unrecorded, every unit after it never ran, and the failure read as a TestShards
+    # internal rather than as the unit that could not load.
+    ctx = bare_context(; shard="", nshards=1)
+    ran = String[]
+    with_provider(; open=k -> PartialSet(k), fold=partial_fold) do
+        TestShards._run(ctx, "u1.jl", () -> (push!(ran, "u1"); @test true))
+        TestShards._run(ctx, "u2.jl", () -> (push!(ran, "u2"); error("a load error")))
+        TestShards._run(ctx, "u3.jl", () -> (push!(ran, "u3"); @test true))
+    end
+    @test ran == ["u1", "u2", "u3"]                      # the shard was not truncated
+    @test [r.key for r in ctx.records] == ["u1.jl", "u2.jl", "u3.jl"]
+    u2 = only(filter(r -> r.key == "u2.jl", ctx.records))
+    @test u2.nerror >= 1                                 # ...and the unit that threw is NOT green
+end