A unit that cannot record its own error must not take the shard with it - #91
Merged
Merged
Conversation
`_run` catches an escaping error so the shard survives a unit that throws.
Recording that error undid it. A provider testset type with no
`Test.record` for `Test.Error` — the natural gap, since only a THROWING
unit reaches that branch, not an ordinary `@test` failure — makes the
record call itself throw, and the `MethodError` escaped `_run`.
Reproduced before fixing, three units, the middle one throwing:
before [ok] u1.jl
ESCAPED after [u1, u2] → MethodError: no method matching
record(::PartialSet, ::Test.Error)
units recorded = ["u1.jl"]
after [ok] u1.jl
Error: u2.jl threw, and its testset could not record the error
[FAIL] u2.jl (0 pass, 0 fail, 1 error)
[ok] u3.jl
units recorded = ["u1.jl", "u2.jl", "u3.jl"]
u3 never ran, and the failure surfaced as a TestShards internal rather
than as the unit that could not load.
Counting the unit as errored when recording fails is the other half, and
the more important one: swallow the record failure alone and the unit
that THREW reports `0 error` and reads green — worse than the truncation.
Tested by recording, not by `hasmethod`. A catch-all
`record(ts, ::Any)` that throws for `Test.Error` satisfies `hasmethod`
and still fails here, so the proxy would license exactly the case it
claims to exclude.
Second, unrelated to that: `_section` walked `ts.results` unlocked.
Julia 1.13 gave `DefaultTestSet` a `results_lock` and takes it on every
`record`; the read side now takes it too, and copies rather than
iterating under it, since `_section` recurses. Only reachable from a unit
that left a task running, which `@shard`'s docstring forbids — but the
forbidden case should be a wrong count, not a walk over a vector being
pushed to.
Also the "hand-popped" left in test_provider.jl's header, missed when
#90 renamed the same phrase everywhere else.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
📚 Docs preview: https://qatlashub.github.io/TestShards.jl/previews/PR91/ (updates on each push to this PR) |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
The suite runs inside a `@shard` block, and `_begin` installs into `TestShards.CURRENT` — so calling it from a test replaces the running block's own context. Every Julia version went red on the same line. `bare_context` exists for exactly this and says so in its docstring; the two testsets directly above use it. `test_claim.jl` already pins that `_begin` from inside is an error. I had all three and still reached for the driver under test. `shard=""` makes `_owns` total, so all three units run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A unit that cannot record its own error takes the shard with it
_runcatches an escaping error so the shard survives a unit that throws — that is whatthe
catchis for. Recording that error undid it.A provider testset type with no
Test.recordforTest.Errormakes the record callitself throw. This is the natural gap, not an exotic one: the only way to reach that
branch is a unit whose body throws (a load error), never an ordinary
@testfailure, soa provider author testing assertions has no reason to cover it. The repo's own
StubSethappens to use a catch-all
record, which is why nothing here caught it.Reproduced before fixing — three units, the middle one throwing:
u3never ran, and what surfaced was a TestShards internal rather than the unit thatcould not load.
Counting it as errored is the other half — and the more important one
Swallow the record failure alone and the unit that threw reports
0 errorand readsgreen. That is worse than the truncation: a truncated shard is at least loud. So
_runtracks whether recording worked and adds the error itself when it did not.Why not
hasmethodA registration-time
hasmethod(Test.record, (T, Test.Error))gate looks like the obviousfix and is the wrong instrument: a catch-all
record(ts, ::Any)that throws forTest.Errorsatisfieshasmethodand still fails here. The proxy would license exactlythe case it claims to exclude. This tests the real thing — try to record, and report
whether it worked.
Second, unrelated: the read side of
resultswas unlocked_sectionwalkedts.resultsdirectly. Julia 1.13 gaveDefaultTestSetaresults_lockand takes it on every
record; the read now takes it too, and copies rather thaniterating under it, since
_sectionrecurses.Only reachable from a unit that left a task running, which
@shard's docstring forbids —but the forbidden case should cost a wrong count, not a walk over a vector being pushed to.
Gated on
hasfield, so 1.10–1.12 are untouched.Also
The
hand-poppedleft intest_provider.jl's header, missed when #90 renamed that phraseeverywhere else.
Version
0.3.41→0.3.42, patch.🤖 Generated with Claude Code