fix(test): render txt test report inline so format=txt no longer 404s on missing asset - #2676
Conversation
… on missing asset TextReporter.cfc included a vendored template at assets/text.cfm that was never carried over from upstream TestBox. Hitting /wheels/app/tests or /wheels/core/tests with format=txt therefore threw a missinginclude exception, while format=html/json/junit kept working. The reporter now builds its plain-text output inline from the TestResult bundleStats tree, mirroring the shape of vendor/wheels/public/tests/txt.cfm and keeping the existing getStatusIndicator/getBundleIndicator helpers. Fixes #2675 Signed-off-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com>
Wheels Bot — No doc updatesReviewed this PR's diff and found no docs that need updating (purely a bug fix that restores |
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR correctly fixes the format=txt 404 by replacing the un-vendored include "assets/text.cfm" with equivalent inline rendering. The implementation is clean, uses the right struct key names, and has no cross-engine issues. One minor gap: the recursive $renderSuiteText helper is never exercised by the tests because both specs use an empty TestResult, leaving that path untested. Overall verdict: comment (no blocking issues).
Correctness
getCFMLEngine() / getCFMLEngineVersion() exist: Confirmed — TestResult.cfc declares them as CFML property entries (lines 38–39) with auto-generated getters, initialized in init() (lines 96–97). No missing-method risk.
globalException guard is correct: The check in TextReporter.cfc:
if ( isStruct( bundle.globalException ) && structKeyExists( bundle.globalException, "message" ) ) {is sound. TestResult.cfc:215 initialises globalException as "" (empty string); BDDRunner.cfc:146 and UnitRunner.cfc:156 assign the caught exception struct on error. isStruct("") == false on all engines, so the guard fires correctly.
writeOutput() inside $renderSuiteText reaches the savecontent buffer: Standard CFML output-buffer semantics guarantee that writeOutput() from any synchronously-called CFC method writes to the active savecontent buffer in the parent scope. Correct pattern.
Tests
Recursive path is never executed — minor gap. Both specs instantiate a fresh new wheels.wheelstest.system.TestResult() which carries zero bundles. The outer for ( var bundle in variables.bundleStats ) loop in runReport is never entered, so $renderSuiteText is never called. A typo in key names (specStats, suiteStats) or a bug in the recursive call would not be caught.
vendor/wheels/tests/specs/wheelstest/TextReporterSpec.cfc — consider adding a third spec that seeds one bundle + one suite + one passing spec and asserts the output contains the bundle path and spec name. This is a suggestion rather than a blocker — the two existing specs do prove the regression (Page not found) is fixed.
Commits
fix(test): render txt test report inline so format=txt no longer 404s on missing asset
Scope test is accurate (TextReporter lives under wheelstest/), subject is under 100 chars, not ALL-CAPS. Passes commitlint.
Docs
CHANGELOG entry under [Unreleased] / Fixed is present and well-worded. No .ai/wheels/ doc update is needed for this internal reporter fix.
Wheels Bot — Reviewer B (round 1)A's review is accurate and well-grounded. I verified every technical claim against the actual source and found no sycophancy, no false positives, and no missed issues. The test-coverage gap A flagged is the only substantive finding, and A correctly categorised it as non-blocking. Converging on approve. SycophancyNone detected. A gave a "comment" verdict (not "approve") specifically because of the untested recursive path, which is the right call for an incomplete-but-not-blocking concern. False positivesNone detected. I verified each claim against the source:
Missed issuesNone detected. The cross-engine surface is minimal ( Verdict alignmentA's "comment" verdict is consistent with the findings: one non-blocking suggestion, everything else verified correct. Appropriate. ConvergenceAligned. A identified the only real gap (recursive path untested), correctly marked it as a suggestion rather than a blocker, and verified the rest of the implementation. I agree on both counts. The PR fixes a real regression with tests that prove it, and the code is correct for all paths including the untested recursive one. Recommending approve. |
…-assets-text-cfm-not-f
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: The PR fixes the format=txt 404 by replacing the un-vendored include "assets/text.cfm" with equivalent inline rendering inside TextReporter.runReport. Since the previous review (ddb1368) the only new commit is a merge of develop into the branch; the PR-specific code is unchanged. The fix is correct, the savecontent + writeOutput() pattern is portable across all engines, and the existing tests prove the regression is resolved. One minor gap carries forward from the last review: the recursive $renderSuiteText helper is never exercised by the specs. Overall verdict: comment (no blocking issues, no changes needed to merge).
Correctness
The savecontent + writeOutput() call-through pattern is sound on all three engines. During savecontent execution the page output buffer is redirected to the capture variable; any synchronously-invoked method that calls writeOutput() writes into that same buffer. Both the summary block and the per-bundle/suite recursive calls are therefore captured correctly.
The two guard conditions are correct:
isStruct( bundle.globalException ) && structKeyExists( bundle.globalException, "message" )—TestResult.cfcinitialisesglobalExceptionas""(empty string), makingisStruct("") == falseon all engines; the guard fires only when an exception struct has been assigned.getCFMLEngine()/getCFMLEngineVersion()— both areproperty-declared auto-getters inTestResult.cfc, initialised ininit(). No missing-method risk.
Tests
Recursive path still unexercised — minor gap carried from the prior review. Both specs instantiate a zero-bundle TestResult, so the for ( var bundle in variables.bundleStats ) loop never executes, and $renderSuiteText is never called. A typo in spec.failMessage, a wrong key name in suiteStats, or a regression in the recursive depth logic would not be caught.
vendor/wheels/tests/specs/wheelstest/TextReporterSpec.cfc — consider a third spec that seeds one bundle + one suite + one passing spec into a TestResult and asserts the output contains the bundle path and spec name. This is a suggestion, not a blocker; the two existing specs are sufficient to confirm the 404 regression is fixed.
Cross-engine
No cross-engine concerns. The implementation uses only core CFML builtins (repeatString, writeOutput, arrayLen, arrayToList, chr, lCase, len) and the savecontent tag — all of which behave identically on Lucee, Adobe CF, and BoxLang. The $renderSuiteText recursive call uses named arguments only, avoiding the mixed positional/named anti-pattern.
Commits
fix(test): render txt test report inline so format=txt no longer 404s on missing asset
Valid type (fix), valid scope (test), 86 chars, not ALL-CAPS. Passes commitlint. DCO Signed-off-by: trailer is present. No issues.
Docs
CHANGELOG [Unreleased] / Fixed entry is present and clearly describes both the symptom and the root cause. No .ai/wheels/ doc update is needed for an internal reporter fix.
Wheels Bot — Reviewer B (round 1)A’s review accurately characterises a merge-only SHA where no PR-specific code changed. I spot-checked every technical claim against the actual source and found nothing to dispute. Converging on approve. SycophancyNone detected. A used the “comment” (non-binding) GitHub verdict rather than “approve”, which is the right choice when a minor suggestion remains outstanding. The overall tone matches the finding weight. False positivesNone detected. I verified each claim against the actual source:
Missed issuesNone detected. A correctly skipped re-analysis of the unchanged implementation (the merge commit carries no PR-specific diff). I confirmed the Verdict alignmentA’s “comment” verdict is consistent with the findings: one non-blocking suggestion (recursive path unexercised by specs), everything else verified correct. ConvergenceAligned. A and B agree: no blocking issues, no changes required, the 404 regression is fixed with adequate tests. Recommending approve. |
…-assets-text-cfm-not-f
There was a problem hiding this comment.
Wheels Bot — Reviewer A
TL;DR: This PR fixes the format=txt 404 by replacing the never-vendored include "assets/text.cfm" with equivalent inline rendering derived from the TestResult.bundleStats tree. The implementation is correct and portable across all engines. The only change since the previous review (SHA 9dfd953f9) is a develop merge that brought in an unrelated commit (dc08f8fe5 — lockingSpec advisory-lock guard); the PR-specific files are byte-identical. One minor gap carries forward: the recursive $renderSuiteText helper is never exercised by the specs. Verdict: comment (no blocking issues).
Correctness
All TestResult getters confirmed. vendor/wheels/wheelstest/system/TestResult.cfc declares every method called in runReport as a CFML property, generating auto-getters at compile time:
- Lines 10-21:
totalDuration,totalBundles,totalSuites,totalSpecs,totalPass,totalFail,totalError,totalSkipped - Line 22:
labels(type="array") generatesgetLabels() - Lines 38-39:
CFMLEngine,CFMLEngineVersiongenerategetCFMLEngine(),getCFMLEngineVersion()
No missing-method risk.
isStruct guard is sound (TextReporter.cfc:62). TestResult initialises globalException as an empty string; exception structs are only assigned by the runners on error. isStruct("") == false on all engines, so the guard fires correctly in both the happy path and the exception path.
writeOutput() inside $renderSuiteText is correctly captured by the parent's savecontent block (TextReporter.cfc:43-68). CFML redirects the page output buffer for the duration of the savecontent block; any synchronously-invoked CFC method's writeOutput() writes into that same buffer. The recursive calls at TextReporter.cfc:66 and TextReporter.cfc:96 are both synchronous, so capture is guaranteed.
Tests
Recursive path still unexercised — minor gap carried from prior reviews. Both specs in vendor/wheels/tests/specs/wheelstest/TextReporterSpec.cfc (lines 7-33) instantiate a zero-bundle TestResult, so the for ( var bundle in variables.bundleStats ) loop at TextReporter.cfc:59 never executes. A typo in spec.failMessage, a wrong key name in suiteStats, or a bug in the recursion depth arithmetic would not be caught.
Suggestion (not a blocker): add a third spec that seeds one bundle + one suite + one passing spec into a TestResult before calling runReport, then asserts the output contains the bundle path and spec name. The two existing specs are sufficient to prove the 404 regression is resolved.
Cross-engine
No concerns. The implementation uses only universal CFML builtins — repeatString, chr, lCase, len, arrayLen, arrayToList, isStruct, structKeyExists, writeOutput, savecontent, reReplace, trim — all of which behave identically on Lucee, Adobe CF, and BoxLang. Named arguments are used exclusively in the $renderSuiteText calls (TextReporter.cfc:66, TextReporter.cfc:96), avoiding the mixed positional/named anti-pattern.
Commits
fix(test): render txt test report inline so format=txt no longer 404s on missing asset
Valid type (fix), valid scope (test), 86 chars, not ALL-CAPS. DCO Signed-off-by: trailer is present. Passes commitlint.
Docs
CHANGELOG.md [Unreleased] / Fixed entry is present and correctly describes both the symptom (Page not found on assets/text.cfm) and the root cause (template never carried over from upstream TestBox). No .ai/wheels/ doc update is needed for an internal reporter fix.
Wheels Bot — Reviewer B (round 1)A's review is accurate on this merge-only SHA. I spot-checked every claim against the actual source and found nothing to dispute. Converging on approve. SycophancyNone detected. A used the "comment" (non-binding) verdict rather than "approve" because the recursive path in False positivesNone detected. I verified each claim against the live file:
Missed issuesNone detected. A correctly characterised the SHA as a merge-only commit with no PR-specific code changes. The Verdict alignmentA's "comment" verdict is consistent with the one remaining gap (recursive path unexercised). No blocking issues identified. ConvergenceAligned. A and B agree: the 404 regression is fixed with correct, engine-portable code and tests that prove the regression scenario. The untested recursive path is a non-blocking suggestion carried across all prior rounds. Recommending approve. |
Summary
vendor/wheels/wheelstest/system/reports/TextReporter.cfcdidinclude "assets/text.cfm", but theassets/directory was never carried over from upstream TestBox when the framework vendored its reporter CFCs. Selectingformat=txtfrom/wheels/app/testsor/wheels/core/teststherefore threwPage [/wheels/wheelstest/system/reports/assets/text.cfm] not foundwhilehtml/json/junitkept working.This PR refactors
TextReporter.runReportto render its plain-text output inline from theTestResult.bundleStatstree (mirroring the inline shape ofvendor/wheels/public/tests/txt.cfm), keeping the existinggetStatusIndicator/getBundleIndicatorhelpers and dropping the brokeninclude. A small recursive$renderSuiteTexthelper handles nested suite stats.Fixes #2675
Related Issue
Closes #2675
Type of Change
Feature Completeness Checklist
Signed-off-by:trailervendor/wheels/tests/specs/wheelstest/TextReporterSpec.cfcreproduces the bug (failing → passing) and asserts the report is a non-empty plain-text stringbot-update-docs.ymlfollow-up if needed)bot-update-docs.ymlfollow-up if needed)bot-update-docs.ymlfollow-up if needed)[Unreleased] / Fixedcurl http://localhost:60007/wheels/core/tests?db=sqlite&format=json&directory=wheels.tests.specs.wheelstestreturns{pass: 123, fail: 0, error: 0};view/regression check returns{pass: 513, fail: 0, error: 0};format=txtend-to-end now returns HTTP 200 with a populated reportTest Plan
vendor/wheels/tests/specs/wheelstest/TextReporterSpec.cfc— direct unit test ofTextReporter.runReport()with a freshTestResult/TestBox(failed before the implementation, passes after)curl http://localhost:60007/wheels/core/tests?db=sqlite&format=txt&directory=wheels.tests.specs.wheelstestnow returns HTTP 200 with the expectedTEST RESULTSheader, totals, and per-spec linesScreenshots / Output