feat: streamlined output, failure detection, and call chain support for external errors#32
Open
gamescoot wants to merge 3 commits into
Open
feat: streamlined output, failure detection, and call chain support for external errors#32gamescoot wants to merge 3 commits into
gamescoot wants to merge 3 commits into
Conversation
Reorder the human-readable report so external errors and failed tests print before the summary, making the summary the last section visible. Co-authored-by: Cursor <cursoragent@cursor.com>
- Route all test output to stdout (LOG EVENT Information message) - Remove DEBUG log lines from TestRunner - Filter tool4d cooperative-yield warnings via FIFO + grep for real-time output - Add instant Ctrl+C handling (SIGKILL via trap on background PID) - Suppress stderr by default (debug=true to re-enable) - Deduplicate external runtime errors in human report (show count per unique error) Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
📝 WalkthroughWalkthroughAdds ChangesTest Output & Debug Logging Refinement
Sequence Diagram(s)sequenceDiagram
participant User
participant Makefile
participant tool4d
participant Filter
User->>Makefile: make test [debug=true] + USER_PARAMS
Makefile->>tool4d: launch tool4d (--user-param outputPath,...)
tool4d->>Filter: stdout stream
tool4d->>Makefile: stderr (suppressed unless debug=true)
Filter->>Makefile: filtered stdout to console
Makefile->>Makefile: trap -> cleanup & exit with tool4d status
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (3)
testing/Project/Sources/Classes/TestRunner.4dm (1)
410-428: 💤 Low valueConsider more robust key generation for error grouping.
The error grouping key uses underscore concatenation:
String(code)+"_"+text+"_"+String(line). While functional, this could theoretically produce collisions if error text contains patterns like"123_Some error_45"that match the separator scheme.For more robust deduplication, consider using a delimiter-free approach:
🔧 Alternative approaches
Option 1: Use JSON.stringify for key generation
-$key:=String:C10($error.code || 0)+"_"+($error.text || "")+"_"+String:C10($error.line || 0) +$key:=JSON Stringify:C1217(New object:C1471("c"; $error.code || 0; "t"; $error.text || ""; "l"; $error.line || 0))Option 2: Use a more unique delimiter
-$key:=String:C10($error.code || 0)+"_"+($error.text || "")+"_"+String:C10($error.line || 0) +$key:=String:C10($error.code || 0)+"\t"+($error.text || "")+"\t"+String:C10($error.line || 0)The current implementation works correctly for typical error messages, so this is primarily a defensive coding suggestion.
🤖 Prompt for 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. In `@testing/Project/Sources/Classes/TestRunner.4dm` around lines 410 - 428, The current key assembly in _groupGlobalErrors() using String:C10($error.code || 0)+"_"+($error.text || "")+"_"+String:C10($error.line || 0) can collide when the error text contains the separator; change the key to a delimiter-free representation such as JSON.stringify of a small object or another unambiguous encoding (e.g., base64 of concatenated fields) built from $error.code, $error.text and $error.line so $grouped keys are unique; update the key generation only (keep $grouped, the grouping logic and This:C1470._formatGlobalErrorForLog unchanged).testing/Project/Sources/Classes/ParallelTestRunner.4dm (1)
192-192: 💤 Low valueConsider whether timeout warrants higher visibility.
A 5-minute timeout for parallel test execution is a critical failure condition that indicates stuck or hanging tests. Logging it as
Information messagemay make it too easy to miss in CI/CD pipelines or when reviewing logs, especially since the Makefile now suppresses stderr by default.While this aligns with the PR's goal of routing all output to stdout, consider whether this particular failure mode deserves to remain at a higher severity or at least be prefixed with "ERROR:" in the message text itself.
🤖 Prompt for 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. In `@testing/Project/Sources/Classes/ParallelTestRunner.4dm` at line 192, The timeout message logged by LOG EVENT:C667 is currently emitted as an Information message (Into system standard outputs:K38:9; Information message:K38:1) but should be elevated for visibility; update the LOG EVENT:C667 invocation to either use an error severity token instead of Information message or prepend the text with an explicit "ERROR:" prefix (e.g., change the severity symbol from Information message:K38:1 to Error message or alter the string to "ERROR: Parallel test execution timeout after 5 minutes\r\n") so the timeout is clearly visible in CI logs.Makefile (1)
68-80: ⚡ Quick winConsider safer FIFO creation and graceful signal handling.
FIFO creation: The
mktemp -ufollowed bymkfifosequence has a narrow race window where another process could create a file with the same temporary name. Usemktemp -dto create a temporary directory first, then create the FIFO inside it:fifo=$(mktemp -d)/fifo; mkfifo "$fifo".Signal handling: The trap immediately uses
kill -KILL, preventing the filter process from flushing buffered output. Trykill -TERMfirst with a brief grace period before falling back tokill -KILL:trap 'disown $$pid $$filter_pid 2>/dev/null; kill -TERM $$filter_pid 2>/dev/null; sleep 0.1; kill -KILL $$pid $$filter_pid 2>/dev/null; rm -f $$fifo; exit 130' INT TERM; \The rest of the logic correctly preserves exit codes and cleans up resources in both normal and error paths.
🤖 Prompt for 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. In `@Makefile` around lines 68 - 80, The FIFO creation and trap should be made race-free and allow graceful shutdown: replace the mktemp -u / mkfifo sequence with creating a temp directory (dir=$$(mktemp -d)) and then fifo="$$dir/fifo"; mkfifo "$$fifo" (ensure you remove the fifo and rmdir "$$dir" in cleanup), update uses of `@fifo` to the fifo variable, and harden quoting; update the trap that currently does kill -KILL immediately to first disown $$pid $$filter_pid 2>/dev/null; kill -TERM $$filter_pid $$pid 2>/dev/null; sleep 0.1; kill -KILL $$pid $$filter_pid 2>/dev/null; rm -f "$$fifo"; rmdir "$$dir" 2>/dev/null; exit 130 so the filter (YIELD_FILTER) and main tool (TOOL4D) get a chance to flush before forcible kill.
🤖 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.
Nitpick comments:
In `@Makefile`:
- Around line 68-80: The FIFO creation and trap should be made race-free and
allow graceful shutdown: replace the mktemp -u / mkfifo sequence with creating a
temp directory (dir=$$(mktemp -d)) and then fifo="$$dir/fifo"; mkfifo "$$fifo"
(ensure you remove the fifo and rmdir "$$dir" in cleanup), update uses of `@fifo`
to the fifo variable, and harden quoting; update the trap that currently does
kill -KILL immediately to first disown $$pid $$filter_pid 2>/dev/null; kill
-TERM $$filter_pid $$pid 2>/dev/null; sleep 0.1; kill -KILL $$pid $$filter_pid
2>/dev/null; rm -f "$$fifo"; rmdir "$$dir" 2>/dev/null; exit 130 so the filter
(YIELD_FILTER) and main tool (TOOL4D) get a chance to flush before forcible
kill.
In `@testing/Project/Sources/Classes/ParallelTestRunner.4dm`:
- Line 192: The timeout message logged by LOG EVENT:C667 is currently emitted as
an Information message (Into system standard outputs:K38:9; Information
message:K38:1) but should be elevated for visibility; update the LOG EVENT:C667
invocation to either use an error severity token instead of Information message
or prepend the text with an explicit "ERROR:" prefix (e.g., change the severity
symbol from Information message:K38:1 to Error message or alter the string to
"ERROR: Parallel test execution timeout after 5 minutes\r\n") so the timeout is
clearly visible in CI logs.
In `@testing/Project/Sources/Classes/TestRunner.4dm`:
- Around line 410-428: The current key assembly in _groupGlobalErrors() using
String:C10($error.code || 0)+"_"+($error.text || "")+"_"+String:C10($error.line
|| 0) can collide when the error text contains the separator; change the key to
a delimiter-free representation such as JSON.stringify of a small object or
another unambiguous encoding (e.g., base64 of concatenated fields) built from
$error.code, $error.text and $error.line so $grouped keys are unique; update the
key generation only (keep $grouped, the grouping logic and
This:C1470._formatGlobalErrorForLog unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c50efbfc-2172-41d9-84eb-63982b595a3c
📒 Files selected for processing (5)
AGENTS.mdCLAUDE.mdMakefiletesting/Project/Sources/Classes/ParallelTestRunner.4dmtesting/Project/Sources/Classes/TestRunner.4dm
…add failure detection - Always stream human-readable report to stdout regardless of format parameter - Require outputPath for format=json and format=junit (error + fallback without it) - Remove JSON stdout fallback (file-write only) - Add hasFailed() method and write error file for non-zero exit propagation - Update _logFooter with clear FAILED/passed banner - Increment JUnit <testsuites> errors when external runtime errors detected - Display call chains for external errors when callchain=true or verbose=true - Add outputPath to all Makefile json/junit targets - Filter cooperative-yield noise via FIFO without buffering - Update docs with new output behavior and outputPath requirement Co-authored-by: Cursor <cursoragent@cursor.com>
gamescoot
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@Makefile`:
- Line 47: The current STDREDIRECT assignment uses a truthy check on the debug
variable so values like "false" disable stderr redirection; change the condition
to test explicitly for debug being "true" (e.g., use a Makefile equality/filter
check against the debug variable) so stderr is only left unredirected when debug
is exactly "true"; update the STDERROR_REDIRECT (STDERR_REDIRECT) assignment to
use this explicit check referencing the debug variable.
- Around line 70-83: The rule can fail from a stale "$(ERROR_FILE)" left by a
previous run; before creating the fifo and launching processes (the block that
uses mkfifo, $(YIELD_FILTER), $(TOOL4D), etc.) ensure you delete any existing
"$(ERROR_FILE)" at the start of the recipe so old markers don't trigger a false
exit; add a step that checks for and removes "$(ERROR_FILE)" (or truncates it)
prior to creating $$fifo and starting $(TOOL4D) and the filter, preserving the
rest of the trap/wait/cleanup logic.
In `@testing/Project/Sources/Classes/TestRunner.4dm`:
- Around line 671-673: The code is incorrectly folding global/external runtime
errors into the JUnit suite error count by incrementing $totalErrors when
This:C1470.results.hasGlobalErrors is true; remove that increment and instead
emit those global errors to the test runner's system-err output stream (where
other global/runtime diagnostics are written) so they are reported but not
included in testsuites/@errors; specifically, stop modifying $totalErrors in the
block that checks This:C1470.results.hasGlobalErrors and call the existing
system-err reporting routine (the same handler used for other global errors) or
append the error text to the global system-err output so JUnit consumers see the
message without inflating the `@errors` attribute.
🪄 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: 23ee19a9-fc72-4585-bb9f-820b5fbec18e
📒 Files selected for processing (5)
AGENTS.mdCLAUDE.mdMakefiletesting/Project/Sources/Classes/TestRunner.4dmtesting/Resources/en.lproj/syntaxEN.json
✅ Files skipped from review due to trivial changes (2)
- CLAUDE.md
- AGENTS.md
| # - stderr redirected to /dev/null (tool4d.4DRT errors) | ||
| # - stdout cooperative-yield warnings stripped via FIFO + grep | ||
| # Pass debug=true to disable stderr suppression. | ||
| STDERR_REDIRECT := $(if $(debug),,2>/dev/null) |
Contributor
There was a problem hiding this comment.
Gate stderr suppression only on explicit debug=true.
Current condition treats any non-empty debug value (including debug=false) as debug mode.
Suggested fix
-STDERR_REDIRECT := $(if $(debug),,2>/dev/null)
+STDERR_REDIRECT := $(if $(filter true,$(debug)),,2>/dev/null)🤖 Prompt for 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.
In `@Makefile` at line 47, The current STDREDIRECT assignment uses a truthy check
on the debug variable so values like "false" disable stderr redirection; change
the condition to test explicitly for debug being "true" (e.g., use a Makefile
equality/filter check against the debug variable) so stderr is only left
unredirected when debug is exactly "true"; update the STDERROR_REDIRECT
(STDERR_REDIRECT) assignment to use this explicit check referencing the debug
variable.
Comment on lines
+70
to
+83
| @fifo=$$(mktemp -u).fifo; mkfifo $$fifo; \ | ||
| $(YIELD_FILTER) < $$fifo & filter_pid=$$!; \ | ||
| if [ -n "$(USER_PARAMS)" ]; then \ | ||
| $(TOOL4D) $(BASE_OPTS) --user-param "$(USER_PARAMS)" $(STDERR_REDIRECT) > $$fifo & \ | ||
| else \ | ||
| $(TOOL4D) $(BASE_OPTS); \ | ||
| fi | ||
| $(TOOL4D) $(BASE_OPTS) $(STDERR_REDIRECT) > $$fifo & \ | ||
| fi; \ | ||
| pid=$$!; \ | ||
| trap 'disown $$pid $$filter_pid 2>/dev/null; kill -KILL $$pid $$filter_pid 2>/dev/null; rm -f $$fifo; exit 130' INT TERM; \ | ||
| wait $$pid; ret=$$?; \ | ||
| kill $$filter_pid 2>/dev/null; wait $$filter_pid 2>/dev/null; \ | ||
| rm -f $$fifo; \ | ||
| if [ -f "$(ERROR_FILE)" ]; then rm -f "$(ERROR_FILE)"; exit 1; fi; \ | ||
| exit $$ret |
Contributor
There was a problem hiding this comment.
Clear stale $(ERROR_FILE) before each run to avoid false failures.
If a stale marker exists (e.g., interrupted previous run), this target exits 1 even when current tests pass.
Suggested fix
test: $(TOOL4D)
- `@fifo`=$$(mktemp -u).fifo; mkfifo $$fifo; \
+ `@rm` -f "$(ERROR_FILE)"; \
+ fifo=$$(mktemp -u).fifo; mkfifo $$fifo; \
$(YIELD_FILTER) < $$fifo & filter_pid=$$!; \
if [ -n "$(USER_PARAMS)" ]; then \
$(TOOL4D) $(BASE_OPTS) --user-param "$(USER_PARAMS)" $(STDERR_REDIRECT) > $$fifo & \
else \
$(TOOL4D) $(BASE_OPTS) $(STDERR_REDIRECT) > $$fifo & \
fi; \
pid=$$!; \
- trap 'disown $$pid $$filter_pid 2>/dev/null; kill -KILL $$pid $$filter_pid 2>/dev/null; rm -f $$fifo; exit 130' INT TERM; \
+ trap 'disown $$pid $$filter_pid 2>/dev/null; kill -KILL $$pid $$filter_pid 2>/dev/null; rm -f $$fifo "$(ERROR_FILE)"; exit 130' INT TERM; \
wait $$pid; ret=$$?; \📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @fifo=$$(mktemp -u).fifo; mkfifo $$fifo; \ | |
| $(YIELD_FILTER) < $$fifo & filter_pid=$$!; \ | |
| if [ -n "$(USER_PARAMS)" ]; then \ | |
| $(TOOL4D) $(BASE_OPTS) --user-param "$(USER_PARAMS)" $(STDERR_REDIRECT) > $$fifo & \ | |
| else \ | |
| $(TOOL4D) $(BASE_OPTS); \ | |
| fi | |
| $(TOOL4D) $(BASE_OPTS) $(STDERR_REDIRECT) > $$fifo & \ | |
| fi; \ | |
| pid=$$!; \ | |
| trap 'disown $$pid $$filter_pid 2>/dev/null; kill -KILL $$pid $$filter_pid 2>/dev/null; rm -f $$fifo; exit 130' INT TERM; \ | |
| wait $$pid; ret=$$?; \ | |
| kill $$filter_pid 2>/dev/null; wait $$filter_pid 2>/dev/null; \ | |
| rm -f $$fifo; \ | |
| if [ -f "$(ERROR_FILE)" ]; then rm -f "$(ERROR_FILE)"; exit 1; fi; \ | |
| exit $$ret | |
| `@rm` -f "$(ERROR_FILE)"; \ | |
| fifo=$$(mktemp -u).fifo; mkfifo $$fifo; \ | |
| $(YIELD_FILTER) < $$fifo & filter_pid=$$!; \ | |
| if [ -n "$(USER_PARAMS)" ]; then \ | |
| $(TOOL4D) $(BASE_OPTS) --user-param "$(USER_PARAMS)" $(STDERR_REDIRECT) > $$fifo & \ | |
| else \ | |
| $(TOOL4D) $(BASE_OPTS) $(STDERR_REDIRECT) > $$fifo & \ | |
| fi; \ | |
| pid=$$!; \ | |
| trap 'disown $$pid $$filter_pid 2>/dev/null; kill -KILL $$pid $$filter_pid 2>/dev/null; rm -f $$fifo "$(ERROR_FILE)"; exit 130' INT TERM; \ | |
| wait $$pid; ret=$$?; \ | |
| kill $$filter_pid 2>/dev/null; wait $$filter_pid 2>/dev/null; \ | |
| rm -f $$fifo; \ | |
| if [ -f "$(ERROR_FILE)" ]; then rm -f "$(ERROR_FILE)"; exit 1; fi; \ | |
| exit $$ret |
🤖 Prompt for 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.
In `@Makefile` around lines 70 - 83, The rule can fail from a stale
"$(ERROR_FILE)" left by a previous run; before creating the fifo and launching
processes (the block that uses mkfifo, $(YIELD_FILTER), $(TOOL4D), etc.) ensure
you delete any existing "$(ERROR_FILE)" at the start of the recipe so old
markers don't trigger a false exit; add a step that checks for and removes
"$(ERROR_FILE)" (or truncates it) prior to creating $$fifo and starting
$(TOOL4D) and the filter, preserving the rest of the trap/wait/cleanup logic.
Comment on lines
+671
to
+673
| If (This:C1470.results.hasGlobalErrors) | ||
| $totalErrors:=$totalErrors+1 | ||
| End if |
Contributor
There was a problem hiding this comment.
Do not fold global errors into JUnit testsuites/@errors.
This inflates suite error counts with non-test errors and breaks JUnit semantics for CI consumers.
Suggested fix
- If (This:C1470.results.hasGlobalErrors)
- $totalErrors:=$totalErrors+1
- End if
+ // Global/external runtime errors are reported in <system-err> only.
+ // Do not include them in <testsuites `@errors`>.As per coding guidelines "**/TestRunner.4dm: Global/external runtime errors ... must be reported in ... <system-err> ... but NOT counted in <testsuites>/@errors."
🤖 Prompt for 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.
In `@testing/Project/Sources/Classes/TestRunner.4dm` around lines 671 - 673, The
code is incorrectly folding global/external runtime errors into the JUnit suite
error count by incrementing $totalErrors when This:C1470.results.hasGlobalErrors
is true; remove that increment and instead emit those global errors to the test
runner's system-err output stream (where other global/runtime diagnostics are
written) so they are reported but not included in testsuites/@errors;
specifically, stop modifying $totalErrors in the block that checks
This:C1470.results.hasGlobalErrors and call the existing system-err reporting
routine (the same handler used for other global errors) or append the error text
to the global system-err output so JUnit consumers see the message without
inflating the `@errors` attribute.
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
1 participant
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
This PR improves the test framework's output behavior, failure detection, and error reporting:
Output Behavior Changes
formatparameter — you always see real-time test progressformat=jsonandformat=junitnow requireoutputPath— report is written to file only, never dumped to stdoutoutputPathis omitted for json/junit, the framework prints an error and falls back to human-only outputFailure Detection & Exit Codes
hasFailed()method: returns true when any tests fail OR external runtime errors are detectederrorfile whenhasFailed()is true, which the Makefile checks to exit non-zeromake testnow exits with code 1 on any failure (test failures or external runtime errors)<testsuites>errorscount incremented by 1 when external errors exist, ensuring CI tools correctly flag the runCall Chain Support for External Errors
callchain=trueorverbose=trueis set, external runtime error call stacks are now displayed in the human report_groupGlobalErrorspreservescallChainJSONfrom the first error in each deduplicated group_formatCallChainutilityMakefile Improvements
PROJECT_DIRandERROR_FILEvariables for error file detectionoutputPathparameterdisownto suppress "Killed: 9" messagesDocumentation
outputPathrequirement in all examplesTest Plan
Basic test run — verify human output streams in real-time:
make test test=_ExampleTestExpected: real-time ✓/✗ output, "All tests passed!" at end, exit 0
JSON output requires outputPath:
make test format=jsonExpected: error message about missing outputPath, falls back to human output
JSON output with outputPath:
make test format=json outputPath=test-results/report.json test=_ExampleTestExpected: human output streams to stdout AND json written to file
Failure detection with external errors (requires host project with runtime errors):
Expected: "FAILED: N external runtime error(s)" banner, exit code 1
Call chain display for external errors:
Expected: each grouped external error shows its "Call Stack:" with method names and line numbers
Call chains hidden by default:
Expected: external errors listed without call stacks
JUnit XML failure flag:
make test format=junit outputPath=test-results/junit.xmlExpected: if external errors exist,
<testsuites errors="N">where N > 0Ctrl+C handling:
Expected: immediate termination, no "Killed: 9" messages, exit code 130
Summary by CodeRabbit
New Features
debug=trueoption to show tool stderr during tests.Improvements
Documentation