Conversation
|
Warning Rate limit exceeded@KyleKincer has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughIntroduces opt-in parallel test execution: new ParallelTestRunner class, a worker entrypoint, test runner selection via user params, Makefile targets/params for parallel runs, documentation for usage and opt-out, and new tests including a sequential-only suite and parallel-focused tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer / Make
participant TestMethod as testing/.../Methods/test.4dm
participant Runner as ParallelTestRunner
participant WorkerPool as Worker Processes
participant Storage as Shared Storage
Dev->>TestMethod: make test-parallel (parallel=true [maxWorkers])
TestMethod->>TestMethod: Parse user params
alt parallel=true
TestMethod->>Runner: new ParallelTestRunner(params)
Runner->>Runner: Discover test suites
alt multiple suites
Runner->>Storage: Initialize shared results
Runner->>WorkerPool: Dispatch suites (signals)
WorkerPool-->>Runner: Signal with suite results
Runner->>Runner: Collect & aggregate results
else single suite
Runner->>Runner: Fallback to sequential run()
end
else
TestMethod->>Runner: new TestRunner().run()
end
Runner-->>Dev: Report (human/json)
sequenceDiagram
participant Runner as ParallelTestRunner
participant Worker as ParallelTestWorker
participant Storage as Shared Storage
Runner->>Worker: ExecuteTestSuite(suite, signal)
Worker->>Worker: Run tests, capture logs/errors
Worker->>Storage: Update totals (completed, totalTests, passed, failed)
Worker-->>Runner: Signal with suiteResults
Runner->>Runner: Process suiteResults, print details
Runner->>Worker: StopWorker
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (16)
Makefile (5)
19-19: USER_PARAMS assembly looks good; consider documenting no-space values.The parser in test.4dm splits on spaces, so any value containing spaces (e.g., tags with spaces after commas) will break into multiple tokens. Either document "values must not contain spaces" or enhance the parser to support quoted values.
63-78: Let parallel targets inherit additional user params (tags, format, excludeTags, etc.).Right now, the parallel targets hardcode the user-param string, ignoring any extra key=value variables passed to make. This breaks parity with the default test target which respects USER_PARAMS. Append USER_PARAMS and place the enforced options at the end so they take precedence.
Apply this diff:
test-parallel: - $(TOOL4D) $(BASE_OPTS) --user-param "parallel=true" + $(TOOL4D) $(BASE_OPTS) --user-param "$(strip $(USER_PARAMS) parallel=true)" # Run tests in parallel mode with JSON output test-parallel-json: - $(TOOL4D) $(BASE_OPTS) --user-param "parallel=true format=json" + $(TOOL4D) $(BASE_OPTS) --user-param "$(strip $(USER_PARAMS) parallel=true format=json)" # Run unit tests in parallel mode test-parallel-unit: - $(TOOL4D) $(BASE_OPTS) --user-param "parallel=true tags=unit" + $(TOOL4D) $(BASE_OPTS) --user-param "$(strip $(USER_PARAMS) parallel=true tags=unit)" # Run tests in parallel with custom worker count (usage: make test-parallel-workers WORKERS=4) test-parallel-workers: - $(TOOL4D) $(BASE_OPTS) --user-param "parallel=true maxWorkers=$(WORKERS)" + $(TOOL4D) $(BASE_OPTS) --user-param "$(strip $(USER_PARAMS) parallel=true maxWorkers=$(WORKERS))"
92-96: Help text: consider clarifying precedence and pass-through behavior.If you adopt the change above, a short note that these targets also honor extra params (e.g., excludeTags) and that later key=value pairs override earlier ones will reduce confusion.
107-111: Examples: add one for test-parallel-unit and a combined example with extra filters.To improve discoverability, consider adding:
- make test-parallel-unit
- make test-parallel parallel=true excludeTags=slow (if you keep parallel=true enforced by the target, the explicit parallel=true isn’t needed)
112-112: Add common phony targets “all” and “clean” or silence the linter.Static analysis flags missing “all” and “clean” phony targets. Either add no-op targets or configure checkmake to ignore them if they’re intentionally omitted.
Proposed minimal addition:
-.PHONY: test test-json test-class test-tags test-exclude-tags test-require-tags test-unit test-integration test-unit-json test-parallel test-parallel-json test-parallel-unit test-parallel-workers help +.PHONY: all clean test test-json test-class test-tags test-exclude-tags test-require-tags test-unit test-integration test-unit-json test-parallel test-parallel-json test-parallel-unit test-parallel-workers help + +all: test + +clean: + @truetesting/Project/Sources/Methods/test.4dm (2)
3-6: Remove unused variable $real.$real is assigned but never used. You can call Get database parameter to populate $userParam without storing the return value.
Apply this diff:
-var $real : Real -$real:=Get database parameter:C643(User param value:K37:94; $userParam) +// Acquire user parameter string from the launcher +Get database parameter:C643(User param value:K37:94; $userParam)
15-27: Don’t strip all spaces from tokens—trim them.Replace string(...; " "; "") removes all spaces, which will corrupt values if they legitimately contain spaces (and also breaks patterns if people type “tags=unit, fast”). Prefer trimming leading/trailing whitespace instead of removing all spaces.
If you want to preserve internal spaces, switch to a trim function rather than Replace string. If native Trim is not available in this context, consider a helper that removes only leading/trailing spaces.
CLAUDE.md (2)
130-135: Minor grammar tweak for clarity.Add “the” for readability: “before generating the final report”.
Apply this diff:
-5. **Result Aggregation**: Collects and merges results from all workers before generating final report +5. **Result Aggregation**: Collects and merges results from all workers before generating the final report
153-156: End bullets with periods for consistency and readability.The bullets read better with terminal punctuation.
Apply this diff:
-- **30-60% reduction** in total test runtime for typical test suites -- **Better resource utilization** on multi-core machines -- **Improved developer experience** with faster feedback loops -- **CI/CD optimization** reducing pipeline duration +- **30-60% reduction** in total test runtime for typical test suites. +- **Better resource utilization** on multi-core machines. +- **Improved developer experience** with faster feedback loops. +- **CI/CD optimization** reducing pipeline duration.testing/Project/Sources/Methods/ParallelTestWorker.4dm (1)
51-82: Failure metadata capturing is thoughtful; consider also capturing the last log line.You currently use the first log message as failureReason. In many frameworks, the most recent message is often the error; consider using the last message when present. Not critical.
Potential tweak:
- If logMessages.length > 0, use the last element for failureReason.
testing/Project/Sources/Classes/_ParallelExecutionTest.4dm (2)
61-74: Add assertions to validate sequential fallback pathThis test has no assertions and doesn’t call any code that exercises the fallback. Either remove the test or assert observable behavior (e.g., no workers started, or that run() delegates to sequential when only one suite is available).
If keeping the test, consider this minimal approach that avoids re-discovery wiping your setup:
- Configure discover filters to yield only one suite for the run()
- Or, inspect that $runner.workerProcesses remains empty after invoking a “dry” execution path
Would you like me to propose a refactor of run() to accept pre-supplied suites for testing?
75-89: Lightweight check, consider adding a post-dispatch assertionAsserting initial emptiness and configured maxWorkers is fine. Optionally, you could dispatch a dummy suite and assert workerProcesses is populated, but that may be too integration-heavy for a unit test.
testing/Project/Sources/Classes/ParallelTestRunner.4dm (4)
251-255: Declare loop variable for clarity and to avoid implicit variant$workerName is used without a local declaration in this method. Declare it explicitly.
Apply this diff:
Function _waitForCompletionAndCollectResults() // Wait for parallel test suites to complete var $parallelSuiteCount : Integer var $totalSuites : Integer + var $workerName : Text
80-94: Be more permissive when parsing user params (true/1/yes)Parsing parallel flag strictly as "true" may surprise users invoking via env/Makefile with 1/yes/True. Consider normalizing and accepting common truthy values; similarly, clamp maxWorkers and coerce invalid values to defaults.
Example change:
-This:C1470.parallelMode:=($params.parallel="true") +If ($params.parallel#Null:C1517) + var $p : Text + $p:=Lowercase:C11(String:C10($params.parallel)) + This:C1470.parallelMode:=($p="true") | ($p="1") | ($p="yes") +End if
315-342: Make opt-out parsing robust to casing and whitespace variantsHandle values like “False”, “ false ”, or mixed case.
Apply this diff:
-If ($classCode#"") +If ($classCode#"") var $lines : Collection $lines:=Split string:C1554($classCode; Char:C90(Carriage return:K15:38)) var $line : Text For each ($line; $lines) // Look for #parallel: comments If (Position:C15("#parallel:"; $line)>0) var $parallelValue : Text $parallelValue:=Substring:C12($line; Position:C15("#parallel:"; $line)+10) - $parallelValue:=Replace string:C233($parallelValue; " "; "") // Remove spaces + $parallelValue:=Replace string:C233($parallelValue; " "; "") // Remove spaces + $parallelValue:=Lowercase:C11($parallelValue) // Check for explicit disable If ($parallelValue="false") return False:C215 End if End if End for each End if
3-9: Remove unused properties (sharedResults, completedSuites) or wire them upThese properties are initialized but unused. Either remove them to reduce noise or integrate them into the aggregation path.
Apply this diff to remove for now:
property parallelMode : Boolean property maxWorkers : Integer property workerProcesses : Collection // Collection of worker process names -property sharedResults : Object // Shared storage for collecting results -property completedSuites : Integer // Counter for completed suites property sequentialSuites : Collection // Suites that opted out of parallel executionAnd delete the corresponding initialization lines in the constructor:
This:C1470.workerProcesses:=[] -This:C1470.completedSuites:=0 This:C1470.sequentialSuites:=[]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
CLAUDE.md(4 hunks)Makefile(4 hunks)testing/Project/Sources/Classes/ParallelTestRunner.4dm(1 hunks)testing/Project/Sources/Classes/_ParallelExecutionTest.4dm(1 hunks)testing/Project/Sources/Classes/_SequentialOnlyTest.4dm(1 hunks)testing/Project/Sources/Methods/ParallelTestWorker.4dm(1 hunks)testing/Project/Sources/Methods/test.4dm(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
testing/Project/Sources/Classes/*Test.4dm
📄 CodeRabbit Inference Engine (CLAUDE.md)
testing/Project/Sources/Classes/*Test.4dm: Name 4D test classes with the suffix "Test" so they are auto-discovered (e.g., TaggingSystemTest.4dm).
Within test classes, name test methods with the prefix "test_" to be executed by the runner.
Use comment-based tagging in tests with the format "// #tags: ..." (e.g., // #tags: unit, integration, slow) to enable tag-based filtering.
Files:
testing/Project/Sources/Classes/_ParallelExecutionTest.4dmtesting/Project/Sources/Classes/_SequentialOnlyTest.4dm
🧠 Learnings (1)
📚 Learning: 2025-08-11T20:17:51.964Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-11T20:17:51.964Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Use comment-based tagging in tests with the format "// #tags: ..." (e.g., // #tags: unit, integration, slow) to enable tag-based filtering.
Applied to files:
testing/Project/Sources/Classes/_ParallelExecutionTest.4dm
🪛 checkmake (0.2.2)
Makefile
[warning] 112-112: Missing required phony target "all"
(minphony)
[warning] 112-112: Missing required phony target "clean"
(minphony)
🪛 LanguageTool
CLAUDE.md
[grammar] ~134-~134: There might be a mistake here.
Context: ... merges results from all workers before generating final report ### Parallel Execution Op...
(QB_NEW_EN)
[grammar] ~153-~153: There might be a mistake here.
Context: ...tal test runtime for typical test suites - Better resource utilization on multi-c...
(QB_NEW_EN)
[grammar] ~154-~154: There might be a mistake here.
Context: ...rce utilization** on multi-core machines - Improved developer experience with fas...
(QB_NEW_EN)
[grammar] ~155-~155: There might be a mistake here.
Context: ... experience** with faster feedback loops - CI/CD optimization reducing pipeline d...
(QB_NEW_EN)
🔇 Additional comments (20)
testing/Project/Sources/Methods/test.4dm (2)
31-35: Runner selection is straightforward and correct.The parameter-driven selection between ParallelTestRunner and TestRunner is clean. Comparing the string value "true" is fine given the current parsing logic.
37-46: Run path unified; QUIT behavior retained.Using the same $runner for both headless and non-headless paths reduces duplication. QUIT 4D guard remains scoped to the appropriate application types.
testing/Project/Sources/Classes/_SequentialOnlyTest.4dm (3)
1-3: Parallel opt-out annotation is clear and compliant.The class is suffixed with “Test” and clearly opts out of parallel execution via // #parallel: false. Good alignment with discovery rules.
6-16: Test method naming and assertions look correct.Method starts with test_, includes tags, and uses areEqual with expected/actual in the right order.
17-26: Second test is well-formed and tagged.Uses isTrue with a Boolean flag and a helpful message. Nothing to change.
CLAUDE.md (1)
111-126: Strong, actionable documentation for enabling parallel execution.Clear examples and alignment with Makefile targets and user params. Nicely done.
testing/Project/Sources/Methods/ParallelTestWorker.4dm (8)
26-33: Result envelope is transport-friendly; good separation.Creating a clean, typed result object with name/tests/passed/failed is solid. Using basic types ensures safe cross-process transfer.
35-49: Per-test result extraction is careful and avoids shared refs.Using getResult() and projecting to a plain object avoids leaking runtime objects between processes. Nice.
93-99: Shared counters are updated under Use() — correct synchronization.Good use of shared object locking to avoid races when aggregating counters.
110-116: Restoring previous error handler is correct; ensure empty string resets.This correctly reinstates the prior handler or removes it. No changes needed.
117-119: Worker shutdown path is straightforward.KILL WORKER is appropriate for the StopWorker command.
19-22: No action required — TestErrorHandler existsThe method is present at testing/Project/Sources/Methods/TestErrorHandler.4dm, so the ON ERR CALL("TestErrorHandler") usage is valid.
- Found implementation: testing/Project/Sources/Methods/TestErrorHandler.4dm
- References: testing/Project/Sources/Methods/ParallelTestWorker.4dm and several testing classes set ON ERR CALL("TestErrorHandler")
10-17: No action required — _TestSuite constructor matches usageThe _TestSuite constructor is declared as:
Class constructor($class : 4D:C1709.Class; $outputFormat : Text; $testPatterns : Collection; $testRunner : cs:C1710.TestRunner)Verified call sites using the same ordering:
- testing/Project/Sources/Methods/ParallelTestWorker.4dm — cs:C1710._TestSuite.new($data.class; $data.outputFormat; $data.testPatterns; $data.testRunner)
- testing/Project/Sources/Classes/TestRunner.4dm — cs:C1710._TestSuite.new($class; This:C1470.outputFormat; This:C1470.testPatterns; This:C1470)
No change required.
101-108: Signal transport via OB Copy is correct — confirm the signal exposes trigger()ParallelTestRunner creates a New signal and passes it into the worker; the worker copies $suiteResults into a shared object on $data.signal and then calls $data.signal.trigger(). I could not find a local definition of the signal type in the repo, so please confirm the runtime/library provides trigger(); otherwise add a defensive existence check before calling trigger().
Files to check:
- testing/Project/Sources/Methods/ParallelTestWorker.4dm — lines ~101–108 (copies suiteResults to $data.signal then calls $data.signal.trigger()).
- testing/Project/Sources/Classes/ParallelTestRunner.4dm — lines ~155–169 (creates $signal := New signal:C1641 and includes it in $suiteData passed to CALL WORKER).
Suggested action: verify the signal API (trigger exists) or guard the call with a quick null/existence check before invoking trigger().
testing/Project/Sources/Classes/_ParallelExecutionTest.4dm (3)
5-18: Nice: clear parallel mode toggling and worker cap checksThese unit tests correctly exercise toggling parallel mode and configuring maxWorkers. Tags and naming also follow the project’s guidelines.
19-31: Good bounds testing for default worker countThe assertions for [1..8] align with the capped default. This will catch regressions if the cap changes unintentionally.
32-47: Positive: verifies suite filtering default pathThis test usefully validates that suites without an explicit opt-out are considered parallel-eligible.
testing/Project/Sources/Classes/ParallelTestRunner.4dm (3)
50-55: Nice touch: human-friendly header and timingThe header log including suite count and max workers improves UX for CLI runs.
238-249: Good: sequential suites are executed and collected after parallel phaseThis preserves opt-out behavior and ensures results are included in the final report.
256-304: Thoughtful per-test output and failure detail extractionDetailed pass/fail lines, runtime error vs failure reason vs log selection, and pushing failedTests will aid debugging. Nice use of OB Copy for safety.
| Function test_parallel_opt_out_parsing($t : cs:C1710.Testing) | ||
| // #tags: unit, parallel | ||
|
|
||
| // Test parsing of parallel opt-out comments | ||
| var $runner : cs:C1710.ParallelTestRunner | ||
| $runner:=cs:C1710.ParallelTestRunner.new() | ||
|
|
||
| // Mock class code with parallel opt-out | ||
| var $classCode : Text | ||
| $classCode:="// Test class with parallel opt-out\r// #parallel: false\rClass constructor()\r\rFunction test_something($t : cs.Testing)\r // Test code here\r" | ||
|
|
||
| // Test parsing logic by creating a mock test suite | ||
| // Note: This would require more complex mocking in a real scenario No newline at end of file |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Exercise real opt-out parsing using a concrete suite
The test builds a code string but never asserts behavior. Leverage the sequential-only test class present in the PR to validate _shouldRunSuiteInParallel returns False.
Apply this diff:
-// Test parsing logic by creating a mock test suite
-// Note: This would require more complex mocking in a real scenario
+// Validate that an opt-out suite is detected
+var $suite : cs:C1710._TestSuite
+$suite:=cs:C1710._TestSuite.new(cs:C1710._SequentialOnlyTest; "human"; []; $runner)
+var $shouldRun : Boolean
+$shouldRun:=$runner._shouldRunSuiteInParallel($suite)
+$t.assert.isFalse($t; $shouldRun; "Suite with // #parallel: false should not run in parallel")📝 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.
| Function test_parallel_opt_out_parsing($t : cs:C1710.Testing) | |
| // #tags: unit, parallel | |
| // Test parsing of parallel opt-out comments | |
| var $runner : cs:C1710.ParallelTestRunner | |
| $runner:=cs:C1710.ParallelTestRunner.new() | |
| // Mock class code with parallel opt-out | |
| var $classCode : Text | |
| $classCode:="// Test class with parallel opt-out\r// #parallel: false\rClass constructor()\r\rFunction test_something($t : cs.Testing)\r // Test code here\r" | |
| // Test parsing logic by creating a mock test suite | |
| // Note: This would require more complex mocking in a real scenario | |
| Function test_parallel_opt_out_parsing($t : cs:C1710.Testing) | |
| // #tags: unit, parallel | |
| // Test parsing of parallel opt-out comments | |
| var $runner : cs:C1710.ParallelTestRunner | |
| $runner:=cs:C1710.ParallelTestRunner.new() | |
| // Mock class code with parallel opt-out | |
| var $classCode : Text | |
| $classCode:="// Test class with parallel opt-out\r// #parallel: false\rClass constructor()\r\rFunction test_something($t : cs.Testing)\r // Test code here\r" | |
| // Validate that an opt-out suite is detected | |
| var $suite : cs:C1710._TestSuite | |
| $suite:=cs:C1710._TestSuite.new(cs:C1710._SequentialOnlyTest; "human"; []; $runner) | |
| var $shouldRun : Boolean | |
| $shouldRun:=$runner._shouldRunSuiteInParallel($suite) | |
| $t.assert.isFalse($t; $shouldRun; "Suite with // #parallel: false should not run in parallel") |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
KyleKincer
force-pushed
the
parallelize
branch
from
ea429b3 to
1a21f3c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Makefile (1)
46-49: Security: use HTTPS for package downloads; avoid --force-depends.Downloading .deb files over HTTP is a supply-chain risk, and forcing dpkg deps can mask install issues. Prefer HTTPS and follow with apt-get -f install.
Apply:
- curl -L -o /tmp/libtinfo5.deb http://archive.ubuntu.com/ubuntu/pool/main/n/ncurses/libtinfo5_6.1-1ubuntu1_amd64.deb; \ - curl -L -o /tmp/libncurses5.deb http://archive.ubuntu.com/ubuntu/pool/main/n/ncurses/libncurses5_6.1-1ubuntu1_amd64.deb; \ - dpkg -i /tmp/libtinfo5.deb /tmp/libncurses5.deb; \ + curl -L -o /tmp/libtinfo5.deb https://archive.ubuntu.com/ubuntu/pool/main/n/ncurses/libtinfo5_6.1-1ubuntu1_amd64.deb; \ + curl -L -o /tmp/libncurses5.deb https://archive.ubuntu.com/ubuntu/pool/main/n/ncurses/libncurses5_6.1-1ubuntu1_amd64.deb; \ + dpkg -i /tmp/libtinfo5.deb /tmp/libncurses5.deb || true; \ + apt-get -f install -y; \ - dpkg --force-depends -i /tmp/tool4d.deb; \ + dpkg -i /tmp/tool4d.deb || true; \ + apt-get -f install -y; \
♻️ Duplicate comments (4)
testing/Project/Sources/Classes/ParallelTestRunner.4dm (4)
43-53: Early-return handler restoration: fixed.Nice — this addresses the previously reported leak of the global error handler on the empty-suite path.
103-108: Bug: invalid fallback using “||” for core count.“|| 4” won’t yield a numeric fallback. Use an explicit check.
Apply:
Function _getDefaultWorkerCount() : Integer // Default to CPU count, but cap at reasonable maximum var $cpuCount : Integer - $cpuCount:=System info:C1571.numberOfCores || 4 // Default to 4 if unable to detect + $cpuCount:=System info:C1571.numberOfCores + If ($cpuCount<=0) + $cpuCount:=4 // Fallback if unavailable + End if return Num:C11(Choose:C955($cpuCount>8; 8; $cpuCount)) // Cap at 8 workers
165-174: Do not pass the runner object across processes.Passing “testRunner”: This to a worker risks cross-process object dereferences. Supply only serializable filter data.
Apply (runner side):
var $suiteData : Object $suiteData:=New object:C1471(\ "class"; $testSuite.class; \ "outputFormat"; $testSuite.outputFormat; \ "testPatterns"; $testSuite.testPatterns; \ - "testRunner"; This:C1470; \ + // Pass only serializable filter options instead of the runner object + "filterOptions"; New object:C1471(\ + "tags"; This:C1470.tags; \ + "excludeTags"; This:C1470.excludeTags; \ + "requireTags"; This:C1470.requireTags \ + ); \ "suiteIndex"; $parallelSuiteIndex; \ "signal"; $signal\ )Run to confirm worker usage of suiteData.testRunner and adapt accordingly:
#!/bin/bash # Inspect worker and suite code paths for 'suiteData.testRunner' dereferences rg -n -C3 -P --hidden --glob 'testing/**' '\bsuiteData\.testRunner\b|\$data\.testRunner\b|filterOptions\.(tags|excludeTags|requireTags)'
180-201: Double wait strategy can miss results and emit spurious 5s timeouts.You wait here per-signal (5s) and again in _waitForCompletionAndCollectResults (5 minutes). Signals that arrive after 5s get incorrectly flagged, and their results may be ignored.
Minimal mitigation now:
- $signaled:=$workerSignal.wait(5) // 5 second timeout + $signaled:=$workerSignal.wait(300) // 5-minute timeout to match overall wait - LOG EVENT:C667(Into system standard outputs:K38:9; "Warning: Worker timeout after 5 seconds\r\n"; Error message:K38:3) + LOG EVENT:C667(Into system standard outputs:K38:9; "Warning: Worker timeout after 5 minutes\r\n"; Error message:K38:3)Recommended follow-up: remove the per-worker wait loop; store workerSignals on This and do unified waiting/collection in _waitForCompletionAndCollectResults.
🧹 Nitpick comments (9)
CLAUDE.md (3)
51-56: Makefile targets: solid quick-start, consider clarifying WORKERS default.As a micro-clarification, note that when WORKERS is omitted, the runner will default to CPU cores (capped by the internal limit); add a parenthetical to reduce ambiguity.
Apply:
- make test-parallel-workers WORKERS=4 # Run with custom worker count + make test-parallel-workers WORKERS=4 # Run with custom worker count (defaults to CPU cores when omitted)
227-233: Minor grammar/hyphenation polish in “Performance Benefits.”Tighten phrasing and hyphenation.
Apply:
-- **30-60% reduction** in total test runtime for typical test suites -- **Better resource utilization** on multi-core machines -- **Improved developer experience** with faster feedback loops -- **CI/CD optimization** reducing pipeline duration +- **30–60% reduction** in total test runtime for typical test suites +- **Better resource utilization** on multi-core machines +- **Improved developer experience** with faster feedback loops +- **CI/CD optimization**, reducing pipeline duration
309-312: Transaction table: clarify default behavior.Small wording tweak for precision.
Apply:
-| No comment | Enables automatic transactions (default) | +| No comment | Enables automatic transactions (default behavior) |Makefile (3)
114-129: Parallel targets: good coverage; add a guard for empty WORKERS (optional).If WORKERS is unset, this passes an empty value (maxWorkers=). The runner ignores non-positive values, but you can provide earlier feedback.
Apply:
test-parallel-workers: - $(TOOL4D) $(BASE_OPTS) --user-param "parallel=true maxWorkers=$(WORKERS)" + @if [ -z "$(WORKERS)" ]; then \ + echo "ERROR: Specify WORKERS, e.g., 'make $@ WORKERS=4'"; \ + exit 2; \ + fi + $(TOOL4D) $(BASE_OPTS) --user-param "parallel=true maxWorkers=$(WORKERS)"
166-170: Examples: include the unit-parallel shortcut for completeness.You list test-parallel and test-parallel-json; include test-parallel-unit to round it out.
Apply:
@echo " make test-parallel" @echo " make test-parallel-json" + @echo " make test-parallel-unit" @echo " make test-parallel-workers WORKERS=4" @echo " make test parallel=true maxWorkers=6"
174-174: Minor: consider standard phony targets “all” and “clean”.Not required, but it satisfies common tooling (e.g., checkmake) and improves UX.
Apply:
.PHONY: test test-json test-junit test-ci test-class test-tags test-exclude-tags test-require-tags test-unit test-integration test-unit-json test-unit-junit test-integration-junit test-parallel test-parallel-json test-parallel-unit test-parallel-workers help tool4d + +.PHONY: all clean +all: test +clean: + @rm -rf test-results/*testing/Project/Sources/Classes/ParallelTestRunner.4dm (3)
3-10: Property set is reasonable; note: sharedResults is currently unused.If intentional for future work, leave a TODO. Otherwise, remove to avoid confusion.
Apply:
-property sharedResults : Object // Shared storage for collecting results +// TODO: remove if unused in worker aggregation +// property sharedResults : Object // Shared storage for collecting results
87-102: Bounds-check is good; add a warning when ignoring out-of-range values.Helps users understand why their input had no effect.
Apply:
If ($params.maxWorkers#Null:C1517) var $maxWorkers : Integer $maxWorkers:=Num:C11($params.maxWorkers) - If ($maxWorkers>0) && ($maxWorkers<=16) // Reasonable limits + If ($maxWorkers>0) && ($maxWorkers<=16) // Reasonable limits This:C1470.maxWorkers:=$maxWorkers - End if + Else + // Optional: surface a one-time warning in human mode + If (This:C1470.outputFormat="human") + LOG EVENT:C667(Into system standard outputs:K38:9; \ + "Ignoring maxWorkers="+String:C10($maxWorkers)+" (allowed range: 1..16)\r\n"; \ + Information message:K38:1) + End if + End if End if
125-133: Compute workerCount after filtering; avoid starting extra workers.Currently workerCount is based on total suites, not the filtered parallel set, which can over-provision idle workers.
Apply:
- $workerCount:=Num:C11(Choose:C955(This:C1470.testSuites.length<This:C1470.maxWorkers; This:C1470.testSuites.length; This:C1470.maxWorkers)) - - // Create worker process names (workers will be created on-demand by CALL WORKER) - var $i : Integer - For ($i; 1; $workerCount) - $workerName:="TestWorker_"+String:C10($i)+"_"+String:C10(Random:C100) - This:C1470.workerProcesses.push($workerName) - End for - // Distribute test suites to workers, respecting parallel opt-out var $suiteIndex : Integer var $parallelSuites : Collection var $sequentialSuites : Collection $suiteIndex:=0 $parallelSuites:=[] $sequentialSuites:=[] @@ End for each + // Derive workerCount from actual parallel workload + $workerCount:=Num:C11(Choose:C955($parallelSuites.length<This:C1470.maxWorkers; $parallelSuites.length; This:C1470.maxWorkers)) + + // Create worker process names (workers will be created on-demand by CALL WORKER) + var $i : Integer + For ($i; 1; $workerCount) + $workerName:="TestWorker_"+String:C10($i)+"_"+String:C10(Random:C100) + This:C1470.workerProcesses.push($workerName) + End forAlso applies to: 142-149, 151-156
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
CLAUDE.md(5 hunks)Makefile(4 hunks)testing/Project/Sources/Classes/ParallelTestRunner.4dm(1 hunks)testing/Project/Sources/Classes/_ParallelExecutionTest.4dm(1 hunks)testing/Project/Sources/Classes/_SequentialOnlyTest.4dm(1 hunks)testing/Project/Sources/Methods/ParallelTestWorker.4dm(1 hunks)testing/Project/Sources/Methods/test.4dm(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- testing/Project/Sources/Classes/_ParallelExecutionTest.4dm
- testing/Project/Sources/Methods/test.4dm
- testing/Project/Sources/Classes/_SequentialOnlyTest.4dm
- testing/Project/Sources/Methods/ParallelTestWorker.4dm
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-23T20:27:11.532Z
Learnt from: CR
PR: KyleKincer/testing#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-23T20:27:11.532Z
Learning: Applies to testing/Project/Sources/Classes/*Test.4dm : Disable automatic transactions for a specific test by adding the comment: // #transaction: false; omit the comment to keep the default rollback behavior
Applied to files:
CLAUDE.md
🪛 checkmake (0.2.2)
Makefile
[warning] 174-174: Missing required phony target "all"
(minphony)
[warning] 174-174: Missing required phony target "clean"
(minphony)
🪛 LanguageTool
CLAUDE.md
[grammar] ~210-~210: There might be a mistake here.
Context: ... merges results from all workers before generating final report ### Parallel Execution Op...
(QB_NEW_EN)
[grammar] ~229-~229: There might be a mistake here.
Context: ...tal test runtime for typical test suites - Better resource utilization on multi-c...
(QB_NEW_EN)
[grammar] ~230-~230: There might be a mistake here.
Context: ...rce utilization** on multi-core machines - Improved developer experience with fas...
(QB_NEW_EN)
[grammar] ~231-~231: There might be a mistake here.
Context: ... experience** with faster feedback loops - CI/CD optimization reducing pipeline d...
(QB_NEW_EN)
🔇 Additional comments (5)
CLAUDE.md (2)
14-14: Good addition: headline feature callout is clear and concise.The new “Parallel test execution” bullet fits well in the overview and matches the rest of the list.
94-98: Filtering + parallel examples look correct.Examples combine flags properly and reflect the Makefile and runner behavior.
Makefile (2)
38-38: USER_PARAMS extension is correct; ordering is fine.Adding parallel and maxWorkers into USER_PARAMS is consistent with the rest of the pipeline and the docs.
147-151: Help output is consistent.All new targets are documented here.
testing/Project/Sources/Classes/ParallelTestRunner.4dm (1)
316-321: Cleanup is correct.Shared storage is nulled under a Use block — good.
| ### How Parallel Execution Works | ||
|
|
||
| 1. **Suite-Level Parallelism**: Test suites run concurrently, but individual tests within a suite run sequentially | ||
| 2. **Worker Pool**: Creates worker processes up to the specified maximum (default: CPU core count, max: 8) | ||
| 3. **Automatic Load Balancing**: Distributes test suites across available workers | ||
| 4. **Test Isolation**: Each worker runs in its own process with separate transaction scope | ||
| 5. **Result Aggregation**: Collects and merges results from all workers before generating final report | ||
|
|
There was a problem hiding this comment.
Doc-code mismatch: max workers; plus grammar fix.
- The doc says “max: 8” (Line 208). In code, maxWorkers accepts up to 16 (bounded in _parseParallelOptions). This can confuse users. Clarify: default worker count is min(CPU cores, 8); user-provided maxWorkers is allowed up to 16.
- Grammar: “before generating final report” → “before generating the final report”.
Apply:
-2. **Worker Pool**: Creates worker processes up to the specified maximum (default: CPU core count, max: 8)
+2. **Worker Pool**: Creates worker processes up to the specified maximum
+ - Default workers: CPU core count capped at 8
+ - User override: `maxWorkers` up to 16 (values above 16 are ignored)
-5. **Result Aggregation**: Collects and merges results from all workers before generating final report
+5. **Result Aggregation**: Collects and merges results from all workers before generating the final report📝 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.
| ### How Parallel Execution Works | |
| 1. **Suite-Level Parallelism**: Test suites run concurrently, but individual tests within a suite run sequentially | |
| 2. **Worker Pool**: Creates worker processes up to the specified maximum (default: CPU core count, max: 8) | |
| 3. **Automatic Load Balancing**: Distributes test suites across available workers | |
| 4. **Test Isolation**: Each worker runs in its own process with separate transaction scope | |
| 5. **Result Aggregation**: Collects and merges results from all workers before generating final report | |
| ### How Parallel Execution Works | |
| 1. **Suite-Level Parallelism**: Test suites run concurrently, but individual tests within a suite run sequentially | |
| 2. **Worker Pool**: Creates worker processes up to the specified maximum | |
| - Default workers: CPU core count capped at 8 | |
| - User override: `maxWorkers` up to 16 (values above 16 are ignored) | |
| 3. **Automatic Load Balancing**: Distributes test suites across available workers | |
| 4. **Test Isolation**: Each worker runs in its own process with separate transaction scope | |
| 5. **Result Aggregation**: Collects and merges results from all workers before generating the final report |
🧰 Tools
🪛 LanguageTool
[grammar] ~210-~210: There might be a mistake here.
Context: ... merges results from all workers before generating final report ### Parallel Execution Op...
(QB_NEW_EN)
🤖 Prompt for AI Agents
In CLAUDE.md around lines 204 to 211, the documentation incorrectly states "max:
8" and uses slightly off grammar; update the parallel execution section to
clarify that the default worker count is min(CPU cores, 8) while a user-provided
maxWorkers value is allowed up to 16 (as enforced in _parseParallelOptions), and
change "before generating final report" to "before generating the final report".
| Function _waitForCompletionAndCollectResults() | ||
| // Wait for parallel test suites to complete | ||
| var $parallelSuiteCount : Integer | ||
| var $totalSuites : Integer | ||
|
|
||
| Use (Storage:C1525.parallelTestResults) | ||
| $totalSuites:=Storage:C1525.parallelTestResults.totalSuites | ||
| End use | ||
|
|
||
| $parallelSuiteCount:=$totalSuites-This:C1470.sequentialSuites.length | ||
|
|
||
| If ($parallelSuiteCount>0) | ||
| var $startWait : Integer | ||
| $startWait:=Milliseconds:C459 | ||
| var $timeout : Integer | ||
| $timeout:=300000 // 5 minute timeout | ||
|
|
||
| var $completedCount : Integer | ||
| $completedCount:=0 | ||
|
|
||
| While ($completedCount<$parallelSuiteCount) | ||
| Use (Storage:C1525.parallelTestResults) | ||
| $completedCount:=Storage:C1525.parallelTestResults.completedCount | ||
| End use | ||
|
|
||
| If ((Milliseconds:C459-$startWait)>$timeout) | ||
| // Timeout - log error and break | ||
| LOG EVENT:C667(Into system standard outputs:K38:9; "Parallel test execution timeout after 5 minutes\r\n"; Error message:K38:3) | ||
| break | ||
| End if | ||
|
|
||
| DELAY PROCESS:C323(Current process:C322; 10) // Wait 10 ticks | ||
| End while | ||
|
|
||
| End if | ||
|
|
||
| // Run sequential suites in main process | ||
| If (This:C1470.sequentialSuites.length>0) | ||
| If (This:C1470.outputFormat="human") | ||
| LOG EVENT:C667(Into system standard outputs:K38:9; "Running "+String:C10(This:C1470.sequentialSuites.length)+" sequential test suite(s)\r\n"; Information message:K38:1) | ||
| End if | ||
|
|
||
| var $testSuite : cs:C1710._TestSuite | ||
| For each ($testSuite; This:C1470.sequentialSuites) | ||
| $testSuite.run() | ||
| This:C1470._collectSuiteResults($testSuite) | ||
| End for each | ||
| End if | ||
|
|
||
| // Cleanup worker processes | ||
| For each ($workerName; This:C1470.workerProcesses) | ||
| CALL WORKER:C1389($workerName; "ParallelTestWorker"; "StopWorker") | ||
| End for each |
There was a problem hiding this comment.
Missing local declaration for $workerName in cleanup loop.
$workerName is not declared in this method; 4D requires explicit var.
Apply:
Function _waitForCompletionAndCollectResults()
// Wait for parallel test suites to complete
var $parallelSuiteCount : Integer
var $totalSuites : Integer
+ var $workerName : Text📝 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.
| Function _waitForCompletionAndCollectResults() | |
| // Wait for parallel test suites to complete | |
| var $parallelSuiteCount : Integer | |
| var $totalSuites : Integer | |
| Use (Storage:C1525.parallelTestResults) | |
| $totalSuites:=Storage:C1525.parallelTestResults.totalSuites | |
| End use | |
| $parallelSuiteCount:=$totalSuites-This:C1470.sequentialSuites.length | |
| If ($parallelSuiteCount>0) | |
| var $startWait : Integer | |
| $startWait:=Milliseconds:C459 | |
| var $timeout : Integer | |
| $timeout:=300000 // 5 minute timeout | |
| var $completedCount : Integer | |
| $completedCount:=0 | |
| While ($completedCount<$parallelSuiteCount) | |
| Use (Storage:C1525.parallelTestResults) | |
| $completedCount:=Storage:C1525.parallelTestResults.completedCount | |
| End use | |
| If ((Milliseconds:C459-$startWait)>$timeout) | |
| // Timeout - log error and break | |
| LOG EVENT:C667(Into system standard outputs:K38:9; "Parallel test execution timeout after 5 minutes\r\n"; Error message:K38:3) | |
| break | |
| End if | |
| DELAY PROCESS:C323(Current process:C322; 10) // Wait 10 ticks | |
| End while | |
| End if | |
| // Run sequential suites in main process | |
| If (This:C1470.sequentialSuites.length>0) | |
| If (This:C1470.outputFormat="human") | |
| LOG EVENT:C667(Into system standard outputs:K38:9; "Running "+String:C10(This:C1470.sequentialSuites.length)+" sequential test suite(s)\r\n"; Information message:K38:1) | |
| End if | |
| var $testSuite : cs:C1710._TestSuite | |
| For each ($testSuite; This:C1470.sequentialSuites) | |
| $testSuite.run() | |
| This:C1470._collectSuiteResults($testSuite) | |
| End for each | |
| End if | |
| // Cleanup worker processes | |
| For each ($workerName; This:C1470.workerProcesses) | |
| CALL WORKER:C1389($workerName; "ParallelTestWorker"; "StopWorker") | |
| End for each | |
| Function _waitForCompletionAndCollectResults() | |
| // Wait for parallel test suites to complete | |
| var $parallelSuiteCount : Integer | |
| var $totalSuites : Integer | |
| var $workerName : Text | |
| Use (Storage:C1525.parallelTestResults) | |
| $totalSuites:=Storage:C1525.parallelTestResults.totalSuites | |
| End use | |
| $parallelSuiteCount:=$totalSuites-This:C1470.sequentialSuites.length | |
| … (rest of method unchanged) … |
🤖 Prompt for AI Agents
In testing/Project/Sources/Classes/ParallelTestRunner.4dm around lines 209-261,
the cleanup loop uses $workerName without a local declaration which 4D requires;
add a local variable declaration (e.g. var $workerName : Text) near the other
var declarations at the start of _waitForCompletionAndCollectResults() so the
For each ($workerName; This:C1470.workerProcesses) loop compiles and runs
correctly.
…-worker-signals Refactor parallel runner waiting
1 participant
Summary by CodeRabbit