Add unit tests for parallel-executor module#252
Add unit tests for parallel-executor module#252nikolasdehor wants to merge 1 commit intoSynkraAI:mainfrom
Conversation
|
@nikolasdehor is attempting to deploy a commit to the Pedro Valério Lopez's projects Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughA comprehensive unit test suite for the ParallelExecutor module is introduced, covering constructor initialization, execution behavior, task tracking, status reporting, concurrency limits, cancellation, and summary aggregation across multiple test cases with mocked console output. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Suggested labels
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Pull request overview
Adds a new Jest unit test suite for the .aios-core/core/orchestration/parallel-executor module to validate task lifecycle/status tracking, summary reporting, cancellation, and logging behavior.
Changes:
- Introduces
ParallelExecutorunit tests coveringexecuteParallel,getStatus,hasRunningTasks,waitForCompletion,cancelAll,clear,setMaxConcurrency, andgetSummary. - Mocks
chalkto avoid color formatting in test output. - Adds assertions around status transitions, error capture, and summary statistics.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| jest.spyOn(console, 'log').mockImplementation(); | ||
| executor = new ParallelExecutor(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
console.log is spied/mocked in beforeEach but never restored. This can make debugging harder and diverges from the pattern used in other tests (e.g. restoring spies in afterEach). Add an afterEach that calls mockRestore() (or use jest.restoreAllMocks()) so the original console method is always put back.
| afterEach(() => { | |
| jest.restoreAllMocks(); | |
| }); |
| test('ignores cancelled tasks in counts', () => { | ||
| executor.runningTasks.set('a', { status: 'cancelled' }); | ||
|
|
||
| const summary = executor.getSummary(); | ||
| expect(summary.total).toBe(1); | ||
| expect(summary.completed).toBe(0); | ||
| expect(summary.failed).toBe(0); | ||
| expect(summary.running).toBe(0); | ||
| }); |
There was a problem hiding this comment.
The test name says "ignores cancelled tasks in counts", but the assertions still expect summary.total to include the cancelled task. Either update the test name to reflect the behavior (e.g. cancelled excluded from completed/failed/running only) or adjust expectations if cancelled should be excluded entirely.
|
|
||
| expect(executePhase).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This test only verifies that maxConcurrency can be passed, but it doesn't assert that the concurrency limit is actually enforced (i.e., that executePhase isn't running more than maxConcurrency tasks at once). Add a test that tracks the peak number of simultaneously-running executePhase calls (using a barrier/pending promise) and asserts it never exceeds the configured limit.
| test('does not exceed configured maxConcurrency', async () => { | |
| const maxConcurrency = 2; | |
| const phases = [ | |
| { phase: 'p1' }, | |
| { phase: 'p2' }, | |
| { phase: 'p3' }, | |
| { phase: 'p4' }, | |
| { phase: 'p5' }, | |
| ]; | |
| let currentRunning = 0; | |
| let peakRunning = 0; | |
| const releaseFns = []; | |
| const executePhase = jest.fn().mockImplementation(() => { | |
| currentRunning += 1; | |
| if (currentRunning > peakRunning) { | |
| peakRunning = currentRunning; | |
| } | |
| let resolveBarrier; | |
| const barrier = new Promise((resolve) => { | |
| resolveBarrier = resolve; | |
| }); | |
| // Store a function that, when called, will mark this task as finished. | |
| releaseFns.push(() => { | |
| currentRunning -= 1; | |
| resolveBarrier(); | |
| }); | |
| return barrier; | |
| }); | |
| const executionPromise = executor.executeParallel(phases, executePhase, { | |
| maxConcurrency, | |
| }); | |
| // Wait until at least maxConcurrency tasks have started. | |
| while (releaseFns.length < Math.min(maxConcurrency, phases.length)) { | |
| // Yield to the event loop to allow more tasks to start. | |
| // eslint-disable-next-line no-await-in-loop | |
| await new Promise((resolve) => setTimeout(resolve, 0)); | |
| } | |
| expect(peakRunning).toBeLessThanOrEqual(maxConcurrency); | |
| // Allow all running tasks to complete. | |
| releaseFns.forEach((release) => release()); | |
| const result = await executionPromise; | |
| expect(executePhase).toHaveBeenCalledTimes(phases.length); | |
| expect(result.results).toHaveLength(phases.length); | |
| expect(result.errors).toHaveLength(0); | |
| }); |
| test('throws on timeout', async () => { | ||
| executor.runningTasks.set('stuck', { status: 'running' }); | ||
|
|
||
| await expect(executor.waitForCompletion(50)).rejects.toThrow( | ||
| 'Timeout waiting for parallel tasks to complete' | ||
| ); | ||
| }); |
There was a problem hiding this comment.
The timeout test uses real timers, and waitForCompletion sleeps in 100ms increments, so this test will incur a real delay and can be slightly flaky under load. Consider using jest.useFakeTimers() / advanceTimersByTime() here (as done in other timeout tests in the repo) to make it fast and deterministic.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/core/orchestration/parallel-executor.test.js (3)
19-23: AddafterEach(() => jest.restoreAllMocks())to properly tear down theconsole.logspy.
jest.resetAllMocks()resets call counts and implementations but does not restore the originalconsole.log. Without an explicitrestoreAllMocks()call, the spy persists for the lifetime of the test file, which can cause unexpected side effects if test ordering changes or the module cache is shared.♻️ Proposed fix
beforeEach(() => { jest.resetAllMocks(); jest.spyOn(console, 'log').mockImplementation(); executor = new ParallelExecutor(); }); + + afterEach(() => { + jest.restoreAllMocks(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/parallel-executor.test.js` around lines 19 - 23, The test sets a console.log spy in the beforeEach (jest.spyOn(console, 'log') in the ParallelExecutor tests) but never restores it; add an afterEach that calls jest.restoreAllMocks() to properly tear down the console.log spy after each test (so add an afterEach(() => jest.restoreAllMocks()) alongside the existing beforeEach where executor = new ParallelExecutor()).
390-398: Misleading test name — cancelled tasks are still reflected intotal.The test name says "ignores cancelled tasks in counts", yet the assertion
expect(summary.total).toBe(1)confirms they are counted intotal. The description should clarify the actual intent: cancelled tasks are excluded from the per-status buckets but still contribute tototal.♻️ Suggested rename
- test('ignores cancelled tasks in counts', () => { + test('excludes cancelled tasks from per-status buckets but includes them in total', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/parallel-executor.test.js` around lines 390 - 398, Rename the test so the title accurately reflects that cancelled tasks are counted in summary.total but excluded from per-status buckets; update the test name for the case in the test that manipulates executor.runningTasks and calls executor.getSummary() (currently using a 'cancelled' status) to something like "counts cancelled tasks in total but ignores them in per-status counts" so the test intent aligns with the assertions on summary.total, summary.completed, summary.failed, and summary.running.
63-65: Mock-order assumption is fragile for parallel execution.
mockResolvedValueOnce/mockRejectedValueOnceassign behaviour by invocation index, implicitly relying on the executor callingexecutePhasefor'good'before'bad'. For a truly parallel executor this depends on the internal iteration order remaining stable. Consider using a named-argument match instead, so the correct outcome is tied to the phase regardless of dispatch order.♻️ More resilient alternative
- const executePhase = jest.fn() - .mockResolvedValueOnce({ ok: true }) - .mockRejectedValueOnce(new Error('Phase failed')); + const executePhase = jest.fn().mockImplementation((phase) => { + if (phase.phase === 'bad') return Promise.reject(new Error('Phase failed')); + return Promise.resolve({ ok: true }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/orchestration/parallel-executor.test.js` around lines 63 - 65, The test's use of mockResolvedValueOnce/mockRejectedValueOnce for executePhase is fragile because it depends on invocation order; replace it with a mockImplementation that inspects the phase argument (e.g., phase.id or phase.name used in the test) and returns Promise.resolve({ok:true}) for the 'good' phase and Promise.reject(new Error('Phase failed')) for the 'bad' phase so behavior is tied to the phase identity rather than call index; update the executePhase mock in parallel-executor.test.js to use this argument-based implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/core/orchestration/parallel-executor.test.js`:
- Around line 264-270: The timeout test is flaky because it relies on real
timers; change the test that calls executor.waitForCompletion(50) to use Jest
fake timers: call jest.useFakeTimers() before setting executor.runningTasks and
invoking executor.waitForCompletion, trigger the timeout by advancing timers
synchronously with jest.advanceTimersByTime(50) (or slightly beyond), assert the
promise rejects as before, and finally restore timers with jest.useRealTimers()
to avoid polluting other tests; locate this logic around the test named 'throws
on timeout' and the call to executor.waitForCompletion and
executor.runningTasks.
- Around line 87-94: The test is ineffective because a single phase cannot
exercise maxConcurrency; modify the test for executeParallel to create more
phases than the provided maxConcurrency (e.g., 10 phases with maxConcurrency: 5)
and replace the simple mock executePhase with a controlled async function that
tracks current concurrent invocations (increment a counter at start, await a
controllable delay, then decrement) so you can assert the peak concurrent
counter never exceeds the configured limit; use the executeParallel and
executePhase identifiers to locate and update the test and add assertions that
peakConcurrent <= 5 and executePhase was called for each phase.
---
Nitpick comments:
In `@tests/core/orchestration/parallel-executor.test.js`:
- Around line 19-23: The test sets a console.log spy in the beforeEach
(jest.spyOn(console, 'log') in the ParallelExecutor tests) but never restores
it; add an afterEach that calls jest.restoreAllMocks() to properly tear down the
console.log spy after each test (so add an afterEach(() =>
jest.restoreAllMocks()) alongside the existing beforeEach where executor = new
ParallelExecutor()).
- Around line 390-398: Rename the test so the title accurately reflects that
cancelled tasks are counted in summary.total but excluded from per-status
buckets; update the test name for the case in the test that manipulates
executor.runningTasks and calls executor.getSummary() (currently using a
'cancelled' status) to something like "counts cancelled tasks in total but
ignores them in per-status counts" so the test intent aligns with the assertions
on summary.total, summary.completed, summary.failed, and summary.running.
- Around line 63-65: The test's use of
mockResolvedValueOnce/mockRejectedValueOnce for executePhase is fragile because
it depends on invocation order; replace it with a mockImplementation that
inspects the phase argument (e.g., phase.id or phase.name used in the test) and
returns Promise.resolve({ok:true}) for the 'good' phase and Promise.reject(new
Error('Phase failed')) for the 'bad' phase so behavior is tied to the phase
identity rather than call index; update the executePhase mock in
parallel-executor.test.js to use this argument-based implementation.
| test('uses custom maxConcurrency from options', async () => { | ||
| const phases = [{ phase: 'a' }]; | ||
| const executePhase = jest.fn().mockResolvedValue({}); | ||
|
|
||
| await executor.executeParallel(phases, executePhase, { maxConcurrency: 5 }); | ||
|
|
||
| expect(executePhase).toHaveBeenCalledTimes(1); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Test doesn't actually verify custom concurrency is respected.
With a single-phase input, executePhase will always be called exactly once regardless of what maxConcurrency is set to. This test provides no signal about whether the option is wired up or enforced. To exercise the concurrency cap, you need more phases than the limit and a way to observe that at most N are running simultaneously.
♻️ More meaningful concurrency test
- test('uses custom maxConcurrency from options', async () => {
- const phases = [{ phase: 'a' }];
- const executePhase = jest.fn().mockResolvedValue({});
-
- await executor.executeParallel(phases, executePhase, { maxConcurrency: 5 });
-
- expect(executePhase).toHaveBeenCalledTimes(1);
- });
+ test('respects custom maxConcurrency — never exceeds the cap', async () => {
+ const concurrencyLimit = 2;
+ let currentlyRunning = 0;
+ let peakConcurrency = 0;
+ const phases = [{ phase: 'a' }, { phase: 'b' }, { phase: 'c' }, { phase: 'd' }];
+
+ const executePhase = jest.fn().mockImplementation(async () => {
+ currentlyRunning++;
+ peakConcurrency = Math.max(peakConcurrency, currentlyRunning);
+ await new Promise((r) => setImmediate(r));
+ currentlyRunning--;
+ return {};
+ });
+
+ await executor.executeParallel(phases, executePhase, { maxConcurrency: concurrencyLimit });
+
+ expect(executePhase).toHaveBeenCalledTimes(4);
+ expect(peakConcurrency).toBeLessThanOrEqual(concurrencyLimit);
+ });📝 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.
| test('uses custom maxConcurrency from options', async () => { | |
| const phases = [{ phase: 'a' }]; | |
| const executePhase = jest.fn().mockResolvedValue({}); | |
| await executor.executeParallel(phases, executePhase, { maxConcurrency: 5 }); | |
| expect(executePhase).toHaveBeenCalledTimes(1); | |
| }); | |
| test('respects custom maxConcurrency — never exceeds the cap', async () => { | |
| const concurrencyLimit = 2; | |
| let currentlyRunning = 0; | |
| let peakConcurrency = 0; | |
| const phases = [{ phase: 'a' }, { phase: 'b' }, { phase: 'c' }, { phase: 'd' }]; | |
| const executePhase = jest.fn().mockImplementation(async () => { | |
| currentlyRunning++; | |
| peakConcurrency = Math.max(peakConcurrency, currentlyRunning); | |
| await new Promise((r) => setImmediate(r)); | |
| currentlyRunning--; | |
| return {}; | |
| }); | |
| await executor.executeParallel(phases, executePhase, { maxConcurrency: concurrencyLimit }); | |
| expect(executePhase).toHaveBeenCalledTimes(4); | |
| expect(peakConcurrency).toBeLessThanOrEqual(concurrencyLimit); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/orchestration/parallel-executor.test.js` around lines 87 - 94, The
test is ineffective because a single phase cannot exercise maxConcurrency;
modify the test for executeParallel to create more phases than the provided
maxConcurrency (e.g., 10 phases with maxConcurrency: 5) and replace the simple
mock executePhase with a controlled async function that tracks current
concurrent invocations (increment a counter at start, await a controllable
delay, then decrement) so you can assert the peak concurrent counter never
exceeds the configured limit; use the executeParallel and executePhase
identifiers to locate and update the test and add assertions that peakConcurrent
<= 5 and executePhase was called for each phase.
| test('throws on timeout', async () => { | ||
| executor.runningTasks.set('stuck', { status: 'running' }); | ||
|
|
||
| await expect(executor.waitForCompletion(50)).rejects.toThrow( | ||
| 'Timeout waiting for parallel tasks to complete' | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Real-timer timeout test is CI-flaky — use fake timers.
A hard 50 ms wall-clock wait in CI is a classic source of intermittent failures: slow runners can exhaust the timeout before the promise chain is set up, or the OS scheduler introduces jitter that makes the timing non-deterministic. Use jest.useFakeTimers() to advance time synchronously.
♻️ Deterministic rewrite with fake timers
- test('throws on timeout', async () => {
- executor.runningTasks.set('stuck', { status: 'running' });
-
- await expect(executor.waitForCompletion(50)).rejects.toThrow(
- 'Timeout waiting for parallel tasks to complete'
- );
- });
+ test('throws on timeout', async () => {
+ jest.useFakeTimers();
+ executor.runningTasks.set('stuck', { status: 'running' });
+
+ const promise = executor.waitForCompletion(50);
+ jest.advanceTimersByTime(51);
+
+ await expect(promise).rejects.toThrow(
+ 'Timeout waiting for parallel tasks to complete'
+ );
+ jest.useRealTimers();
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/core/orchestration/parallel-executor.test.js` around lines 264 - 270,
The timeout test is flaky because it relies on real timers; change the test that
calls executor.waitForCompletion(50) to use Jest fake timers: call
jest.useFakeTimers() before setting executor.runningTasks and invoking
executor.waitForCompletion, trigger the timeout by advancing timers
synchronously with jest.advanceTimersByTime(50) (or slightly beyond), assert the
promise rejects as before, and finally restore timers with jest.useRealTimers()
to avoid polluting other tests; locate this logic around the test named 'throws
on timeout' and the call to executor.waitForCompletion and
executor.runningTasks.
|
Consolidated into #426 |
Summary
parallel-executororchestration moduleTest Coverage
Test plan
Summary by CodeRabbit
Closes #307