test: isolate mock cluster ports and logs#5923
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCluster port selection now uses deterministic helper functions (configurable via new ChangesCluster & Mock Port and Env Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
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)
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 |
Deploying egg with
|
| Latest commit: |
4c92451
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b6e4f4df.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-9def10d5.egg-cci.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5923 +/- ##
==========================================
+ Coverage 85.09% 85.22% +0.13%
==========================================
Files 667 668 +1
Lines 19185 19310 +125
Branches 3743 3791 +48
==========================================
+ Hits 16325 16457 +132
+ Misses 2467 2462 -5
+ Partials 393 391 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable clusterPort for the egg cluster and improves port detection logic to avoid conflicts by incorporating process.pid and threadId. It also enhances environment variable handling by supporting NODE_ENV as a fallback for EGG_SERVER_ENV and ensuring EGG_HOME is correctly set. Feedback was provided to improve type safety in the ClusterApplication constructor by replacing an any type cast with a more specific record type.
| */ | ||
| constructor(options: MockClusterApplicationOptions) { | ||
| const opt = options.opt; | ||
| const opt: any = options.opt ?? {}; |
There was a problem hiding this comment.
Using any here reduces type safety. Since you are subsequently adding an env property to this object, it would be better to use a more descriptive type such as Record<string, any> to maintain better type safety while allowing dynamic property assignment.
| const opt: any = options.opt ?? {}; | |
| const opt = (options.opt ?? {}) as Record<string, any>; |
Deploying egg-v3 with
|
| Latest commit: |
4c92451
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a27d9b89.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-9def10d5.egg-v3.pages.dev |
There was a problem hiding this comment.
Pull request overview
This PR aims to make Egg's test helpers less flaky under parallel Vitest runs by isolating mock home/log paths and spreading the default ports used by @eggjs/mock and @eggjs/cluster.
Changes:
- Make
formatOptions()derive mock home paths fromNODE_ENV/EGG_SERVER_ENVand setEGG_HOMEalongsideHOME. - Add
clusterPortto the mock/cluster option types and forwardEGG_HOMEinto cluster child processes. - Seed cluster client and sticky-worker port detection from deterministic process-based ranges to reduce parallel port collisions.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
plugins/mock/test/format_options.test.ts |
Adds coverage for NODE_ENV=test home-path mocking behavior. |
plugins/mock/src/lib/types.ts |
Extends mock option types with clusterPort. |
plugins/mock/src/lib/format_options.ts |
Changes env resolution and starts mocking EGG_HOME in addition to HOME. |
plugins/mock/src/lib/cluster.ts |
Changes default mock cluster port allocation and injects EGG_HOME into child env. |
packages/cluster/src/utils/options.ts |
Documents the new clusterPort option on cluster startup options. |
packages/cluster/src/master.ts |
Seeds cluster-client/sticky port detection from deterministic per-process ranges. |
| if (!isMocked(process.env, 'HOME') && (env === 'default' || env === 'test' || env === 'prod')) { | ||
| mm(process.env, 'HOME', options.baseDir); | ||
| } | ||
| if (!isMocked(process.env, 'EGG_HOME') && (env === 'default' || env === 'test' || env === 'prod')) { |
| const clusterPort = await detectPort( | ||
| this.options.clusterPort ?? CLUSTER_CLIENT_PORT_START + (process.pid % 30_000), | ||
| ); | ||
| this.options.clusterPort = clusterPort; | ||
| this.log('[master] detected cluster port: %s', clusterPort); | ||
| // If sticky mode, detect worker port | ||
| if (this.options.sticky) { | ||
| const stickyWorkerPort = await detectPort(); | ||
| const stickyWorkerPort = await detectPort(clusterPort + 1); |
| var eggMockMasterPort: number; | ||
| } | ||
| globalThis.eggMockMasterPort = 17000 + (process.pid % 1000); | ||
| globalThis.eggMockMasterPort = 17000 + (process.pid % 20) * 1000 + (threadId % 10) * 100; |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/mock/src/lib/cluster.ts`:
- Around line 82-87: The code in cluster.ts force-sets EGG_HOME on every child
by falling back to options.baseDir (const opt.env = ... EGG_HOME:
opt.env?.EGG_HOME ?? process.env.EGG_HOME ?? options.baseDir), which bypasses
the env gate in format_options.ts; change the merge so EGG_HOME is only
preserved if explicitly provided by opt.env or process.env (i.e., remove the
fallback to options.baseDir), so: keep the spread merge but set EGG_HOME to
opt.env?.EGG_HOME ?? process.env.EGG_HOME (no default), ensuring cluster.ts
(opt.env block / EGG_HOME assignment) no longer overrides home unless explicitly
set and thus respects the env gating logic in format_options.ts.
In `@plugins/mock/src/lib/format_options.ts`:
- Around line 80-85: The fallback logic uses const env =
process.env.EGG_SERVER_ENV ?? process.env.NODE_ENV and then checks (env ===
'default' || env === 'test' || env === 'prod') before mocking HOME/EGG_HOME, but
NODE_ENV commonly equals 'production' so those branches won't run; update the
condition in format_options.ts (around the env checks and the isMocked/MM calls
for 'HOME' and 'EGG_HOME') to also accept 'production' (or normalize env
'production' to 'prod') so that when NODE_ENV='production' the mm calls will run
and HOME/EGG_HOME are isolated.
🪄 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: 4815ef01-0ce5-42f3-9794-4f6d258d480c
📒 Files selected for processing (6)
packages/cluster/src/master.tspackages/cluster/src/utils/options.tsplugins/mock/src/lib/cluster.tsplugins/mock/src/lib/format_options.tsplugins/mock/src/lib/types.tsplugins/mock/test/format_options.test.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugins/mock/test/format_options.test.ts (1)
145-161:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing
EGG_HOMEassertions for theEGG_SERVER_ENV=default/test/prodsub-cases.
format_options.tsnow mocksEGG_HOMEunder the sameshouldMockHomegate asHOME, but the pre-existing sub-cases only assertHOME. The firstformatOptions()call (line 152) will mockEGG_HOMEtobaseDir; subsequent sub-cases (lines 155–161) will seeisMockedreturningtruebut the value stays correct. Adding the assertions would close the coverage gap introduced by the newEGG_HOMElogic.✅ Proposed assertions to add
mm(process.env, 'EGG_SERVER_ENV', 'default'); formatOptions(); assert.equal(process.env.HOME, baseDir); + assert.equal(process.env.EGG_HOME, baseDir); mm(process.env, 'EGG_SERVER_ENV', 'test'); formatOptions(); assert.equal(process.env.HOME, baseDir); + assert.equal(process.env.EGG_HOME, baseDir); mm(process.env, 'EGG_SERVER_ENV', 'prod'); formatOptions(); assert.equal(process.env.HOME, baseDir); + assert.equal(process.env.EGG_HOME, baseDir);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/mock/test/format_options.test.ts` around lines 145 - 161, The test is missing assertions that EGG_HOME is mocked alongside HOME when formatOptions() runs; after each formatOptions() call in the sub-cases where you set process.env.EGG_SERVER_ENV to 'default', 'test', and 'prod' (the mm calls and subsequent formatOptions() invocations in the spec), add assertions that process.env.EGG_HOME equals baseDir (similar to the existing HOME assertions) so each sub-case verifies EGG_HOME was set by formatOptions().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@plugins/mock/test/format_options.test.ts`:
- Around line 145-161: The test is missing assertions that EGG_HOME is mocked
alongside HOME when formatOptions() runs; after each formatOptions() call in the
sub-cases where you set process.env.EGG_SERVER_ENV to 'default', 'test', and
'prod' (the mm calls and subsequent formatOptions() invocations in the spec),
add assertions that process.env.EGG_HOME equals baseDir (similar to the existing
HOME assertions) so each sub-case verifies EGG_HOME was set by formatOptions().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 064b72e0-e3c9-41e5-8e2d-4c40129e3757
📒 Files selected for processing (5)
packages/cluster/src/master.tspackages/cluster/test/master/detect_ports.test.tsplugins/mock/src/lib/cluster.tsplugins/mock/src/lib/format_options.tsplugins/mock/test/format_options.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cluster/test/master/detect_ports.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/cluster/src/master.ts
| const env = process.env.EGG_SERVER_ENV ?? process.env.NODE_ENV; | ||
| const nodeEnv = process.env.NODE_ENV; | ||
| const shouldMockHome = | ||
| env === 'default' || env === 'test' || env === 'prod' || env === 'production' || nodeEnv === 'test'; |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/cluster/test/master/detect_ports.test.ts (1)
16-54: ⚡ Quick winConsider adding a non-sticky
detectPorts()path test.The current suite covers only
sticky: true. Theif (this.options.sticky)branch indetectPorts()means that when sticky isfalse,detectPortshould only be called once andstickyWorkerPortshould remainundefined. Adding this case would complete branch coverage for the method under test.🧪 Suggested additional test
+ it('should only detect cluster port when not in sticky mode', async () => { + const master = Object.create(Master.prototype) as Master; + master.options = { + clusterPort: 34567, + sticky: false, + } as any; + master.logger = { error: vi.fn() } as any; + master.log = vi.fn(); + + await master.detectPorts(); + + assert.equal(detectPortMock.mock.calls.length, 1); + assert.equal(master.options.clusterPort, 34567); + assert.equal(master.options.stickyWorkerPort, undefined); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cluster/test/master/detect_ports.test.ts` around lines 16 - 54, Add a test that exercises the non-sticky branch of Master.detectPorts(): instantiate a Master-like object (Object.create(Master.prototype)), set master.options = { clusterPort: 34567, sticky: false } and a stubbed logger, mock detectPort (detectPortMock) to a known behavior, call await master.detectPorts(), then assert detectPort was called only once with the resolved cluster port and that master.options.stickyWorkerPort remains undefined while master.options.clusterPort is set from the single detectPort result; this covers the if (this.options.sticky) false path in detectPorts().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/cluster/test/master/detect_ports.test.ts`:
- Around line 16-54: Add a test that exercises the non-sticky branch of
Master.detectPorts(): instantiate a Master-like object
(Object.create(Master.prototype)), set master.options = { clusterPort: 34567,
sticky: false } and a stubbed logger, mock detectPort (detectPortMock) to a
known behavior, call await master.detectPorts(), then assert detectPort was
called only once with the resolved cluster port and that
master.options.stickyWorkerPort remains undefined while
master.options.clusterPort is set from the single detectPort result; this covers
the if (this.options.sticky) false path in detectPorts().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2b2828bf-8906-4d4a-b662-0e662dfd6d05
📒 Files selected for processing (6)
packages/cluster/README.mdpackages/cluster/test/master/detect_ports.test.tsplugins/mock/src/lib/cluster.tsplugins/mock/src/lib/format_options.tsplugins/mock/test/cluster_constructor.test.tsplugins/mock/test/format_options.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/cluster/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- plugins/mock/test/format_options.test.ts
- plugins/mock/src/lib/format_options.ts
| export function getClusterPortStart(clusterPort?: number): number { | ||
| return clusterPort ?? CLUSTER_CLIENT_PORT_START + (process.pid % 30_000); |
| opt.env = { | ||
| ...process.env, | ||
| ...opt.env, | ||
| }; |
| if (!isMocked(process.env, 'HOME') && shouldMockHome) { | ||
| mm(process.env, 'HOME', options.baseDir); | ||
| } | ||
| if (!isMocked(process.env, 'EGG_HOME') && shouldMockHome) { |
killagu
force-pushed
the
agent/egg-dev/9def10d5
branch
from
6945e8a to
f28d41a
Compare
| if (opt.env === undefined) { | ||
| opt.env = { ...process.env }; | ||
| } |
killagu
force-pushed
the
agent/egg-dev/9def10d5
branch
from
f28d41a to
4c92451
Compare
Summary
Tests
Summary by CodeRabbit
New Features
Bug Fixes
Tests