feat(core): give every test a full address, and report by it - #271
Open
roxblnfk wants to merge 25 commits into
Open
feat(core): give every test a full address, and report by it#271roxblnfk wants to merge 25 commits into
roxblnfk wants to merge 25 commits into
Conversation
Concurrent tests interleave their lifecycle events and output on one stream, and reporters had no way to tell whose output is whose. Introduce a TestIdentity DTO (a random numeric id for now) carried by TestInfo, and thread it to the messenger so every MessageReceived is stamped with the identity of the test it belongs to. - TestIdentity: new @api DTO; TestInfo mints one when none is supplied and keeps it across with(). - MessageReceived gains an optional ?TestIdentity (null for messages that belong to no test). - Messenger::scope() takes an optional identity; MessengerHub binds it on the per-test State, which stamps it on each dispatched MessageReceived and passes it down to forks. OutputInterceptor supplies $info->identity. Because the State is fiber-local, an interleaving test dispatches from its own State and carries its own identity — attribution is settled at the source, not guessed from ordering downstream. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Under interleaving execution (fibers, a Revolt loop) several tests are in flight at once, and the TeamCity reporter kept a single "current test" scalar — so a sibling starting mid-test clobbered attribution and interleaved testStarted / testStdOut / testFinished collapsed into garbage. Key each reporter's per-test state (pending start, current name, in-batch flag) by TestIdentity::$id instead of a single scalar, attribute streamed output via the identity now carried on MessageReceived, and stamp a flowId (the identity id) on every test- and batch-level service message. TeamCity/IDEs use flowId to demultiplex parallel flows, so interleaved tests are tracked independently. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adapted from the bridge-revolt branch, where the same sandbox ran on a Revolt loop. 1.x has no event-loop bridge, so it uses `testo/fiber`'s cooperative scheduler (`#[RunInFiber(Schedule::RoundRobin)]`) instead: every test takes one work-and-yield step per round, so their output interleaves, and step counts differ per test so they finish in a staggered order — handy for eyeballing concurrent output and the per-test `flowId` attribution. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`TestInfo::$identity` and `MessageReceived::$identity` are public surface a reporter needs: per-test state must be keyed by the identity id, not a single "current test" field, or concurrent tests clobber each other's attribution. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The terminal reporter ignored the identity now carried on MessageReceived: it streamed every message through one channel-grouping state, so interleaving tests (fibers, an event loop) collapsed into a single `[stdout]` block and no line could be traced to its test. It also keyed the DataProvider-batch guard by `spl_object_hash($testDefinition)` — a definition object address, not a running test. - ChannelRenderer groups by *owner + channel* instead of channel alone, so a switch of owner opens a fresh header instead of appending to a stranger's block, and takes an optional owner label to name it. - TerminalPlugin tracks the in-flight tests by TestIdentity::$id and passes the id as the owner. It passes the *label* only while more than one test is in flight, so a sequential run keeps the plain `[channel] time` header byte-for-byte — an unlabelled block therefore means exactly one test was running when it opened. - reset() no longer claims the cursor sits at line start: it forgets which header is open, not how far the previous content got, so a reset mid-line (much more likely once tests interleave) can't glue the next header onto a partial line. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The guard was keyed by `spl_object_hash($testInfo->testDefinition)` — the address of an object shared by every run of one test method, not the running test. Two tests in flight over the same definition collide: the first pipeline's finish clears the marker, so the second falls through and emits a rolled-up `<testcase>` on top of its per-dataset ones. Keying by TestIdentity::$id makes the marker per running test, matching TeamcityPlugin. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`$currentTestName` and `$currentIndentLevel` were single slots holding "the test being reported right now". Under interleaving that is not a thing: the test entering a batch sets the indent for everyone, and the batch announcing its data set renames whatever test reports next. Key both by TestIdentity::$id, as TerminalPlugin already does. Reachable today only via `#[RunInFiber(RoundRobin)]` on a case whose tests use data providers, but it is also the precondition for buffering a test's block: buffering fixes where a line lands, not what it says. The DataProvider-description test built its two data sets from separate TestInfo objects, which the runtime never does — data sets are derived with `TestInfo::with()` and share the batch's identity. Fixture corrected to match. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A line-oriented report cannot hold two open groups: a test's header, its data sets and its result line only mean anything adjacent. So when tests interleave, exactly one writes live and the rest accumulate a block that goes out whole once the stream frees. The lease is taken by the first write and held until close() — not until the writer pauses, since a group must stay contiguous. Releasing it drains the blocks that finished meanwhile, in finish order. Two flat arrays carry the state: open blocks keyed by test id, and a queue of finished ones (ids dropped — only order matters, and each block ends with the result line naming its test). A write after close opens a new block on the same terms rather than reopening the old one. flush() also drains blocks of tests that never closed: a hung or aborted test still produced output, and losing it is worse than printing a block with no result line. No caller yet — wiring TerminalLogger onto it is the next step. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A line-oriented tree cannot hold two open groups. Under interleaving the reporter emitted each line the moment it arrived, so a batch header, its data sets and its result line ended up shuffled with another test's — two trees spliced into nonsense. The logger now writes through SharedStream: every line carries the id of the test it belongs to, one test streams live and the rest accumulate a block released by closeTest() at TestPipelineFinished, after the result line. Report structure that belongs to no test (suite header, case footer, summary) writes through, and case/session boundaries flush. Channel grouping follows the same lifetime — one ChannelRenderer per block, opened on the test's first write and dropped on close — so a block always opens with a fresh header and no test inherits another's channel state. That makes the owner label added in d79b028 pointless: a block ends with the result line that names it, and the plugin no longer passes one. reset() goes back to assuming the caller left the cursor at line start. The renderer only sees its own content, and data sets share the batch's block, so a result line written past it left it thinking mid-line and inserted a blank line between data sets. Line alignment across blocks is SharedStream's job, which does see every byte. Verified byte-identical sequential output against 1.x on the sandbox suite. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both were added in d79b028 to keep interleaved output attributable while every test wrote to the stream as it went. Blocks made them unreachable: each block has its own renderer, so there is never a second owner to group apart from, and a block ends with the result line that names its test, so there is nothing for a label to disambiguate. Their concern is still covered — by SharedStreamTest for the separation and TerminalPluginTest for the attribution. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The hard case for a line-oriented report is not two tests streaming text — it is two open tree nodes. Two of the sandbox's tests now nest data sets under a batch node while yielding to each other, so the shape the reporter has to keep intact is visible by eye. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The note added alongside the labelling attempt described a channel header naming its test. Blocks replaced that: the terminal keeps each test's whole tree contiguous and releases it in finish order. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TestIdentity used to be a bare counter. Reporters keyed their per-test state on
`spl_object_hash($testDefinition)` and kept a single "current test" scalar, which
concurrent tests clobber, and nothing in the run could say *which* test an event
or a captured line belonged to in terms a human or a filter would recognise.
Identity is now the address itself: suite, case, type, test, and the data set
coordinates. Each level is its own type declaring exactly the fields it has —
`SuiteIdentity { suite }`, `CaseIdentity { suite, case, type }`,
`TestIdentity { suite, case, type, test, ?provider, ?dataSet }` — so no address
carries a field that does not apply to it. The fields are plain scalars rather
than a reference to the level above, so reading one never walks a chain; step
down with `toCase()`, `toTestIdentity()`, `with()`.
The counter stays as `randomId` on the shared base, minted from a process-wide
`RuntimeSequence` so a suite, a case and a test never collide. It answers a
different question than the address does: *which run of this node is in flight*.
Repeats, retries and the data sets of one batch share it, which is what keeps a
batch and its data sets in one report block and one TeamCity flow.
A data set is addressed by index, not by key: provider keys may repeat, so only
the index tells two data sets apart. Half an address is rejected outright.
Suite and case contexts now carry their address too, and `CaseInfo::with()`
clones instead of rebuilding — reconstructing would mint a second address for a
case the tests already descended from.
Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rage reports
Identity named a test but could not locate it, so three reporters each rebuilt
the same address from reflection — and TeamCity got it wrong. Its `locationHint`
named a data set by the provider *key* rather than the index, in reverse order,
dropping the data set index entirely. Four data sets of one provider that repeats
a key collided on a single hint, and the string it emitted selected nothing when
pasted back into `--filter`:
--filter='DataCoordinatesTest::coordinates:9:0' → 1 test, 0 assertions, risky
The filter has always taken `Class::method:<provider>:<dataSet>`, both by index.
Identity now answers exactly that, so the hint is the address with `php_qn://`
and the file in front of it. Five data sets get five hints, each of which runs
the data set it points at.
To get there Identity had to carry where the code *is*, not just what it is
called: the file the case was read from, and the namespace of a free test
function — which has no class to qualify it. `CaseInfo` normalises the file to
one spelling, since a located path and a reflected filename disagree on the
separator and the field would otherwise name one file two ways.
Both forms are composed in the constructor now. `fqn()` is the filter string;
`qualifiedName()` is the same without the coordinates, which is what consumers
that group by test method need — coverage `<covered by>` and the JUnit
`classname` are joined on it by Infection, so per-data-set granularity there
would break the lookup rather than sharpen it.
Two reflection walks fell out along the way. `Formatter` no longer needs the
`caseReflection` override that attributed an inherited `#[Test]` to the concrete
subclass instead of the abstract base it is declared on — an address is minted
from the case as found, so it never named the base. `TeamcityPlugin` no longer
assembles a location suffix: a data set's info already carries its own address.
The `locationHint` was pinned by no test at all, which is how the collision
survived; it is now. Three fixtures had to be corrected rather than accommodated:
they passed a test name that disagreed with their own reflection, a shape
discovery cannot produce, since it keys a test by its short name and the runner
passes that key through untouched.
Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Every part of an address that a case is always reached through is required now,
and the parts that only ever appeared together live in one field.
`CaseInfo` took an optional suite and stood a case on `SuiteIdentity('undefined')`
when none came; `CaseDefinition` took an optional file and `CaseInfo` normalised
whatever it could find, since a located path and a reflected filename disagree on
the separator. Both are required, the file is an `Internal\Path` that normalises
itself, and the placeholder and the normaliser are both gone.
`TestIdentity` carried `test` plus a `namespace` that was only ever set when
`case` was null — an invariant a docblock asked callers to respect. The two are
one field: `test` is the name *relative to* `case`, so a method keeps its bare
name and a free function, having no class to be relative to, carries its own FQN.
"A class and a namespace" is no longer a state the type can be put in, and
composing the qualified name drops from three branches to one.
Two `__toString()` branches went with the file becoming non-nullable: "neither a
class nor a file" is unreachable, so nothing has to render a node with no name.
The fixtures had to follow, since none of this has a default to fall back on any
more. Each now names the suite it belongs to and the file its case was read from
rather than leaning on a stand-in.
Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ithmetic
`headerColorIsNameDerivedFromTheFullEightColorPalette` pinned `stderr` to cyan,
which is not a claim about behaviour — it restates `abs(crc32("stderr")) % 8 == 1`
and so depends on the palette's order as much as its size. Reordering the colors
changes nothing a reader would notice and broke it; the comment admitted as much
by explaining which mutant it was there to kill. It also failed in CI, having
never decided whether ANSI would be emitted at all: `Style` keeps colorization in
a process-global flag that three neighbouring output cases switch off for their
own assertions, so the result hung on the order the finder walked files in —
`Rendering/` lands before `Terminal/` on one platform and after it on another.
Two properties replace it, both indifferent to order:
- a channel always renders in the same color, which is what lets a reader follow
one channel down a report;
- every color of the palette is reachable across enough channel names, stated as
a set — so dropping one still fails, and black stays excluded on purpose.
Checked against the code: removing a color fails the second test, reordering the
palette fails neither, and the old test failed on reordering.
Inspecting ANSI at all means owning that global flag, so the case now sets it and
restores what it found. The three neighbours were restoring a hardcoded `true`
rather than the previous value — right only because it matches today's default —
and now save and restore too.
Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The palette assertions inspect ANSI, but nothing in them decided whether ANSI would be emitted. Colorization lives in a process-global flag and building a `TerminalPlugin` writes to it — `TerminalPluginTest` built one with `ColorMode::Never` and left it that way, so every case running afterwards rendered plain. Which cases those are depends on the order the finder walks files in: `Rendering/` lands before `Terminal/` on one platform and after it on another, which is why this passed here and failed on the CI runner. That test puts the flag back now, and the palette assertions turn it on for the render they inspect rather than trusting what they inherit, so neither depends on the other running first. Four classes carried `setUp`/`tearDown` that switched colorization off for their assertions. Testo has no such convention — its hooks are the `#[BeforeTest]` / `#[AfterTest]` attributes — so those methods never ran and the assertions were passing against colorized output all along. Removed rather than wired up: they were describing a precaution that was not being taken. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…e test's `TestIdentity::with()` bypassed the constructor to copy the batch's `runtimeId` onto the data set it derived, so a data set had no run of its own and every consumer grouped by a number that was shared by accident — while `with()` had to reach past the very constructor that composes the address. A data set now mints its own `runtimeId`, and `pipelineId` — the run of the test an address belongs to — carries over instead. That is what the terminal block, the channel grouping and the TeamCity flow key on, so a batch still reports as one node; the address keeps telling data sets apart by its coordinates, as before. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A consumer that reports a tree rather than a stream — PhpStorm wants `nodeId`/`parentNodeId` — needs each node's parent, and only one of those links was reachable: a data set found its test through `pipelineId`, a test its case through `TestInfo::$caseInfo`, but a case had no way back to its suite. The gap left the tree to be inferred from the order events arrive in, which concurrent tests break by construction: interleaved batches nest one inside the other. `Identity::$parentId` closes it as a plain number on the base, so the two fields a tree needs are read the same way at every level. It stays a number rather than a reference: no level holds the one above it, and reading an address never walks a chain of objects. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reporter named nodes only by name and left their nesting to the order messages happen to arrive in. That holds for a sequential runner and breaks for Testo: tests run concurrently, two nodes are open at once, and a consumer nesting by "whatever opened last" puts one test's node inside its neighbour's — PhpStorm showed a second data-provider batch inside the first, with neither ever closing. Every message now carries `nodeId`/`parentNodeId` off the address, so the tree is stated outright and order stops mattering. `flowId` stays what it was — TeamCity's own grouping, and only for tests, since suites and cases of one process never overlap. Suites and cases had no identifier at all before and now have nodes too, which is what lets a case name its suite. The message builders take the address instead of a pre-computed flow id — fewer parameters, and one place (`Formatter::placement()`) that decides where a message belongs. Assisted-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A caller of run() without an identity got data sets that shared the batch's runtimeId, colliding with the batch node in the TeamCity tree. Nothing outside the interceptor calls it, so the parameter is required and the method private. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Entries in the per-test maps are removed when the test's pipeline finishes, so a test the runner never finished — a hang, an abort — left its entry behind for the life of the session. No test spans a case boundary, so everything still tracked there is exactly that leftover; SharedStream already flushes on the same reasoning. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Symmetry with SuiteIdentity::toCase(): every step-down is now to<Level>(). Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Thirteen identical labels read as a copy-paste accident; the volume is the point, so keep the count and vary the letter. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
…side scope() with no identity minted a state stamped null, so a plugin isolating output mid-test silently stripped attribution from everything written inside: TeamCity dropped those messages, the terminal wrote them through unowned. A fork already inherits its parent's identity for exactly this reason — now a scope does too, and null means "inherit" rather than "no test". Between tests the ambient identity is already null, so nothing changes there; the explicit argument still wins, which is how the per-data-set scope narrows the batch's address. Assisted-By: Claude Fable 5 <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.
What was changed
Identitygrew from a bare counter into the full address of a test — suite, case class, type, source file, test name, the namespace of a free function, and the coordinates of a data set — split acrossSuiteIdentity/CaseIdentity/TestIdentity, one type per level.Alongside the address, each level carries three process-local numbers that answer different questions:
runtimeId— which run is this, one per node, so a data set has its own;pipelineId— which test run is it part of, which a data set inherits from its batch;parentId— whose child is it, suite ← case ← test ← data set.Reporters now key their per-test state on those numbers instead of a single "current test" field, and a new
SharedStreamholds back a test's lines until it can print them as one contiguous block. TeamCity, JUnit and coverage stopped rebuilding identifiers from reflection and read them off the address, which also gave TeamCity'slocationHintthe same shape--filteraccepts.TeamCity output additionally states its tree outright — every message carries
nodeId/parentNodeId— rather than leaving nesting to the order messages arrive in.See commit history for details.
Why?
Tests that run concurrently — fibers today, an event loop later — interleave their events and output. A scalar "current test" and
spl_object_hash($testDefinition)cannot tell two live tests apart, so their report lines spliced into each other and data-set trees came out shuffled. Nothing in a run could name which test an event or a captured line belonged to in terms a human or a filter would recognise.The three reporters that needed such a name each derived it themselves, and disagreed. TeamCity was outright broken: it addressed a data set by the provider key instead of the index, with the two fields reversed and the data-set index dropped, so data sets of a provider that repeats a key collided on one hint — and the string it emitted selected nothing when pasted back into
--filter. One address, minted once where the test is discovered, removes the duplication and the disagreement.The same interleaving broke the IDE's test tree, for a reason no address alone fixes: a consumer that nests by "whatever opened last" put one concurrent batch's node inside its neighbour's, and neither ever closed. Naming the parent explicitly is the only form that survives concurrency — hence
parentIdand the node ids on the wire. The IDE side of that is j-plugins/testo-plugin#71, which needs a release of this branch to work.Checklist
🤖 Generated with Claude Code