test: surface OpenCode harness output on assertion failure - #2885
Open
karotkriss wants to merge 1 commit into
Open
test: surface OpenCode harness output on assertion failure#2885karotkriss wants to merge 1 commit into
karotkriss wants to merge 1 commit into
Conversation
…tion fails This case drives a real node harness that prints the reason it failed to stderr, but the assertion reported only the exit status, so a failure read as "expected exit 0, got 1" with no indication of the cause. Before: not ok - ...owned arm: expected exit 0, got 1 After: not ok - ...owned arm (exit 1): turn-end guard was suppressed by an external healthy watcher Diagnostics only - the assertion's pass and fail conditions are unchanged, and this adopts the `|| fail "...: $out"` form already used throughout this file.
Confidence Score: 5/5The PR appears safe to merge because it changes only failure reporting while preserving the test’s existing pass and fail conditions. The new conditional is equivalent to the prior zero-status assertion, and the captured output is surfaced only on the existing failure path. Reviews (1): Last reviewed commit: "test: surface harness output when the Op..." | Re-trigger Greptile |
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
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.
Intent
Improve the failure diagnostics of one existing test case in tests/fm-pi-watch-extension.test.sh: test_opencode_healthy_arm_output_does_not_suppress_guard.
Background on why this is being raised at all. This one-line change was originally carried, unrelated, inside a session-lock pull request (#2839) alongside a functional change to .opencode/plugins/fm-primary-watch-arm.js. Reviewers correctly flagged that whole hunk as out of scope for that issue, and it has been removed from that branch. The functional plugin part is deliberately NOT being re-filed: investigation could not substantiate the race it claimed to fix. Instrumenting settleReadiness in the real plugin against this very test fixture showed readiness is always settled by observeArmOutput on the streamed-output path and never by the close handler, across 60 sequential runs and 8 concurrent runs under a load average above 20, which matches Node's contract that a child's close event is emitted only after its stdio streams have ended. The genuine load-sensitive flake in this test file is a PID-recycling issue in the session-transition case, which pull request #2845 already fixes with a per-arm lifetime marker. Only the diagnostic improvement below is worth keeping, so it is being filed on its own.
The problem this change fixes. The case drives a real node harness in a command substitution and captures its output in "$out". That harness prints the specific reason it failed to stderr, for example "turn-end guard was suppressed by an external healthy watcher". The assertion, however, used expect_code, which reports only the exit status. A CI failure therefore read "expected exit 0, got 1" and discarded the one piece of information that explains it, which is precisely what made an earlier CI shard failure in this file hard to diagnose.
The change. Replace the expect_code call with the
[ "$status" -eq 0 ] || fail "... (exit $status): $out"form, so the harness output travels into the failure message. A short comment records why this case does not use expect_code.Decisions and constraints, so the diff is not read as arbitrary:
[ -z "$out" ] || fail "...: $out"assertion is left exactly as it was.Verification performed: the case passes unchanged. The diagnostic was then proven to work rather than assumed, by deliberately sabotaging the plugin so an external healthy watcher settles as an owned failure and re-running both forms against that identical sabotage. The old form reported "expected exit 0, got 1". The new form reported "(exit 1): turn-end guard was suppressed by an external healthy watcher". shellcheck is clean on the changed file, and the change was confirmed conflict-free with open pull request #2845, which edits a different region of the same file.
What Changed
expect_codeassertion with a local zero-exit check that includes captured harness output in failure messages while preserving the existing pass condition.Risk Assessment
✅ Low: Captain, the change is narrowly scoped and preserves the assertion semantics while correctly including captured harness output in failures.
Testing
The focused case passed normally, then a controlled end-to-end failure showed both assertion forms still exit 1 while the changed form exposes the harness reason; the comparison transcript was saved as reviewer-visible evidence and no worktree artifacts remain.
Evidence: Diagnostic counterfactual transcript
Source: Diagnostic counterfactual transcript
Old: expected exit 0, got 1 New: (exit 1): turn-end guard was suppressed by an external healthy watcherPipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
✅ **Review** - passed
✅ No issues found.
✅ **Test** - passed
✅ No issues found.
Inspectedgit diff --unified=40 f170cedeb735759e9547a5b9de1a26eca7ea6d71..31a1b825f7f273bc032ea19ab609ea38ce391615 -- tests/fm-pi-watch-extension.test.shRan onlytest_opencode_healthy_arm_output_does_not_suppress_guardthrough its real Node and OpenCode plugin harnessRan the base and changed assertions against identical disposable plugin sabotage that classifies external healthy readiness as an owned failureVerified the evidence transcript and clean worktree withgit status --short✅ **Document** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.