feat: resilient error reporting across component/host boundary#30
Merged
gamescoot merged 4 commits intoMay 7, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds per-assertion source location and function metadata, extends runtime/global error capture (including call-chain and message), records test function/class names in context, refactors report generation (file resolution, JSON/JUnit output control), and exposes a terse JSON call-chain flag; plus Makefile, docs, and .gitignore updates. ChangesTesting runtime, error capture, and reporting
Sequence Diagram(s)sequenceDiagram
participant Runner as TestRunner
participant TF as _TestFunction
participant Testing as Testing (context)
participant Host as hostStorage
participant Storage as Storage:C1525
participant Assert as Assert._recordAssertion
Runner->>TF: start run() (sets t.testFunctionName/testClassName)
TF->>Host: _collectProcessErrors() (drain hostStorage.testErrors)
TF->>Storage: _collectProcessErrors() (drain Storage:C1525.testErrors)
TF->>TF: enrich errors (functionName, formula) and callChainJSON
TF->>Assert: _addRuntimeErrorAsAssertion() (append assertions with line/function)
TF->>Runner: return test result
Runner->>Runner: _captureGlobalErrors() (read host/storage testErrors)
Runner->>Runner: _generateReport() (resolve files, include runtimeErrors, gate call chains)
Runner->>FileSystem: _resolveOutputFile() / write outputs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
testing/Project/Sources/Classes/Testing.4dm (1)
65-71: Consider resettingtestFunctionName/testClassNamehere too.For consistency,
resetForNewTest()should clear the new context fields alongside the other per-test state. Today it works because_TestFunction.run()re-assigns them right after calling this, but ifresetForNewTestis ever invoked in another path (or by a caller that forgets to reassign), stale names will leak into assertions recorded via_recordAssertion/_findTestLine.♻️ Proposed tweak
Function resetForNewTest() This:C1470.failed:=False:C215 This:C1470.done:=False:C215 This:C1470.logMessages:=[] This:C1470.assertions:=[] This:C1470.stats.resetStatistics() This:C1470.failureCallChain:=Null + This:C1470.testFunctionName:="" + This:C1470.testClassName:=""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/Project/Sources/Classes/Testing.4dm` around lines 65 - 71, resetForNewTest currently clears per-test state but does not clear the context fields testFunctionName and testClassName, which can leak into _recordAssertion/_findTestLine if resetForNewTest is called outside _TestFunction.run; update Function resetForNewTest() to explicitly reset This.testFunctionName and This.testClassName (e.g., set to empty string or Null consistent with your codebase) alongside This.failed, This.done, This.logMessages, This.assertions, This.stats.resetStatistics(), and This.failureCallChain so the test context is always cleared.testing/Project/Sources/Classes/TestRunner.4dm (1)
566-571: Nit: indentation inconsistency.The new
If ($failedTest.callChain#Null:C1517)block is indented one level shallower than the surroundingIf ($failedTest.runtimeErrors...)block, while its body$terseFailure.callChain:=...is indented two levels deeper than itsIf. TheEnd ifat line 570 and subsequent$failedTests.pushat line 571 are also off relative to each other. Functionally fine (4D doesn't care), but it obscures the structure on review.♻️ Align indentation
- // Include call chain for failed tests - If ($failedTest.callChain#Null:C1517) - $terseFailure.callChain:=$failedTest.callChain - End if - $failedTests.push($terseFailure) + // Include call chain for failed tests + If ($failedTest.callChain#Null:C1517) + $terseFailure.callChain:=$failedTest.callChain + End if + $failedTests.push($terseFailure)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/Project/Sources/Classes/TestRunner.4dm` around lines 566 - 571, Reformat the indentation for the callChain conditional to match the surrounding runtimeErrors block: align the "If ($failedTest.callChain#Null:C1517)" with the enclosing "If ($failedTest.runtimeErrors...)" line, indent the body "$terseFailure.callChain:=$failedTest.callChain" one level further than that If, place the corresponding "End if" directly under the If (same indent), and ensure "$failedTests.push($terseFailure)" aligns with the enclosing block's statements; adjust spacing around these symbols so the If/End if pair and the push call visually reflect the control structure.testing/Project/Sources/Classes/Assert.4dm (1)
275-293: Current implementation is correct; defensive matching is optional.Web documentation for 4D's
Get call chainconfirms that class function frames reportnameas the bare function name (e.g.,"testExample"), not qualified with class name. The current code matches this correctly:$frame.nameagainst$t.testFunctionName(bare). Tests confirm the feature works—line numbers are being captured and included in assertion output.However, the qualified name variable (
$functionName) is built in_recordAssertionbut only used in the output object; it's never used for matching in_findTestLine. The proposed defensive matching (checking both bare and qualified names) is reasonable for robustness but unnecessary based on current evidence. If you want to add it, the suggested diff is sound and would do no harm.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@testing/Project/Sources/Classes/Assert.4dm` around lines 275 - 293, The current _findTestLine function only matches bare names ($frame.name against $t.testFunctionName) which is correct but to be defensive update _findTestLine to also accept a qualified name: construct the qualified function name exactly as done in _recordAssertion (the same format used to populate $functionName) and, inside the For each ($frame; $chain) loop, check both ($frame.name=$t.testFunctionName) OR ($frame.name=qualifiedName) before returning $frame.line; ensure you reference $t.testFunctionName and the same qualified-name construction used in _recordAssertion so matching covers both bare and class-qualified frames.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@testing/Project/Sources/Classes/_TestFunction.4dm`:
- Around line 474-483: The function _errorBelongsToProcess currently returns
True when an error object lacks a processNumber, which can misattribute errors;
update _errorBelongsToProcess so it only returns True when $error.processNumber
is present and equals $processNumber, and return False when $error.processNumber
is Null (i.e., change the default return to False and ensure the conditional
using $error.processNumber#Null checks equality before returning True), keeping
the existing Null check for $error itself intact.
- Around line 68-86: The runtime error collection currently pushes every entry
from This:C1470._collectProcessErrors($processNumber) into
This:C1470.runtimeErrors and calls This:C1470._addRuntimeErrorAsAssertion for
each, causing cascaded errors to inflate assertions; fix by de-duplicating
processErrors before pushing: iterate the array returned by
This:C1470._collectProcessErrors($processNumber), compute a unique key for each
error (e.g., error.code + "|" + error.method + "|" + error.line or similar
fields present), keep only the first error per key, and then push those uniques
into This:C1470.runtimeErrors and call This:C1470._addRuntimeErrorAsAssertion
once per unique; ensure This:C1470.t.failed is set true only once for the test;
alternatively, if you prefer immediate bail-out, short-circuit the handler by
checking Storage.testErrors for this process and set a bail flag the test body
inspects to stop execution — implement one of these two approaches in the block
that processes $processErrors (referencing This:C1470._collectProcessErrors,
This:C1470.runtimeErrors, This:C1470._addRuntimeErrorAsAssertion, and
This:C1470.t.failed).
---
Nitpick comments:
In `@testing/Project/Sources/Classes/Assert.4dm`:
- Around line 275-293: The current _findTestLine function only matches bare
names ($frame.name against $t.testFunctionName) which is correct but to be
defensive update _findTestLine to also accept a qualified name: construct the
qualified function name exactly as done in _recordAssertion (the same format
used to populate $functionName) and, inside the For each ($frame; $chain) loop,
check both ($frame.name=$t.testFunctionName) OR ($frame.name=qualifiedName)
before returning $frame.line; ensure you reference $t.testFunctionName and the
same qualified-name construction used in _recordAssertion so matching covers
both bare and class-qualified frames.
In `@testing/Project/Sources/Classes/Testing.4dm`:
- Around line 65-71: resetForNewTest currently clears per-test state but does
not clear the context fields testFunctionName and testClassName, which can leak
into _recordAssertion/_findTestLine if resetForNewTest is called outside
_TestFunction.run; update Function resetForNewTest() to explicitly reset
This.testFunctionName and This.testClassName (e.g., set to empty string or Null
consistent with your codebase) alongside This.failed, This.done,
This.logMessages, This.assertions, This.stats.resetStatistics(), and
This.failureCallChain so the test context is always cleared.
In `@testing/Project/Sources/Classes/TestRunner.4dm`:
- Around line 566-571: Reformat the indentation for the callChain conditional to
match the surrounding runtimeErrors block: align the "If
($failedTest.callChain#Null:C1517)" with the enclosing "If
($failedTest.runtimeErrors...)" line, indent the body
"$terseFailure.callChain:=$failedTest.callChain" one level further than that If,
place the corresponding "End if" directly under the If (same indent), and ensure
"$failedTests.push($terseFailure)" aligns with the enclosing block's statements;
adjust spacing around these symbols so the If/End if pair and the push call
visually reflect the control structure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbc3b76f-d26e-4f55-9de3-f293e1845018
📒 Files selected for processing (6)
testing/Project/Sources/Classes/Assert.4dmtesting/Project/Sources/Classes/TestRunner.4dmtesting/Project/Sources/Classes/Testing.4dmtesting/Project/Sources/Classes/_TestFunction.4dmtesting/Project/Sources/Methods/TestErrorHandler.4dmtesting/Project/Sources/Methods/TestGlobalErrorHandler.4dm
Comment on lines
+68
to
+86
| This:C1470.function.apply(This:C1470.classInstance; [This:C1470.t]) | ||
|
|
||
| // Collect errors captured by ON ERR CALL handler in Storage.testErrors | ||
| var $processErrors : Collection | ||
| $processErrors:=This:C1470._collectProcessErrors($processNumber) | ||
|
|
||
| If ($processErrors.length>0) | ||
| var $qualifiedName : Text | ||
| $qualifiedName:=This:C1470.class.name+"."+This:C1470.functionName | ||
|
|
||
| var $error : Object | ||
| For each ($error; $processErrors) | ||
| $error.functionName:=$qualifiedName | ||
| $error.formula:=$error.method || "" | ||
| This:C1470.runtimeErrors.push($error) | ||
| This:C1470._addRuntimeErrorAsAssertion($error) | ||
| End for each | ||
| This:C1470.t.failed:=True:C214 | ||
| End if |
Contributor
There was a problem hiding this comment.
Cascading errors after the first runtime error will each become a failed assertion.
Because ON ERR CALL is non-fatal, execution continues past the failing line. If the first runtime error (e.g., ALERT($result.length)) leaves a variable in an unexpected state, subsequent lines can produce additional runtime errors that are all captured in processErrors, and each gets pushed both into runtimeErrors and as a separate isRuntimeError:true assertion (lines 82-83). Reports may show N failed assertions for what is really one root cause, inflating assertionCount and noise in the JSON output.
Consider either:
- stopping execution of the test body on first error (e.g., check
Storage.testErrorsfor this process inside the handler and set a flag the test can bail on), or - de-duplicating
processErrorsby(code, method, line)before turning each into an assertion.
Not blocking — the current behavior matches the PR goal of "continue executing remaining tests" at the runner level — but worth documenting the per-test cascade semantics.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testing/Project/Sources/Classes/_TestFunction.4dm` around lines 68 - 86, The
runtime error collection currently pushes every entry from
This:C1470._collectProcessErrors($processNumber) into This:C1470.runtimeErrors
and calls This:C1470._addRuntimeErrorAsAssertion for each, causing cascaded
errors to inflate assertions; fix by de-duplicating processErrors before
pushing: iterate the array returned by
This:C1470._collectProcessErrors($processNumber), compute a unique key for each
error (e.g., error.code + "|" + error.method + "|" + error.line or similar
fields present), keep only the first error per key, and then push those uniques
into This:C1470.runtimeErrors and call This:C1470._addRuntimeErrorAsAssertion
once per unique; ensure This:C1470.t.failed is set true only once for the test;
alternatively, if you prefer immediate bail-out, short-circuit the handler by
checking Storage.testErrors for this process and set a bail flag the test body
inspects to stop execution — implement one of these two approaches in the block
that processes $processErrors (referencing This:C1470._collectProcessErrors,
This:C1470.runtimeErrors, This:C1470._addRuntimeErrorAsAssertion, and
This:C1470.t.failed).
Collaborator
There was a problem hiding this comment.
I believe this is good behavior as we want it. We definitely do not want to stop execution (that's the problem we're trying to solve), and if there are multiple errors on a single line, we'd want all of them.
Contributor
There was a problem hiding this comment.
@FlutesForHire, understood — that's a reasonable design choice. Capturing every runtime error as its own assertion gives full fidelity into what went wrong per test, and stopping execution would defeat the purpose of the non-fatal error handling this PR introduces. I'll keep this in mind for future reviews.
✏️ Learnings added
Learnt from: FlutesForHire
Repo: KyleKincer/testing PR: 30
File: testing/Project/Sources/Classes/_TestFunction.4dm:68-86
Timestamp: 2026-04-23T17:20:42.844Z
Learning: In `testing/Project/Sources/Classes/_TestFunction.4dm`, capturing all runtime errors (including cascading ones) as individual failed assertions is intentional. Each error from `_collectProcessErrors` is pushed into `runtimeErrors` and recorded as a separate `isRuntimeError:true` assertion. Stopping execution on the first error is explicitly not desired — the goal is to continue executing remaining tests while reporting every error. Multiple errors on a single line should all be reported.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-30T19:27:58.388Z
Learning: Applies to **/*Test.4dm : Use the Testing context parameter ($t : cs:Testing) to access transaction control methods (startTransaction, validateTransaction, cancelTransaction, inTransaction) when manual transaction control is needed
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-30T19:27:58.388Z
Learning: Applies to **/*Test.4dm : Implement test assertion patterns using the Assert library methods for validating test conditions in 4D test functions
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T19:27:45.415Z
Learning: Applies to **/*Test.4dm : Use manual transaction control methods (`startTransaction()`, `validateTransaction()`, `cancelTransaction()`, `withTransaction()`, `withTransactionValidate()`) when `// `#transaction`: false` is set
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-30T19:27:58.388Z
Learning: Applies to **/*Test.4dm : Leverage automatic transaction management for database isolation by allowing tests to run in automatic transactions that are rolled back after completion
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T19:27:45.415Z
Learning: Applies to **/*Test.4dm : Disable automatic transaction management per-test using `// `#transaction`: false` annotation
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-30T19:27:58.388Z
Learning: Applies to **/*Test.4dm : Use `// `#transaction`: false` annotation to disable automatic transaction management for tests requiring manual transaction control
Learnt from: CR
Repo: KyleKincer/testing PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-30T19:27:45.415Z
Learning: Applies to **/*.4dm : Pass host project's Storage object to Testing framework when running tests from a host project using `Testing_RunTestsWithCs(cs; $hostStorage; $userParams)` method
Comment on lines
+532
to
+568
| // Include call chain in verbose JSON output | ||
| If (This:C1470.verboseOutput) && ($failedTest.callChain#Null:C1517) | ||
| // Include call chain for failed tests | ||
| If ($failedTest.callChain#Null:C1517) |
Collaborator
There was a problem hiding this comment.
Do we really want to remove the verboseOutput guard clause here?
Also, nitpick: The If on line 568 is at the wrong indention level.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 38: The test-junit and test-parallel targets still invoke tool4d directly
and thus ignore the new USER_PARAMS variable; update the recipes for test-junit
and test-parallel so they invoke the test target via $(MAKE) and forward
USER_PARAMS (e.g. change their recipe from calling tool4d ... to something like:
$(MAKE) test $(USER_PARAMS)) so that USER_PARAMS is honored consistently by
test, test-junit and test-parallel; reference the USER_PARAMS variable and the
test, test-junit, and test-parallel targets when making the change.
In `@scripts/test-sandbox.sh`:
- Around line 63-71: The current rewrite injects an unescaped absolute path into
USER_PARAM (via RAW_PATH -> ABS_PATH), which breaks TestRunner._parseParamString
because it splits on spaces; instead preserve round-trippable tokens by escaping
or encoding spaces before substitution: when building ABS_PATH, transform spaces
into an escaped form (e.g., replace " " with "\ " or URL-encode as "%20") and
then perform
USER_PARAM="${USER_PARAM/outputPath=$RAW_PATH/outputPath=$ESCAPED_ABS_PATH}";
alternatively, don't replace the token with an absolute path and leave
outputPath=$RAW_PATH unchanged so the parser receives the original token. Ensure
you update the substitution using the ESCAPED_ABS_PATH variable and keep
references to USER_PARAM, RAW_PATH and ABS_PATH consistent.
In `@testing/Project/Sources/Classes/TestRunner.4dm`:
- Around line 31-37: The unconditional LOG EVENT calls that write "[DEBUG] ..."
to standard output (see the LOG EVENT(... Into system standard outputs ...)
lines around calls to This:C1470._runInternal(), _captureGlobalErrors(), and
_generateReport()) corrupt JSON when format=json and no outputPath is set;
modify the run path so those debug LOG EVENTs are either skipped when the runner
is configured for JSON stdout (e.g., check the runner's format/outputPath flags)
or redirected to a different sink (stderr or a logger that doesn't use standard
outputs) before emitting the JSON body to stdout.
In `@testing/Project/Sources/Methods/TestErrorHandler.4dm`:
- Around line 38-50: The current filter in TestErrorHandler removes any call
chain entry from database "testing", which also drops user test frames; update
the filter logic in the loop over $rawChain (variables $rawChain,
$filteredChain, $entry) to stop excluding by ($entry.database#"testing") and
instead only skip explicit framework/internal frames — e.g., check and skip when
$entry.name matches known internal handler names like "TestErrorHandler" or when
an entry flag/field indicates an internal/framework module (e.g.,
$entry.isInternal or $entry.module == "framework"), then push all other entries
and continue to produce $callChainJSON via JSON Stringify:C1217($filteredChain).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6e06173a-ccfb-43c8-9be4-3e9c0c85c6a4
📒 Files selected for processing (9)
.gitignoreMakefilescripts/test-sandbox.shtesting/Project/Sources/Classes/TestRunner.4dmtesting/Project/Sources/Classes/Testing.4dmtesting/Project/Sources/Classes/_TestFunction.4dmtesting/Project/Sources/Methods/TestErrorHandler.4dmtesting/Project/Sources/Methods/TestGlobalErrorHandler.4dmtesting/Resources/en.lproj/syntaxEN.json
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- testing/Project/Sources/Classes/Testing.4dm
Comment on lines
+31
to
+37
| LOG EVENT:C667(Into system standard outputs:K38:9; "[DEBUG] run: before _runInternal\r\n"; Information message:K38:1) | ||
| This:C1470._runInternal() | ||
| LOG EVENT:C667(Into system standard outputs:K38:9; "[DEBUG] run: after _runInternal\r\n"; Information message:K38:1) | ||
| This:C1470._captureGlobalErrors() | ||
| LOG EVENT:C667(Into system standard outputs:K38:9; "[DEBUG] run: after _captureGlobalErrors\r\n"; Information message:K38:1) | ||
| This:C1470._generateReport() | ||
| LOG EVENT:C667(Into system standard outputs:K38:9; "[DEBUG] run: after _generateReport\r\n"; Information message:K38:1) |
Contributor
There was a problem hiding this comment.
Unconditional debug logging corrupts stdout JSON output.
These LOG EVENT(... Into system standard outputs ...) calls write to the same stream as the JSON body. With format=json and no outputPath, the new [DEBUG] ... lines are emitted before/around the JSON string, so consumers no longer get valid JSON. That breaks the PR’s “always produce JSON output” guarantee. Gate them off for JSON stdout, or send them to a different sink.
Also applies to: 420-433, 497-619
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@testing/Project/Sources/Classes/TestRunner.4dm` around lines 31 - 37, The
unconditional LOG EVENT calls that write "[DEBUG] ..." to standard output (see
the LOG EVENT(... Into system standard outputs ...) lines around calls to
This:C1470._runInternal(), _captureGlobalErrors(), and _generateReport())
corrupt JSON when format=json and no outputPath is set; modify the run path so
those debug LOG EVENTs are either skipped when the runner is configured for JSON
stdout (e.g., check the runner's format/outputPath flags) or redirected to a
different sink (stderr or a logger that doesn't use standard outputs) before
emitting the JSON body to stdout.
gamescoot
pushed a commit
to gamescoot/testing
that referenced
this pull request
May 4, 2026
…ains - Route test-junit, test-ci, junit tag variants, and parallel targets through `make test` so outputPath/verbose/callchain USER_PARAMS apply (CodeRabbit). - Remove debug LOG EVENT lines that broke JSON-on-stdout consumers. - Error handlers: stop filtering every testing-database stack frame; only drop the handler method so real test frames remain (CodeRabbit / host chains). - Terse JSON failures: callChain only when callchain=true; verbose no longer implies callChain (explicit flag; addresses FlutesForHire). - Drop scripts/test-sandbox.sh from this branch — keep PR scoped; parallel sandbox lives on wip/parked-local-work-20260504. Co-authored-by: Cursor <cursoragent@cursor.com>
gamescoot
force-pushed
the
cursor/error-reporting-to-host
branch
2 times, most recently
from
ec3d099 to
b13b8d2
Compare
When you build the testing project as a component or run tests locally, tool4d drops files we don't want in the repo. Add the following entries to `.gitignore` so they stop showing up in `git status`: - Components/ (the compiled .4dbase output) - testing/Logs/ (4D runtime logs) - testing/test-results/ (JUnit XML and other test output) - testing/*.4DD, *.Match (datafiles created on first run) - .DS_Store (macOS Finder metadata)
gamescoot
force-pushed
the
cursor/error-reporting-to-host
branch
from
b13b8d2 to
e114b5d
Compare
…SWE-44545)
Why
---
When a test running through the testing component hits a runtime
error (e.g. dereferencing a null object), the component's own
`ON ERR CALL` handler can only see code that runs inside the testing
database. Calls into the *host* project — where the test method
actually lives — are invisible. That makes runtime failures hard to
diagnose from the test report: you see a test failed with "An
Alphanumeric argument was expected", but not where in your code it
happened.
What
----
Add an opt-in path that lets the host project install its own error
handler so the full call stack (host + component code) ends up
attached to the failing test.
1. New host-side error handlers
`Methods/TestErrorHandler.4dm` and
`Methods/TestGlobalErrorHandler.4dm` are intended to be installed
by the host project via `ON ERR CALL`. When a runtime error
fires, they:
- walk `Last errors` and record one call-chain entry per call,
- drop only the handler's own entry (so real test methods that
happen to live in the testing database stay in the chain),
- push the resulting error (with its call chain) onto
`Storage.testErrors`.
The host wires them up before invoking the runner, so the runner
doesn't need to know anything about them — it just looks for
pre-captured errors in shared Storage.
2. Test runner picks them up
`TestRunner` / `_TestFunction` now drain `Storage.testErrors`
(and the host's `Storage.testErrors` when running as a component)
at the end of each test. For each captured error they:
- record it as a `runtimeErrors` entry on the test result,
- synthesize a failed assertion ("Runtime error: …") so the
test is reported as failed in every output format,
- surface the captured call chain on the test result.
3. New user-params for choosing what JSON includes
callchain=true include `callChain` on terse JSON failures
(verbose JSON has always included it; this
makes it available without going full verbose)
outputPath=<f> write JSON results to <f> instead of stdout.
Stdout intentionally still contains [DEBUG]
framing for human-debugged runs, so use
`outputPath=` when you need clean parseable
JSON.
4. Makefile cleanup
The Makefile now threads `format`, `tags`, `test`, `requireTags`,
`parallel`, `maxWorkers`, `outputPath`, `verbose`, and `callchain`
through `--user-param`, and routes every convenience target
(`test-junit`, `test-ci`, `test-parallel*`, …) through the main
`test` target. So whichever entry point you use, the same
parameter handling applies.
5. `Resources/en.lproj/syntaxEN.json` regenerated to match the new
public symbols.
Co-authored-by: Cursor <cursoragent@cursor.com>
The JUnit XML report had three problems that made CI consumers
(Jenkins, GitHub/GitLab test reports, etc.) display nonsense.
1. <testsuites> failures and errors were wrong, sometimes negative
----------------------------------------------------------------
Real example output for a run with 24 actual failures:
<testsuites tests="1185" failures="-880" errors="904" …>
`_buildJUnitXML` was adding `globalErrorCount` (the count of
external, non-test runtime errors that fire outside any test) into
the @errors attribute, then computing
@failures = results.failed − @errors. When external errors
outnumbered failed tests, @failures went negative.
External runtime errors are already reported in `<system-err>` and
should not contribute to @errors at all. Stop mixing them in:
@errors now counts only failed *tests* that had a runtime error,
and @failures = results.failed − @errors is correct again.
2. @timestamp was malformed
------------------------
Real example: timestamp="2026-05-04T04:00:00ZT14:29:40".
`String(Current date; ISO date GMT)` already returns a full
"YYYY-MM-DDT00:00:00Z" string, but the code was appending
"T" + Current time on top of it, producing the doubled "ZT…".
Fix: take just the YYYY-MM-DD prefix from that call and append the
local time, producing plain ISO 8601
"YYYY-MM-DDTHH:MM:SS". The trailing Z is dropped because
`Current date` / `Current time` are local values, not UTC.
3. Skipped tests were invisible to JUnit consumers
------------------------------------------------
Skipped testcases were emitted as bare self-closing
`<testcase … />` elements, indistinguishable from passing tests.
Fix: emit a `<skipped/>` child on those `<testcase>`s and add a
matching @Skipped attribute to `<testsuite>` and `<testsuites>` so
counts roll up correctly.
Verified on the testing project's own suite (208 / 207 / 0 / 1) and
Symphony's full suite (1182 / 1143 / 22 / 17): both now report the
expected totals, plain ISO timestamps, and skipped <testcase>s with
<skipped/> children. Existing call-stack output inside <failure> /
<error> CDATA is unchanged.
gamescoot
force-pushed
the
cursor/error-reporting-to-host
branch
from
e114b5d to
be2c7cc
Compare
Update README, AGENTS, CLAUDE, AI_AGENT_TESTING_GUIDE, and docs/guide for the PR's surface changes: - Document the new --user-param flags: outputPath (JSON + JUnit file output) and callchain (terse-JSON callChain on failed tests). - Refresh JSON output samples to the actual current shape (tests/passed/failed/ skipped/duration/rate/status, globalErrors/globalErrorCount, per-test runtimeErrors[], failures[].callChain when verbose or callchain=true). - Update JUnit XML samples to show skipped="N" attributes, the <skipped/> child element, and the <system-err> block; explain the corrected counting rules (failures = assertion failures, errors = test runtime errors, externals isolated to <system-err>). - Add a Runtime Error Capture section explaining per-test vs global capture, the captured record schema, and where each field shows up in each output format. - Add a Host Project Integration section with a worked example of the host's RunTests entry point, TestErrorHandler / TestGlobalErrorHandler methods, the shared Storage.testErrors collection, and the preemptive-capable handler requirement. Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@testing/Project/Sources/Classes/_TestFunction.4dm`:
- Around line 393-407: _addRuntimeErrorAsAssertion currently always injects
"line"; $error.line into the assertion object causing a null "line" key; change
it to mirror _recordAssertion in Assert.4dm by only adding the "line" property
when $error.line is present and > 0 (i.e., conditionally include the "line"
entry instead of unconditionally setting "line"; $error.line), so runtime-error
assertions omit the key when no valid line number exists.
In `@testing/Project/Sources/Classes/Assert.4dm`:
- Around line 275-293: _findTestLine currently compares $frame.name only to
$t.testFunctionName, which misses class-qualified names like
"ClassName.FunctionName"; update the match in Function _findTestLine to also
check the class-qualified form by constructing the qualified name using
$t.testClassName (when not Null/empty) and compare $frame.name to either
$t.testFunctionName or ($t.testClassName + "." + $t.testFunctionName), ensuring
you guard for Null/empty $t.testClassName so you don't build an invalid string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb40df65-d24a-4713-b1d2-aef3f8b6f4d1
📒 Files selected for processing (9)
.gitignoreMakefiletesting/Project/Sources/Classes/Assert.4dmtesting/Project/Sources/Classes/TestRunner.4dmtesting/Project/Sources/Classes/Testing.4dmtesting/Project/Sources/Classes/_TestFunction.4dmtesting/Project/Sources/Methods/TestErrorHandler.4dmtesting/Project/Sources/Methods/TestGlobalErrorHandler.4dmtesting/Resources/en.lproj/syntaxEN.json
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- testing/Project/Sources/Classes/Testing.4dm
- Makefile
🚧 Files skipped from review as they are similar to previous changes (3)
- testing/Project/Sources/Methods/TestErrorHandler.4dm
- testing/Project/Sources/Methods/TestGlobalErrorHandler.4dm
- testing/Project/Sources/Classes/TestRunner.4dm
Collaborator
|
Everything looks good to me with these changes. @KyleKincer I'm assuming you'd prefer to be the one that does the merge, since you're the owner of this repo? |
Owner
@FlutesForHire Looks good to me as well! I don't mind y'all taking ownership and merging changes into this-- you have more stake in it than I do at this point. Don't let me slow you down! More than happy to look at anything you would like me to, but merge away. |
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
3 participants
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.
Summary
ON ERR CALLduring a test are captured, attached to the offending test as a structuredruntimeErrors[]entry plus a synthetic failed assertion withisRuntimeError: true, and the remaining tests still run._generateReport()is invoked fromrun()(was buried inside_runSuitesSequentially) so the report still renders after global error capture, even when execution is bumpy.ON ERR CALLcannot catch errors raised in host scope. The component now reads$hostStorage.testErrorsso a host that installs its ownTestErrorHandler/TestGlobalErrorHandlerand shares itsStoragecan route host-side errors into the per-test and global pipelines (runtimeErrors[]per test,globalErrors[]/<system-err>for the rest).failures/errors/skippedcounts on<testsuites>and<testsuite>; valid ISO 8601timestamp(was producing…T00:00:00ZT16:47:10);<skipped/>child on every skipped<testcase>so consumers (Jenkins, JUnitXML, GitLab) classify them correctly. External runtime errors live in a<system-err>block instead of inflatingerrorsand drivingfailuresnegative.outputPath=writes JSON or JUnit reports to disk (sidesteps any debug noise on stdout);callchain=trueaddscallChainto terse-JSON failures without going full verbose. Both flow through every Makefile target.lineandfunctionName(qualifiedClass.method) when the call chain has the test frame; runtime errors are mirrored intoruntimeErrors[]withfunctionNameandformula; captured errors include acallChainJSONsnapshot taken at the moment the error fired.README,docs/guide.md,AGENTS,CLAUDE, andAI_AGENT_TESTING_GUIDEupdated for the new flags, refreshed JSON / JUnit samples, the captured-error schema, and the host-integration wiring.Changes
.gitignoreComponents/,testing/Logs/,testing/test-results/,testing/*.4DD,testing/*.Match,Makefile.local,.DS_Store)MakefileoutputPath/verbose/callchainto--user-param. Reroute every secondary target (test-junit,test-ci,test-parallel*, etc.) through$(MAKE) test …so all flags compose consistently.testing/Project/Sources/Classes/Testing.4dmtestFunctionName/testClassNameproperties (initialized in the constructor, reset inresetForNewTest) so reports can attribute assertions and errors to a qualified test name.testing/Project/Sources/Classes/Assert.4dm_recordAssertionnow recordslineandfunctionNamewhen the call chain contains the test frame; new_findTestLine($t; $callChain)helper walks the chain looking for that frame.testing/Project/Sources/Classes/_TestFunction.4dmStorage.testErrorsand host$hostStorage.testErrors(matched byprocessNumber); each error getsfunctionName/formulaand a synthetic failed assertion via new_addRuntimeErrorAsAssertion;failureCallChainpopulated fromerror.callChainJSON; new_getHostStoragehelper;_errorBelongsToProcessno longer falls back toTruefor unowned errors. Transaction cleanup simplified.testing/Project/Sources/Classes/TestRunner.4dm_generateReport()moved intorun()so it runs after global error capture; newcallChainOutputproperty gates terse-JSONcallChain(verbose=trueORcallchain=true); preps and drainshostStorage.testErrors(init + global drain); JSON terse mode emits per-testruntimeErrors[]and honorsoutputPath=via new_writeJSONToFile; JUnit fix: accuratefailures/errors/skippedcounts on<testsuites>and<testsuite>,<skipped/>child for skipped cases, valid ISO 8601 timestamp; shared_resolveOutputFilehandles relative + absolute (POSIX / Windows) paths; human-mode call chains now gated onverbose=true.testing/Project/Sources/Methods/TestErrorHandler.4dmLast errors[0].messagefor the human-readablemessagefield; capturesGet call chainimmediately and stores it ascallChainJSONon the error record; chain filter only drops the handler frame itself (preserves test-method frames in thetestingdatabase).testing/Project/Sources/Methods/TestGlobalErrorHandler.4dmTestErrorHandler.testing/Resources/en.lproj/syntaxEN.jsonREADME.md,AGENTS.md,CLAUDE.md,AI_AGENT_TESTING_GUIDE.md,docs/guide.mdTest plan
Prerequisites
HRS/SWE-44545-error-handling-testTesting.4dbasecomponent into your symphonyComponents/folderSteps
Baseline — full suite passes across all three output formats
All three exit
0. Confirm:test-results/report.jsonis a single valid JSON document containingtests,passed,failed,skipped, plusglobalErrorCountandglobalErrors— these fields are the host-error pathway showing it's wired (typically0/[]when symphony is healthy).test-results/junit.xmlopens with<testsuites tests="N" failures="0" errors="0" skipped="M" time="…" timestamp="YYYY-MM-DDTHH:MM:SS">. Thetimestampis a valid ISO 8601 (noZ, no doubledT…). Each skipped<testcase>contains a<skipped/>child. A<system-err>block is present (empty when no host errors fired, populated when they did).Per-test runtime error — captured, reported, non-fatal
In
UpdateSOLocationTest.test_empty_updates_collection, addALERT($result.length)as the first line of the body (forces[8] Alphanumeric argument expected), then:make test format=json test=UpdateSOLocationTest outputPath=/tmp/r.jsonIn
/tmp/r.json:"tests": 13, "passed": 12, "failed": 1, "status": "fail"— the runner did not crash, the report was still written, and the other 12 tests still ran.assertions[]contains a synthetic entry withisRuntimeError: true,actual: "[8] An Alphanumeric argument was expected.",line: 3,functionName: "UpdateSOLocationTest.test_empty_updates_collection".runtimeErrors[]containscode: 8,message: "An Alphanumeric argument was expected.",formula: "ALERT($result.length)".callchain=trueenriches terse JSON without going full verboseWith the broken test still in place:
make test format=json test=UpdateSOLocationTest callchain=true outputPath=/tmp/r-cc.jsonIn
/tmp/r-cc.json, thefailures[]entry fortest_empty_updates_collectionnow carries acallChaincollection containing the test-method frame (and surrounding stack). Re-running withoutcallchain=trueconfirms it disappears — terse stays terse by default.Host-side capture proves the component/host boundary (optional but the headline feature)
Move the
ALERTout of the test method into a tiny host helper:Call
_HostErrorHelper()from the test in place of the inlineALERT. Re-run the step 2 command. The runtime error still surfaces on the test with the sameruntimeErrors[]shape — but this time the error fired in host scope, where the component's ownON ERR CALLcannot reach. It only reaches the report because the host'sTestErrorHandlerpushed it ontoStorage.testErrors, which the component drained via$hostStorage.testErrors.Cleanup — Revert the
ALERTchange(s) and delete_HostErrorHelperif you added it.make testreturns all green.Made with Cursor