Skip to content

fix(runner): report skipped concurrent instances as skips, not passes - #77

Merged
Drownek merged 5 commits into
Drownek:masterfrom
monikon22:fix/concurrency-instance-skips
Sep 13, 2026
Merged

Drownek merged 5 commits into
Drownek:masterfrom
monikon22:fix/concurrency-instance-skips

Conversation

@monikon22

Copy link
Copy Markdown
Contributor

Under concurrency: N, a describe.serial block that stops early on some of its
instances reported the positions those instances never reached as passes. Below
is the shape of a real 10-bot run, with the plugin's names replaced:

  FAIL  Claims > rejects a claim on an already claimed chunk   5.0s [7/10]
    10 instances: min 0ms / avg 1.5s / max 5.0s
      - [4/10] ?: OK (0ms)
      - [5/10] pw_418a: FAIL (5.0s) Expected message matching "Already claimed" not received
      - [6/10] ?: OK (0ms)

Instances 4 and 6 had stopped at an earlier test in the block and never ran this
one. They still counted toward 7/10, printed as OK (0ms), dragged the min
down to zero, and had no name to show. The summary said Skipped: 0 for the
whole run.

What changed

TestInstanceResult gains skipped and skipReason, and aggregateInstances
copies them off the per-instance TestResult instead of dropping them. Skip rows
from a serial block now carry the bot's username too — that's where the ? came
from.

The reporter reads the flag:

  FAIL  Claims > rejects a claim on an already claimed chunk   5.0s [5/8, 2 skipped]
    8 instances ran, 2 skipped: min 50ms / avg 1.9s / max 5.0s
      - [4/10] pw_7e4e: SKIP serial block "Claims" stopped at "grants the claim permission"
      - [5/10] pw_418a: FAIL (5.0s) Expected message matching "Already claimed" not received
      - [6/10] pw_d411: SKIP serial block "Claims" stopped at "grants the claim permission"

The ratio is over the instances that ran. Duration statistics skip the zeros. A
skipped instance shows its stop reason and no duration, because (0ms) next to a
skip is the exact thing that read as an instant pass.

A test row only counts as skipped when every instance skipped it, so
Skipped: alone can't show this. The console summary and
summary.skippedInstances in the JSON report now carry the instance count.

Also here

toHaveReceivedMessage failures quote the last 10 lines of the buffer they
searched, or say outright that nothing arrived. "Expected message matching
Already claimed not received" reads identically whether the server answered
something else or stayed silent, and on a live server that is the difference
between a throttled command and a plugin bug.

Verification

npm run typecheck and npm run build in runner-package. Reporter output
checked against a synthetic result reproducing the run above; JSON report
inspected for the new fields. No behavioural change to test execution — this
touches how results are recorded and printed, nothing about what runs.

A serial block instance that stopped early reported its remaining tests as
`passed: true, durationMs: 0`, and `aggregateInstances` copied that into the
aggregate's `instances` array verbatim. A skip was then indistinguishable from
an instant pass for every consumer of that array. Skipped instances also lost
their bot's name, which the reporter renders as "?".

Adds `skipped`/`skipReason` to `TestInstanceResult` and sets `botUsername` on
skip rows. Rendering is unchanged here; see the follow-up.
The per-test instance ratio counted skipped instances as passes, so a block
that stopped early on two of ten bots still printed "[8/10]" with two of those
eight never having run the test. The detail list printed the same instances as
"OK (0ms)" under a "?" label, and their zeros pulled the min/avg duration down.

The ratio is now over the instances that ran, with the skip count alongside it;
skipped instances render as SKIP with their stop reason and no duration. The
JSON report carries `skipped`/`skipReason` per instance.
A test row only counts as skipped when every one of its concurrent instances
skipped it, so a run where several bots stopped their block early still printed
"Skipped: 0" with nothing anywhere saying otherwise.

The console summary now notes how many concurrent instances were skipped next
to the test count, and the JSON report carries the same number as
`summary.skippedInstances`.
"Expected message matching X not received" reads the same whether the server
answered something else or said nothing at all, which is the whole question
when a test fails on some concurrent instances and passes on others.

The failure now appends the last 10 lines of the buffer it searched, or says
outright that nothing was received.
Covers the new `skipped`/`skipReason` fields on each concurrent instance,
`summary.skippedInstances`, and why a skipped instance's `passed: true` must
not be read as a pass.

@Drownek Drownek left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Great catch, clean implementation.

What I checked:

  • Instance reporting: propagating skipped and skipReason through aggregateInstances fixes the false-positive pass counts. [7/10] now shows as [5/8, 2 skipped], which is correct.
  • Duration stats: ranInstances() keeps (0ms) skipped placeholders out of the min/avg calculations, so they no longer drag the numbers down.
  • Bot attribution: passing botUsername on skipped serial positions gets rid of the anonymous ? in failure traces.
  • Assertion diagnostics: quoting the trailing 10 lines of the message buffer in toHaveReceivedMessage (or saying outright that nothing was received) makes it way easier to tell whether a command got throttled or the plugin just replied with something unexpected. Good call.
  • JSON reporter schema stays backwards compatible — summary.skippedInstances is purely additive.

Ran it locally across a few concurrent runs, no issues.

LGTM

@Drownek
Drownek merged commit e5fc00c into Drownek:master Sep 13, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants