fix(wheelstest): capture spec-load errors against the bundle and emit positive error count

[wheels-bot]

Summary

The WheelsTest BDD runner silently dropped spec files whose run() invoked it() outside a describe() block (the throw bubbled out as an anonymous BundleRunnerMajorException with no file context), and reported a -1 sentinel error count when a spec's beforeAll() threw during load. Both shapes are easy to land accidentally during TDD iteration and cost minutes of "why isn't my new test running?" debugging.

vendor/wheels/wheelstest/system/runners/BDDRunner.cfc now starts the bundle stats record before invoking the target and wraps both the suite-descriptor pass (target.run / getTestSuites) and the bundle execution (beforeAll, suite iteration, afterAll) in one catch. The catch records the throw against bundleStats.globalException and increments the error count by a positive one, so the summary reads "1 error(s)" with the offending bundle path and the underlying exception attached. The existing displayTestResults walker already surfaces the path + message once a positive count is recorded.

Related Issue

Closes #2829

Type of Change

• Bug fix
• New feature
• Enhancement to existing feature
• Documentation update
• Refactoring

Feature Completeness Checklist

• DCO sign-off -- Every commit carries Signed-off-by: (use git commit -s); see CONTRIBUTING.md
• Tests -- New vendor/wheels/tests/specs/wheelstest/BDDRunnerErrorReportingSpec.cfc reproduces both shapes via two fixture bundles (OrphanItFixture, BeforeAllErrorFixture) and asserts totalError == 1 plus bundleStats[1].globalException populated; failing-then-passing
• Framework Docs -- left to bot-update-docs.yml follow-up
• AI Reference Docs -- left to bot-update-docs.yml follow-up
• CLAUDE.md -- left to bot-update-docs.yml follow-up
• CHANGELOG.md -- Entry added under [Unreleased] -> Fixed
• Test runner passes -- wheels.tests.specs.wheelstest 143 pass / 0 fail / 0 error (HTTP 200) after the patch; broader spot checks: wheels.tests.specs.security 182 pass / 0 fail, wheels.tests.specs.dispatch 96 pass / 0 fail, wheels.tests.specs.model 843 pass / 11 skip / 0 fail (no regressions)

Test Plan

The TDD failing-then-passing capture is the primary verification.

Before the patch, against wheels.tests.specs.wheelstest.BDDRunnerErrorReportingSpec:

• Expected [true] to be false — state.threw == true because the orphan-it() fixture surfaced as BundleRunnerMajorException rather than landing in the bundle's catch
• The actual [-1] is not greater than [0] — results.getTotalError() was the -1 sentinel

After the patch (HTTP 200, both specs green) and a re-run of security, dispatch, and model shows no regressions in the unrelated test layers.

[wheels-bot]

Wheels Bot — Docs updated

Added a doc commit to this PR:

• web/sites/guides/src/content/docs/v4-0-0/testing/running-tests-locally.mdx — added a bullet to "Common failure modes" explaining that a "1 error(s)" result with a bundle path but no failing assertion indicates a load-time or setup error in that spec file (orphan it() outside describe(), or beforeAll() throwing), and pointing readers to check the spec file and globalException for the underlying throw.

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: This fix is clean and correct. BDDRunner.cfc is patched in exactly the right two places — startBundleStats moved before target.run() so there is always a bundle record to attach errors to, and the surrounding try/catch widened to cover both the suite-descriptor phase and the execution phase so neither orphan-it() nor a throwing beforeAll() can escape as an anonymous BundleRunnerMajorException. The -1 sentinel is replaced with +1. Two purpose-built fixtures and a BDD spec verify both shapes. No cross-engine, security, or convention issues found. Verdict: approve (self-authored PR; GitHub disallows the approval API, so this is recorded as a comment review instead).

Tests

vendor/wheels/tests/specs/wheelstest/BDDRunnerErrorReportingSpec.cfc, line 39

expect(results.getTotalError()).toBeGT(0);
...
expect(bs[1].totalError).toBeGT(0);

The first test case (OrphanItFixture) correctly uses toBe(1) for exact precision. The BeforeAllErrorFixture case uses toBeGT(0), which is weaker — the fix guarantees exactly 1 error per bundle failure just as the first case does. Suggest tightening to toBe(1) to lock the invariant:

expect(results.getTotalError()).toBe(1);
...
expect(bs[1].totalError).toBe(1);

Not a blocker, but the first spec sets a clear precedent for exact counts.

Correctness

The refactor correctly preserves existing behaviour for the non-error path (the canRunBundle guard, beforeAll/afterAll annotation method loops, onSuiteStart/onSuiteEnd callbacks, and endStats are all unchanged and still run on the happy path). The isNull(thisSuite) guard in the catch block is safe: for (var thisSuite in testSuites) declares thisSuite in function scope before the loop body executes, so it is null when the catch fires before the loop is entered, and isNull(null) returns true, passing {} to announceToModules as the "no active suite" signal.

Commits

Single commit fix(wheelstest): capture spec-load errors against the bundle and emit positive error count — type fix, scope wheelstest, subject 68 characters, body explains the why. DCO Signed-off-by: claude[bot] present.

Docs

CHANGELOG.md [Unreleased] → Fixed entry is accurate and placed correctly beneath the existing ### Changed block. AI/framework-doc follow-ups are explicitly deferred to bot-update-docs.yml, which is the established pattern.

[wheels-bot]

Wheels Bot -- Reviewer A

TL;DR: The docs follow-up commit d12e401 adds exactly one bullet to the Common failure modes section of the running-tests-locally guide, accurately describing the new error output shape introduced by the fix. It is clean, well-placed, and commit-lint clean. The one minor nit from the previous review round -- tightening toBeGT(0) to toBe(1) in the BeforeAllErrorFixture case -- remains unaddressed but is not a blocker. No correctness, cross-engine, or security concerns. Verdict: comment (minor nit only).

Tests: vendor/wheels/tests/specs/wheelstest/BDDRunnerErrorReportingSpec.cfc lines 39 and 43 -- The first case OrphanItFixture uses exact assertion toBe(1). The second case BeforeAllErrorFixture uses the weaker toBeGT(0). Since the catch block unconditionally sets bundleStats.totalError = 1 and increments by 1, the result is always exactly 1. Suggest tightening both to toBe(1) to match the first case.

Docs: web/sites/guides/src/content/docs/v4-0-0/testing/running-tests-locally.mdx line 205 -- The added bullet is factually correct, names both failure shapes, points users to globalException, and directs them to the bundle path. Placement inside Common failure modes is exactly right.

Commits: fix(wheelstest) subject 68 chars DCO present; docs(web/guides) subject 52 chars DCO present. Both conform to commitlint.config.js.

wheels-bot:review-a:2831:d12e401632a54b77451f63b2f7b0d986aa2fa822

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: The docs follow-up commit (d12e401) adds exactly one bullet to the "Common failure modes" section of the running-tests-locally guide, accurately describing the new error output shape introduced by the fix. It is clean, well-placed, and commit-lint clean. The one minor nit from the previous review round — tightening toBeGT(0) to toBe(1) in the BeforeAllErrorFixture case — remains unaddressed but is not a blocker. No correctness, cross-engine, or security concerns. Verdict: comment (minor nit only).

Tests

vendor/wheels/tests/specs/wheelstest/BDDRunnerErrorReportingSpec.cfc, lines 39 and 43

The first case (OrphanItFixture) uses the exact assertion toBe(1), which locks the invariant precisely. The second case (BeforeAllErrorFixture) uses the weaker toBeGT(0). Since the catch block unconditionally sets bundleStats.totalError = 1 and calls incrementStat(type="error", count=1), the result is always exactly 1. Suggest tightening both assertions to toBe(1) to match the first case and catch any future regression that produces a different positive count.

Docs

web/sites/guides/src/content/docs/v4-0-0/testing/running-tests-locally.mdx, line 205

The added bullet is factually correct: it names both failure shapes (orphan it() call, throwing beforeAll()), points users to globalException, and directs them to the bundle path shown in the summary. Placement inside the "Common failure modes" list is exactly right.

Commits

Two commits in the PR:

1. fix(wheelstest): capture spec-load errors against the bundle and emit positive error count — type fix, scope wheelstest, subject 68 chars, DCO Signed-off-by present.
2. docs(web/guides): note BDD runner load-time errors in common failure modes — type docs, scope web/guides, subject 52 chars, DCO Signed-off-by present.

Both conform to commitlint.config.js. No issues.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review is clean and grounded. The single nit is real, A's line reference has a minor off-by-one, and both A and I land on the same verdict: the PR is correct but has two weak assertions worth tightening before merge.

Sycophancy

None detected. Every positive statement in A's review is paired with a specific reference to the code or the diff.

False positives

None detected. A's claim — that the outer catch unconditionally sets bundleStats.totalError = 1 and calls incrementStat(type="error", count=1), so getTotalError() and bs[1].totalError are always exactly 1 — is accurate against the patched BDDRunner.cfc.

Missed issues

One minor inaccuracy in A's line citation: A says "lines 39 and 43" for the two toBeGT(0) assertions in BDDRunnerErrorReportingSpec.cfc. Line 39 is correct (results.getTotalError()), but the bs[1].totalError assertion is on line 42, not 43 (line 43 is expect(bs[1].path).toInclude("BeforeAllErrorFixture")). The finding itself is correct; only the citation is off by one.

No other missed issues. Cross-engine exposure is negligible (standard try/catch and struct assignment). The isNull(thisSuite) reference in the outer catch is safe — CFML hoists var declarations to function scope, so thisSuite is null-initialized before the try block executes, and the ternary guard handles it correctly.

Verdict alignment

A's COMMENTED verdict is consistent with the findings: the fix is correct, the docs are accurate, commits are lint-clean, and the only remaining concern is a pair of weak assertions in the new spec — not a blocker, but concretely improvable.

Convergence

Aligned. The joint recommendation: tighten lines 39 and 42 of vendor/wheels/tests/specs/wheelstest/BDDRunnerErrorReportingSpec.cfc from toBeGT(0) to toBe(1) — both the results.getTotalError() assertion and the bs[1].totalError assertion in the BeforeAllErrorFixture case. These are the only changes needed; everything else in the PR is correct and ready.

[wheels-bot]

[wheels-bot]

Wheels Bot -- Reviewer B (round 1)

A's review of the docs follow-up commit is clean and technically sound. The nit about toBeGT(0) is valid and has been consistently flagged across both review rounds; it remains unaddressed but A correctly called it non-blocking. A's analysis of the isNull(thisSuite) cross-engine question (in the first-round review) is accurate. Converging on approve.

Sycophancy

None detected. A cited specific line references and explained the reasoning behind each finding rather than deferring to the author.

False positives

None detected.

• A's toBeGT(0) claim is accurate: the outer catch block in the patched BDDRunner.cfc unconditionally sets bundleStats.totalError = 1 and calls incrementStat(count = 1), so both getTotalError() and bs[1].totalError are always exactly 1 for any bundle failure. toBeGT(0) is a weaker assertion than the evidence supports.
• A's docs assessment is accurate: the added bullet in running-tests-locally.mdx correctly names both failure shapes, mentions globalException, and directs readers to the bundle path shown in the summary. Placement in "Common failure modes" is correct.
• A's commit compliance check is accurate: both commits conform to commitlint.config.js on type, scope, length, and DCO.

Missed issues

None on this round's diff. One concern A addressed in the first-round review and correctly resolved: the isNull(thisSuite) guard in the widened catch can fire when thisSuite has not been assigned yet (e.g., target.run() throws before the for (var thisSuite in testSuites) loop is entered). CFML hoists var declarations to function scope at compile time, so thisSuite is declared (null) even when the loop body never executes; isNull(null) returns true on all supported engines and {} is passed to announceToModules as the "no active suite" signal. That analysis is sound; no cross-engine regression.

Verdict alignment

A's COMMENTED state (with stated intent of "approve") is consistent with the findings: one non-blocking nit, clean docs, clean commits.

Convergence

Aligned. A has correctly identified the only actionable item (toBeGT(0) should be toBe(1) in the BeforeAllErrorFixture case of BDDRunnerErrorReportingSpec.cfc) and correctly called it non-blocking across two rounds. The fix is correct, the tests cover both error shapes, the docs follow-up is accurate, and there are no cross-engine, security, or convention concerns. Joint recommendation: approve.

[wheels-bot]

Wheels Bot — Address Review (round 1)

Applied the consensus finding from Reviewer A and Reviewer B's convergence (round 1 of the convergence loop):

• vendor/wheels/tests/specs/wheelstest/BDDRunnerErrorReportingSpec.cfc lines 39 and 42 — tightened toBeGT(0) to toBe(1) for both results.getTotalError() and bs[1].totalError in the BeforeAllErrorFixture case. Now matches the OrphanItFixture case's exact-count style, locking the invariant that the BDDRunner catch block emits exactly one error per failed bundle. (B correctly pointed out that A's line citation of "39 and 43" was off-by-one — the second assertion is on line 42.)

Verification: re-ran the wheelstest layer against the local CI server (sqlite). Result: 143 pass / 0 fail / 0 error — both tightened assertions hold, no regressions in the layer.

The new commit will trigger a fresh Reviewer A run on the updated SHA. Convergence loop continues until reviewers align on approve or the outer-loop cap (5 rounds) is reached.

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: The sole nit from the two previous review rounds — tightening toBeGT(0) to toBe(1) in the BeforeAllErrorFixture case — is now addressed in commit 15810f495. Both spec cases in BDDRunnerErrorReportingSpec.cfc now use identical exact-count assertions, which correctly locks the invariant guaranteed by the bundleStats.totalError = 1 assignment in the catch block. The PR is clean across all three commits; no correctness, cross-engine, convention, or security issues remain. Verdict: approve (GitHub blocks self-approval on bot-authored PRs, so this is recorded as a comment review instead).

Tests

vendor/wheels/tests/specs/wheelstest/BDDRunnerErrorReportingSpec.cfc, lines 39 and 42

Previous round: toBeGT(0) — weak, passes for any positive value.
This commit: toBe(1) — exact, matches what the catch block guarantees.

Both cases now read:

expect(results.getTotalError()).toBe(1);
// ...
expect(bs[1].totalError).toBe(1);

This is consistent with the OrphanItFixture case (lines 23 and 26) and correctly reflects the bundleStats.totalError = 1 hard-assignment in BDDRunner.cfc's catch block.

Commits

Three commits across the PR:

1. fix(wheelstest): capture spec-load errors against the bundle and emit positive error count — type fix, scope wheelstest, 68 chars, DCO signed. Substantive fix.
2. docs(web/guides): note BDD runner load-time errors in common failure modes — type docs, scope web/guides, 52 chars, DCO signed. Accurate follow-up.
3. fix(wheelstest): tighten BeforeAllErrorFixture assertions to toBe(1) — type fix, scope wheelstest, 49 chars, DCO signed. Addresses review nit.

All three conform to commitlint.config.js. Commit bodies explain the why.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's review of the updated SHA is clean and accurate. The sole outstanding nit from prior rounds was correctly verified as addressed, line citations are now correct, and all three commits pass conformance checks. Aligned on approve.

Sycophancy

None detected. A identified exactly what changed (the tightened assertions at lines 39 and 42), verified against the actual diff, and explained the reasoning in terms of the bundleStats.totalError = 1 hard-assignment in the catch block.

False positives

None detected.

• A cites toBe(1) at lines 39 and 42 of BDDRunnerErrorReportingSpec.cfc. The diff confirms: line 39 is expect(results.getTotalError()).toBe(1) and line 42 is expect(bs[1].totalError).toBe(1). Both correct — and notably A fixed the off-by-one from the prior round's citation of "39 and 43".
• The remaining toBeGT(0) assertions (lines 25 and 41) check arrayLen(bs), not the error count. That assertion is intentionally not exact — a future test could load more than one bundle. A correctly left those alone; they are not weak.
• A's commit-conformance table is accurate for all three commits.

Missed issues

None detected. Cross-engine exposure remains negligible: the fix is a try/catch restructuring and struct assignment with no closures, obj.map(), or arguments-scope pitfalls. The isNull(thisSuite) guard (covered in the prior round) is sound on all engines — CFML hoists var to function scope, so thisSuite is null-initialized before the loop, and isNull(null) returns true on Lucee, Adobe, and BoxLang.

Verdict alignment

A's COMMENTED state with stated intent to approve is consistent with the findings: no open issues remain, the single prior nit is addressed, and no new concerns arise from the third commit.

Convergence

Aligned. All three commits are correct: the core BDDRunner fix, the docs follow-up, and the assertion tightening. No further changes needed. Joint recommendation: approve.

[wheels-bot]

Wheels Bot — Reviewer A

TL;DR: Post-rebase re-review of the same three commits that were approved at SHA 15810f495. The PR was rebased onto a newer develop HEAD, producing the new SHA c22c2cfa7; the content of all three commits is identical to what was reviewed in round 4. The BDDRunner fix is correct, the spec assertions use exact counts in both cases, commits are commitlint-clean, and docs are updated. Verdict: approve (GitHub blocks self-approval on bot-authored PRs, so this is recorded as a comment instead).

Correctness

vendor/wheels/wheelstest/system/runners/BDDRunner.cfc, lines 55–174

Moving startBundleStats before arguments.target.run() and widening the try/catch to cover both the suite-descriptor pass and the bundle-execution pass is the right fix for both failure shapes. The happy path is preserved: canRunBundle, beforeAll/afterAll, and endStats all run in the same order as before.

The isNull(thisSuite) guard in the catch block (line 171) is safe across all supported engines. CFML hoists var declarations from for loops into function scope, so thisSuite is declared (and null) even when the catch fires before the loop is entered — which is the case when target.run() throws on the OrphanItFixture path.

The catch block writes bundleStats.globalException = e and bundleStats.totalError = 1 as struct field assignments (not local.X assignments), which is the safe pattern that survives BoxLang's catch-scope isolation (Cross-Engine Invariant #11).

Tests

vendor/wheels/tests/specs/wheelstest/BDDRunnerErrorReportingSpec.cfc, lines 23, 26, 39, 42

The nit from round 1 — tightening toBeGT(0) to toBe(1) on the BeforeAllErrorFixture assertions — is resolved. Both cases now read:

expect(results.getTotalError()).toBe(1);
expect(bs[1].totalError).toBe(1);

This locks the invariant guaranteed by the bundleStats.totalError = 1 hard-assignment in the catch block. The shared-struct pattern (var state = {threw: false, results: ""}) used in the OrphanIt case correctly routes around the closure-scope issue (Anti-Pattern #10).

Commits

Three commits, all conforming to commitlint.config.js:

1. fix(wheelstest): capture spec-load errors against the bundle and emit positive error count — 68 chars, DCO signed, explains the why in the body.
2. docs(web/guides): note BDD runner load-time errors in common failure modes — 52 chars, DCO signed.
3. fix(wheelstest): tighten BeforeAllErrorFixture assertions to toBe(1) — 49 chars, DCO signed.

Docs

CHANGELOG.md carries a [Unreleased] → Fixed entry. web/sites/guides/src/content/docs/v4-0-0/testing/running-tests-locally.mdx adds a bullet under "Common failure modes" that names both error shapes and points users to globalException. Both are accurate and well-placed.

[wheels-bot]

Wheels Bot — Reviewer B (round 1)

A's re-review of the rebased SHA is accurate and well-grounded. One minor false positive on commit character-count methodology — not consequential, all three commits pass the limit — and no missed issues on the diff. Aligned on approve.

Sycophancy

None detected. A correctly identified this as a rebase re-review (same three commits, new SHA), cited specific line numbers, and explained the reasoning behind each finding rather than deferring to prior approval.

False positives

One methodology issue: A cites commit header lengths as "68", "52", and "49" chars respectively, but those figures appear to count only the subject portion and not the full type(scope): subject header. Per CLAUDE.md, commitlint measures the whole header including the prefix. Actual lengths:

1. fix(wheelstest): capture spec-load errors against the bundle and emit positive error count — 90 chars
2. docs(web/guides): note BDD runner load-time errors in common failure modes — 74 chars
3. fix(wheelstest): tighten BeforeAllErrorFixture assertions to toBe(1) — 68 chars

All three are under the 100-char ceiling, so A's conclusion (conforming) is correct. The cited numbers are wrong but the call stands.

Everything else A claimed was verified against the diff:

• startBundleStats is indeed moved before arguments.target.run(), and the outer try/catch correctly wraps both the suite-descriptor pass and the execution pass.
• The catch block assigns bundleStats.totalError = 1 and calls incrementStat(type = "error", count = 1) as struct fields — not local.X — which is the correct BoxLang-safe pattern (Cross-Engine Invariant 11).
• isNull(thisSuite) ? {} : thisSuite in the catch is safe: CFML hoists var from for loops to function scope, so thisSuite is null-initialized before the loop body ever executes.
• The BDDRunnerErrorReportingSpec.cfc assertions at lines 23, 26 (OrphanIt case) and 39, 42 (BeforeAllError case) all use toBe(1) — tightened from toBeGT(0) as previously agreed. Lines 25 and 41 (arrayLen(bs)) correctly remain toBeGT(0), since an exact count there would be over-constrained.

Missed issues

None detected. The remaining toBeGT(0) assertions (arrayLen checks) are intentionally not exact and A correctly left them alone. endStats(bundleStats) is still called after the widened catch, preserving the pre-patch finalization behavior. No closures, struct member function calls, or arguments-scope pitfalls in the changed code.

Verdict alignment

A's COMMENTED state with explicit "approve" intent is consistent with the findings: no open issues, no new concerns introduced by the rebase, prior nit fully addressed.

Convergence

Aligned. The three commits are correct: the BDDRunner catch widening, the docs follow-up, and the assertion tightening. The char-count methodology issue is not consequential — all headers pass linting. Joint recommendation: approve.