feat(egg-bin): support vitest threads pool mode#5816
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughAdds a Vitest pool/isolate flag and propagation via environment, introduces optional Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as "egg-bin CLI"
participant Env as "Process Env"
participant Vitest as "Vitest Runner"
participant Mock as "plugins/mock.formatOptions"
participant Worker as "Worker/Process"
CLI->>Env: set EGG_VITEST_POOL = flags.pool
CLI->>Vitest: start tests (pool = flags.pool, isolate flag)
Vitest->>Worker: spawn workers according to pool
Worker->>Mock: call formatOptions(options)
Mock->>Env: read EGG_VITEST_POOL
alt EGG_VITEST_POOL == "threads" and options.startMode not set
Mock->>Mock: set options.startMode = "worker_threads"
end
Mock->>Worker: return formatted options (including startMode)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: |
5972971
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://94d4d285.egg-cci.pages.dev |
| Branch Preview URL: | https://feat-vitest-threads-pool.egg-cci.pages.dev |
Add --pool flag to egg-bin test command (default: threads) and propagate via EGG_VITEST_POOL env var so @eggjs/mock can auto-switch cluster-client to worker_threads IPC when running in threads pool. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
killagu
force-pushed
the
feat/vitest-threads-pool
branch
from
a500301 to
aa46b5a
Compare
Deploying egg-v3 with
|
| Latest commit: |
5972971
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b3d5cc8c.egg-v3.pages.dev |
| Branch Preview URL: | https://feat-vitest-threads-pool.egg-v3.pages.dev |
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5816 +/- ##
==========================================
- Coverage 85.23% 85.19% -0.04%
==========================================
Files 650 650
Lines 12522 12529 +7
Branches 1434 1436 +2
==========================================
+ Hits 10673 10674 +1
- Misses 1729 1735 +6
Partials 120 120 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/mock/src/lib/types.ts (1)
70-70: Redundant property declaration.
MockClusterOptionsextendsMockOptions, sostartModeis already inherited. This explicit redeclaration is unnecessary unless you intend to override the JSDoc or narrow the type.♻️ Suggested removal
export interface MockClusterOptions extends MockOptions { workers?: number | string; cache?: boolean; port?: number; /** * opt pass to coffee, such as { execArgv: ['--debug'] } */ opt?: object; - startMode?: 'process' | 'worker_threads'; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/mock/src/lib/types.ts` at line 70, MockClusterOptions currently redeclares startMode which is already inherited from MockOptions; remove the redundant startMode?: 'process' | 'worker_threads' declaration from the MockClusterOptions interface (or only keep it if you intentionally need to narrow/override the type or JSDoc), leaving MockOptions as the single source of truth for that property.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@plugins/mock/src/lib/types.ts`:
- Line 70: MockClusterOptions currently redeclares startMode which is already
inherited from MockOptions; remove the redundant startMode?: 'process' |
'worker_threads' declaration from the MockClusterOptions interface (or only keep
it if you intentionally need to narrow/override the type or JSDoc), leaving
MockOptions as the single source of truth for that property.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
plugins/mock/src/lib/format_options.tsplugins/mock/src/lib/types.tstools/egg-bin/package.jsontools/egg-bin/src/commands/test.tstools/egg-bin/test/commands/test.test.ts
There was a problem hiding this comment.
Pull request overview
Adds support for running egg-bin test with Vitest pool selection (forks vs threads) and wires the chosen pool mode through to @eggjs/mock so cluster-related fixtures can run under thread-based execution.
Changes:
- Add
--pool (forks|threads)flag toegg-bin test, defaulting tothreads, and pass it into Vitest config. - Propagate pool mode via
EGG_VITEST_POOLand use it in@eggjs/mockto defaultstartModetoworker_threadsunder threads pool. - Add a new egg-bin test intended to cover
--pool threadswith a cluster-client fixture.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/egg-bin/test/commands/test.test.ts | Adds a new CLI test for running Vitest in threads pool with a cluster-client fixture. |
| tools/egg-bin/src/commands/test.ts | Introduces --pool flag, sets EGG_VITEST_POOL, and configures Vitest pool accordingly. |
| tools/egg-bin/package.json | Adds sdk-base to devDependencies (currently appears unused within egg-bin/fixtures). |
| pnpm-lock.yaml | Updates lockfile, including a new importer entry for tegg/core/bundler. |
| plugins/mock/src/lib/types.ts | Adds startMode to MockOptions type. |
| plugins/mock/src/lib/format_options.ts | Auto-sets startMode to worker_threads when EGG_VITEST_POOL=threads. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| }); | ||
|
|
||
| it('should success on ts with --pool threads and cluster-client', async () => { | ||
| const cwd = getFixtures('example-ts-cluster-client'); |
There was a problem hiding this comment.
getFixtures('example-ts-cluster-client') points to a fixture directory that doesn't exist in tools/egg-bin/test/fixtures/, so this new test will fail at runtime. Add the missing example-ts-cluster-client fixture (with the minimal deps/files needed to reproduce the cluster-client scenario) or update the test to use an existing fixture.
| const cwd = getFixtures('example-ts-cluster-client'); | |
| const cwd = getFixtures('example-ts-test'); |
pnpm-lock.yaml
Outdated
| tegg/core/bundler: | ||
| dependencies: | ||
| '@eggjs/controller-decorator': | ||
| specifier: workspace:* | ||
| version: link:../controller-decorator | ||
| '@eggjs/core-decorator': | ||
| specifier: workspace:* | ||
| version: link:../core-decorator | ||
| '@eggjs/metadata': | ||
| specifier: workspace:* | ||
| version: link:../metadata | ||
| '@eggjs/tegg-common-util': | ||
| specifier: workspace:* | ||
| version: link:../common-util | ||
| '@eggjs/tegg-loader': | ||
| specifier: workspace:* | ||
| version: link:../loader | ||
| '@eggjs/tegg-types': | ||
| specifier: workspace:* | ||
| version: link:../types | ||
| typescript: | ||
| specifier: 'catalog:' | ||
| version: 5.9.3 | ||
| devDependencies: | ||
| '@types/node': | ||
| specifier: 'catalog:' | ||
| version: 24.10.2 | ||
| vitest: | ||
| specifier: 'catalog:' | ||
| version: 4.0.15(@types/node@24.10.2)(@vitest/ui@4.0.15)(esbuild@0.27.0)(jiti@2.6.1)(less@4.4.2)(sass@1.93.2)(terser@5.44.0)(tsx@4.20.6)(yaml@2.8.2) | ||
|
|
There was a problem hiding this comment.
The lockfile adds an importer entry for tegg/core/bundler, but there is no corresponding workspace directory/package in the repository. This will leave the lockfile inconsistent with the workspace and may break/complicate pnpm install. Please remove this stray importer change or add the missing package directory if it was intended.
| tegg/core/bundler: | |
| dependencies: | |
| '@eggjs/controller-decorator': | |
| specifier: workspace:* | |
| version: link:../controller-decorator | |
| '@eggjs/core-decorator': | |
| specifier: workspace:* | |
| version: link:../core-decorator | |
| '@eggjs/metadata': | |
| specifier: workspace:* | |
| version: link:../metadata | |
| '@eggjs/tegg-common-util': | |
| specifier: workspace:* | |
| version: link:../common-util | |
| '@eggjs/tegg-loader': | |
| specifier: workspace:* | |
| version: link:../loader | |
| '@eggjs/tegg-types': | |
| specifier: workspace:* | |
| version: link:../types | |
| typescript: | |
| specifier: 'catalog:' | |
| version: 5.9.3 | |
| devDependencies: | |
| '@types/node': | |
| specifier: 'catalog:' | |
| version: 24.10.2 | |
| vitest: | |
| specifier: 'catalog:' | |
| version: 4.0.15(@types/node@24.10.2)(@vitest/ui@4.0.15)(esbuild@0.27.0)(jiti@2.6.1)(less@4.4.2)(sass@1.93.2)(terser@5.44.0)(tsx@4.20.6)(yaml@2.8.2) |
| "npminstall": "catalog:", | ||
| "rimraf": "catalog:", | ||
| "runscript": "catalog:", | ||
| "sdk-base": "catalog:", |
There was a problem hiding this comment.
sdk-base is added as a devDependency here, but there are no references to it in tools/egg-bin source or fixtures. If it’s not required for the new pool/cluster-client behavior, consider removing it to avoid unused dependency drift (or add a brief note in the PR description explaining why egg-bin needs it).
| "sdk-base": "catalog:", |
| /** | ||
| * Start mode for egg cluster-client | ||
| */ | ||
| startMode?: 'process' | 'worker_threads'; | ||
| } |
There was a problem hiding this comment.
startMode is added to MockOptions, but MockClusterOptions (which extends MockOptions) already declares startMode further down in this file. This duplication can get out of sync over time; prefer defining it only once (e.g., on MockOptions) and removing the redundant declaration from MockClusterOptions.
![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 support for vitest's threads pool mode in egg-bin. It adds a --pool flag to the test command, defaulting to threads, which is a change from the previous hardcoded 'forks'. The chosen pool mode is propagated via an EGG_VITEST_POOL environment variable to @eggjs/mock, which then correctly configures its startMode for cluster clients. The changes are well-implemented and include a new test case for verification. The code is clean and the logic is sound. I have not found any issues of medium or higher severity.
Add the example-ts-cluster-client fixture required by the threads pool test, and regenerate pnpm-lock.yaml without unrelated tegg/core/bundler entries that caused dedupe check failures in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (6)
tools/egg-bin/test/fixtures/example-ts-cluster-client/package.json (1)
4-6: Useworkspace:*for internal monorepo dependencies.The
@eggjs/mockpackage is an internal monorepo dependency. Per coding guidelines, internal dependencies should use theworkspace:*protocol.Suggested fix
"devDependencies": { - "@eggjs/mock": "*" + "@eggjs/mock": "workspace:*" },As per coding guidelines: "Use
workspace:*for internal package dependencies in package.json".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bin/test/fixtures/example-ts-cluster-client/package.json` around lines 4 - 6, The package.json devDependencies lists an internal monorepo package "@eggjs/mock" with a non-workspace specifier; update the dependency to use the workspace protocol by changing the version value for "@eggjs/mock" to "workspace:*" so the project uses the monorepo workspace copy (locate the "devDependencies" block and the "@eggjs/mock" entry).tools/egg-bin/test/fixtures/example-ts-cluster-client/test/index.test.ts (2)
7-9: Consider adding assertion on response body.The test verifies
/getHostsreturns 200, but doesn't assert on the response body to confirm the published value was propagated. This would strengthen the test's validation of the publish/subscribe flow.Suggested enhancement
// wait for async subscribe propagation await new Promise((resolve) => setTimeout(resolve, 500)); - await app.httpRequest().get('/getHosts').expect(200); + const res = await app.httpRequest().get('/getHosts').expect(200); + // Optionally assert the propagated value + // assert(res.body.includes('www.testme.com')); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bin/test/fixtures/example-ts-cluster-client/test/index.test.ts` around lines 7 - 9, The test currently only checks that GET '/getHosts' returns 200 but doesn't validate the response payload; update the test in index.test.ts to assert the response body contains the expected published hosts value by reading the response from await app.httpRequest().get('/getHosts') (instead of only .expect(200)) and adding assertions against the JSON/body fields (e.g., expected host array or specific host entry) after the existing await new Promise(...) wait to confirm the subscribe propagation; use the same request call (app.httpRequest().get('/getHosts')) and assert on res.body or res.text to validate the published value was propagated.
1-3: Add explicit imports from vitest.Per coding guidelines, test functions should be explicitly imported from vitest rather than relying on globals.
Suggested fix
import { app } from '@eggjs/mock/bootstrap'; +import { describe, it } from 'vitest'; describe('cluster-client test', () => {As per coding guidelines: "Import test functions from vitest:
import { describe, it } from 'vitest'".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bin/test/fixtures/example-ts-cluster-client/test/index.test.ts` around lines 1 - 3, The test file relies on global test helpers; add explicit vitest imports by importing the needed functions (e.g., describe, it, beforeEach, afterEach as required) from 'vitest' at the top of the file so globals aren't relied on—specifically update the top of the file that currently imports app from '@eggjs/mock/bootstrap' to also import the test functions (for example import { describe, it } from 'vitest') and adjust any other vitest globals used to be imported explicitly.tools/egg-bin/test/fixtures/example-ts-cluster-client/tsconfig.json (1)
8-10: Consider enabling stricter TypeScript settings.The fixture disables
noImplicitAnyandstrictmode. While test fixtures may warrant relaxed settings for simplicity, the coding guidelines recommend enabling strict mode in all TypeScript packages.If the extensive use of
anytypes in this fixture is intentional for testing purposes, this is acceptable. Otherwise, consider tightening these settings and adding proper type definitions.As per coding guidelines: "Enable strict mode in all TypeScript packages".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bin/test/fixtures/example-ts-cluster-client/tsconfig.json` around lines 8 - 10, Enable TypeScript strictness in this fixture by turning on "strict" and "noImplicitAny" in tsconfig.json (set "strict": true and "noImplicitAny": true) to comply with the coding guidelines; if test-specific looseness is required, document why and only keep "skipLibCheck": true if necessary, otherwise remove or set it to false, and add proper type definitions to the fixture code instead of using implicit any types.tools/egg-bin/test/fixtures/example-ts-cluster-client/app/controller/home.ts (2)
10-10: Prefer typed access overanycasts.Using
as anybypasses type checking. For test fixtures, this is acceptable but could be improved by defining an interface for the extended app properties. This would provide better IDE support and catch typos at compile time.📝 Proposed approach
Define an interface for the extended app:
import { Controller, Application } from 'egg'; interface ExtendedApp extends Application { registryClient: { publish(options: { dataId: string; publishData: unknown }): void; }; val?: unknown; } export default class HomeController extends Controller { async index(): Promise<void> { this.ctx.body = 'hi cluster'; } async publish(): Promise<void> { const val = this.ctx.request.body.value; (this.ctx.app as ExtendedApp).registryClient.publish({ dataId: 'demo.DemoService', publishData: val, }); this.ctx.body = 'ok'; } async getHosts(): Promise<void> { const val = (this.ctx.app as ExtendedApp).val; this.ctx.body = val ? JSON.stringify(val) : ''; } }As per coding guidelines: "Avoid
anytype; useunknownwhen type is truly unknown in TypeScript".Also applies to: 18-18
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bin/test/fixtures/example-ts-cluster-client/app/controller/home.ts` at line 10, Replace the unsafe (this.ctx.app as any) casts in HomeController with a proper typed interface: declare an ExtendedApp interface that extends egg's Application and includes registryClient.publish(...) (accepting { dataId: string; publishData: unknown }) and optional val?: unknown, import Application, then update the publish and getHosts methods to cast this.ctx.app to ExtendedApp (or better, type this.ctx.app as ExtendedApp) and access registryClient and val via that interface instead of any.
4-20: Consider adding explicit return types to public methods.As per coding guidelines, all exported methods should have explicit return type annotations. Since these are async handlers that set
ctx.bodywithout returning a value, the return type should bePromise<void>.📝 Proposed fix
export default class HomeController extends Controller { - async index() { + async index(): Promise<void> { this.ctx.body = 'hi cluster'; } - async publish() { + async publish(): Promise<void> { const val = this.ctx.request.body.value; (this.ctx.app as any).registryClient.publish({ dataId: 'demo.DemoService', publishData: val, }); this.ctx.body = 'ok'; } - async getHosts() { + async getHosts(): Promise<void> { const val = (this.ctx.app as any).val; this.ctx.body = val ? JSON.stringify(val) : ''; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/egg-bin/test/fixtures/example-ts-cluster-client/app/controller/home.ts` around lines 4 - 20, The async controller methods index, publish, and getHosts currently lack explicit return types; update their signatures to declare Promise<void> as the return type (e.g., async index(): Promise<void> { ... }) so each public handler explicitly returns Promise<void> while still setting this.ctx.body and not returning a value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@tools/egg-bin/test/fixtures/example-ts-cluster-client/app/controller/home.ts`:
- Line 10: Replace the unsafe (this.ctx.app as any) casts in HomeController with
a proper typed interface: declare an ExtendedApp interface that extends egg's
Application and includes registryClient.publish(...) (accepting { dataId:
string; publishData: unknown }) and optional val?: unknown, import Application,
then update the publish and getHosts methods to cast this.ctx.app to ExtendedApp
(or better, type this.ctx.app as ExtendedApp) and access registryClient and val
via that interface instead of any.
- Around line 4-20: The async controller methods index, publish, and getHosts
currently lack explicit return types; update their signatures to declare
Promise<void> as the return type (e.g., async index(): Promise<void> { ... }) so
each public handler explicitly returns Promise<void> while still setting
this.ctx.body and not returning a value.
In `@tools/egg-bin/test/fixtures/example-ts-cluster-client/package.json`:
- Around line 4-6: The package.json devDependencies lists an internal monorepo
package "@eggjs/mock" with a non-workspace specifier; update the dependency to
use the workspace protocol by changing the version value for "@eggjs/mock" to
"workspace:*" so the project uses the monorepo workspace copy (locate the
"devDependencies" block and the "@eggjs/mock" entry).
In `@tools/egg-bin/test/fixtures/example-ts-cluster-client/test/index.test.ts`:
- Around line 7-9: The test currently only checks that GET '/getHosts' returns
200 but doesn't validate the response payload; update the test in index.test.ts
to assert the response body contains the expected published hosts value by
reading the response from await app.httpRequest().get('/getHosts') (instead of
only .expect(200)) and adding assertions against the JSON/body fields (e.g.,
expected host array or specific host entry) after the existing await new
Promise(...) wait to confirm the subscribe propagation; use the same request
call (app.httpRequest().get('/getHosts')) and assert on res.body or res.text to
validate the published value was propagated.
- Around line 1-3: The test file relies on global test helpers; add explicit
vitest imports by importing the needed functions (e.g., describe, it,
beforeEach, afterEach as required) from 'vitest' at the top of the file so
globals aren't relied on—specifically update the top of the file that currently
imports app from '@eggjs/mock/bootstrap' to also import the test functions (for
example import { describe, it } from 'vitest') and adjust any other vitest
globals used to be imported explicitly.
In `@tools/egg-bin/test/fixtures/example-ts-cluster-client/tsconfig.json`:
- Around line 8-10: Enable TypeScript strictness in this fixture by turning on
"strict" and "noImplicitAny" in tsconfig.json (set "strict": true and
"noImplicitAny": true) to comply with the coding guidelines; if test-specific
looseness is required, document why and only keep "skipLibCheck": true if
necessary, otherwise remove or set it to false, and add proper type definitions
to the fixture code instead of using implicit any types.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
tools/egg-bin/test/fixtures/example-ts-cluster-client/agent.tstools/egg-bin/test/fixtures/example-ts-cluster-client/app.tstools/egg-bin/test/fixtures/example-ts-cluster-client/app/controller/home.tstools/egg-bin/test/fixtures/example-ts-cluster-client/app/router.tstools/egg-bin/test/fixtures/example-ts-cluster-client/config/config.default.tstools/egg-bin/test/fixtures/example-ts-cluster-client/lib/registry_client.tstools/egg-bin/test/fixtures/example-ts-cluster-client/package.jsontools/egg-bin/test/fixtures/example-ts-cluster-client/test/index.test.tstools/egg-bin/test/fixtures/example-ts-cluster-client/tsconfig.json
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
||
| describe('cluster-client test', () => { | ||
| it('should publish & subscribe via cluster-client', async () => { | ||
| await app.ready(); | ||
| await app.httpRequest().post('/publish').send({ value: 'www.testme.com' }).expect('ok').expect(200); | ||
| // wait for async subscribe propagation | ||
| await new Promise((resolve) => setTimeout(resolve, 500)); | ||
| await app.httpRequest().get('/getHosts').expect(200); |
There was a problem hiding this comment.
The test doesn’t actually verify that the publish/subscribe path worked: it only asserts HTTP 200 for /getHosts, and the fixed setTimeout(500) makes the test potentially flaky. Consider asserting the response body contains the published value (e.g. www.testme.com) and replacing the fixed sleep with a small polling loop (retry /getHosts until it returns the expected content or a timeout).
| describe('cluster-client test', () => { | |
| it('should publish & subscribe via cluster-client', async () => { | |
| await app.ready(); | |
| await app.httpRequest().post('/publish').send({ value: 'www.testme.com' }).expect('ok').expect(200); | |
| // wait for async subscribe propagation | |
| await new Promise((resolve) => setTimeout(resolve, 500)); | |
| await app.httpRequest().get('/getHosts').expect(200); | |
| import { strict as assert } from 'node:assert'; | |
| describe('cluster-client test', () => { | |
| it('should publish & subscribe via cluster-client', async () => { | |
| await app.ready(); | |
| await app.httpRequest().post('/publish').send({ value: 'www.testme.com' }).expect('ok').expect(200); | |
| // wait for async subscribe propagation by polling /getHosts | |
| const expectedValue = 'www.testme.com'; | |
| const timeoutMs = 5000; | |
| const intervalMs = 200; | |
| const start = Date.now(); | |
| let lastBody: string | undefined; | |
| // eslint-disable-next-line no-constant-condition | |
| while (true) { | |
| const res = await app.httpRequest().get('/getHosts'); | |
| // Ensure the request itself succeeds | |
| assert.strictEqual(res.status, 200, `Expected /getHosts to return 200, got ${res.status}`); | |
| const bodyText = | |
| typeof res.text === 'string' | |
| ? res.text | |
| : res.body != null | |
| ? JSON.stringify(res.body) | |
| : ''; | |
| lastBody = bodyText; | |
| if (bodyText.includes(expectedValue)) { | |
| break; | |
| } | |
| if (Date.now() - start > timeoutMs) { | |
| assert.fail( | |
| `Timed out waiting for /getHosts to contain "${expectedValue}". Last response body: ${lastBody}`, | |
| ); | |
| } | |
| await new Promise(resolve => setTimeout(resolve, intervalMs)); | |
| } |
| /** | ||
| * Start mode for egg cluster-client | ||
| */ | ||
| startMode?: 'process' | 'worker_threads'; | ||
| } |
There was a problem hiding this comment.
The docstring for startMode says it is for “egg cluster-client”, but in this repo startMode is used as an Egg cluster start option (process vs worker_threads) in packages/cluster (e.g. master/app_worker/agent_worker). Please rename/clarify the comment so it reflects the actual behavior (cluster start mode), and consider removing the duplicate startMode field from MockClusterOptions since it now inherits it from MockOptions.
| // when running under vitest threads pool, use worker_threads start mode | ||
| // so egg cluster-client uses thread-based IPC instead of process-based |
There was a problem hiding this comment.
The inline comment explains this as switching “egg cluster-client” IPC, but startMode here is a cluster start option (process vs worker_threads) that impacts how Egg app/agent workers are started. To avoid confusion for readers, consider rewording the comment to describe that it’s selecting Egg cluster start mode under vitest threads pool (and only indirectly affects IPC).
| // when running under vitest threads pool, use worker_threads start mode | |
| // so egg cluster-client uses thread-based IPC instead of process-based | |
| // when running under vitest threads pool, choose Egg's worker_threads cluster start mode | |
| // so Egg app/agent run in worker threads instead of child processes (only indirectly affecting IPC) |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…app lifecycle When vitest runs with isolate: false, all test files share the same worker and module cache. The app must not be closed between files, otherwise tegg runtime state (LoadUnitInstances) is destroyed and subsequent files fail with "not found load unit for proto". Key changes: - TeggVitestRunner detects isolate: false via config.isolate and sets globalThis.__eggVitestSharedMode flag - setup_vitest.ts afterAll skips app.close() when shared mode is detected - egg-bin propagates EGG_VITEST_ISOLATE env var and isolate config to vitest - onAfterRunFiles() does NOT close app (vitest calls it per-batch, not globally) Verified: 121/126 cnpmcore tests pass with isolate=false (5 pre-existing failures). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
You can also share your feedback on Copilot code review. Take the survey.
| /** | ||
| * Start mode for egg cluster-client | ||
| */ | ||
| startMode?: 'process' | 'worker_threads'; | ||
| } |
There was a problem hiding this comment.
startMode is now part of MockOptions, so re-declaring the same property in MockClusterOptions is redundant and can drift over time. Consider removing startMode from MockClusterOptions unless it needs a different type/behavior there.
| } | ||
|
|
||
| export default class TeggVitestRunner extends VitestTestRunner { | ||
| private sharedMode: boolean; |
There was a problem hiding this comment.
sharedMode is assigned but never read. With noUnusedLocals: true (from @eggjs/tsconfig), this will fail TypeScript compilation (TS6133). Consider removing the class field and using a local const sharedMode = config.isolate === false just for setting the global flag, or actually using this.sharedMode elsewhere.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugins/mock/src/setup_vitest.ts (1)
61-68: Add CJS parity coverage for this shared-mode lifecycle path.Line 61 introduces process-orchestration-sensitive behavior; please add a matching CommonJS regression case alongside the TypeScript fixture to prevent mode-specific regressions.
Based on learnings: Add regression cases that exercise both CommonJS and TypeScript example apps when features affect HTTP or process orchestration.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/mock/src/setup_vitest.ts` around lines 61 - 68, The shared-mode early return (const sharedMode = ...; if (sharedMode) return;) in setup_vitest.ts creates a lifecycle path exercised only by the TypeScript fixture; add a matching CommonJS regression test that runs the same shared-mode scenario so CJS builds exercise the early-return path too—create a CJS fixture app mirroring the existing TypeScript example and a vitest case that sets __eggVitestSharedMode or EGG_VITEST_ISOLATE='false' to trigger the sharedMode branch, ensuring the test suite invokes the same process-orchestration behavior and verifies teardown is skipped for CJS as it is for TS.tegg/core/vitest/src/runner.ts (1)
88-93: Useconfig.isolate === falsefor explicit clarity and maintainability.The current
!config.isolateworks correctly since Vitest always providesisolateas aboolean(neverundefined). However, an explicitconfig.isolate === falsecheck better communicates intent and is more maintainable for future readers.Proposed change
- this.sharedMode = !config.isolate; + this.sharedMode = config.isolate === false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/vitest/src/runner.ts` around lines 88 - 93, The constructor currently sets this.sharedMode using a negation (!config.isolate); change it to an explicit comparison to false by assigning this.sharedMode = config.isolate === false to improve clarity and maintainability; update the constructor code path that checks this.sharedMode (same constructor and any logic referencing sharedMode) to rely on the new explicit boolean assignment (symbols: constructor, sharedMode, config.isolate).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tegg/core/vitest/src/runner.ts`:
- Around line 99-104: Add an explicit final teardown by overriding onAfterRunAll
in the tegg Vitest runner to sweep fileAppMap when running in shared mode
(isolate: false): implement override onAfterRunAll() to iterate fileAppMap, skip
any entries guarded by __eggVitestSharedMode if still needed, call app.close()
(or the existing closeApp helper) inside a try/catch for each app, log errors,
and then clear fileAppMap so apps aren’t left alive after the worker finishes;
ensure you still call super.onAfterRunAll() at the end.
---
Nitpick comments:
In `@plugins/mock/src/setup_vitest.ts`:
- Around line 61-68: The shared-mode early return (const sharedMode = ...; if
(sharedMode) return;) in setup_vitest.ts creates a lifecycle path exercised only
by the TypeScript fixture; add a matching CommonJS regression test that runs the
same shared-mode scenario so CJS builds exercise the early-return path
too—create a CJS fixture app mirroring the existing TypeScript example and a
vitest case that sets __eggVitestSharedMode or EGG_VITEST_ISOLATE='false' to
trigger the sharedMode branch, ensuring the test suite invokes the same
process-orchestration behavior and verifies teardown is skipped for CJS as it is
for TS.
In `@tegg/core/vitest/src/runner.ts`:
- Around line 88-93: The constructor currently sets this.sharedMode using a
negation (!config.isolate); change it to an explicit comparison to false by
assigning this.sharedMode = config.isolate === false to improve clarity and
maintainability; update the constructor code path that checks this.sharedMode
(same constructor and any logic referencing sharedMode) to rely on the new
explicit boolean assignment (symbols: constructor, sharedMode, config.isolate).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1799e1b0-cd9e-4e03-a274-bb95b1d5ed04
📒 Files selected for processing (3)
plugins/mock/src/setup_vitest.tstegg/core/vitest/src/runner.tstools/egg-bin/src/commands/test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/egg-bin/src/commands/test.ts
| override onAfterRunFiles(): void { | ||
| // NOTE: vitest calls onAfterRunFiles() after each batch of files, not once | ||
| // after all files globally. In shared mode (isolate: false), we must NOT | ||
| // close the app here — the worker thread termination handles cleanup. | ||
| super.onAfterRunFiles(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether shared-mode code path has an explicit terminal teardown.
rg -n -C3 '__eggVitestSharedMode|EGG_VITEST_ISOLATE|onAfterRunFiles|afterAll|app\.close\(' tegg/core/vitest/src/runner.ts plugins/mock/src/setup_vitest.tsRepository: eggjs/egg
Length of output: 3266
🏁 Script executed:
#!/bin/bash
# Search for worker termination cleanup, app lifecycle hooks, and final teardown mechanisms
rg -n -C2 'onAfterRunAll|teardown|cleanup|preExit|LifecyclePreDestroy|worker.*terminat' tegg/core/vitest/src/runner.ts
# Also check the app/context lifecycle
rg -n -C2 'app\.close\(\)|teardown|destroy|LifecyclePreDestroy' tegg/ --type ts | head -50Repository: eggjs/egg
Length of output: 3809
🏁 Script executed:
#!/bin/bash
# Look at the complete runner class structure
cat -n tegg/core/vitest/src/runner.ts | head -120
# Check for other lifecycle hooks in the class
rg -n 'override\s+\w+\(' tegg/core/vitest/src/runner.tsRepository: eggjs/egg
Length of output: 4903
🏁 Script executed:
#!/bin/bash
# Search for all methods in the runner class to see complete structure
rg -n 'override|async.*\(' tegg/core/vitest/src/runner.ts | grep -E 'override|^\s*async'
# Also check what methods VitestTestRunner provides by looking at its type usage
rg -n 'onAfterRun|afterAll|afterEach|beforeAll|beforeEach' tegg/core/vitest/src/runner.tsRepository: eggjs/egg
Length of output: 561
🏁 Script executed:
#!/bin/bash
# Get the complete file to see all methods
wc -l tegg/core/vitest/src/runner.ts
# Show the end of the file to see if there's a destructor or final hook
tail -50 tegg/core/vitest/src/runner.ts
# Search for process event handlers or vitest final hooks
rg -n 'process\.|\.on\(|\.once\(' tegg/core/vitest/src/runner.tsRepository: eggjs/egg
Length of output: 1598
🏁 Script executed:
#!/bin/bash
# Check if there are any tests verifying cleanup in shared mode
rg -n 'shared.*mode|isolate.*false|resource.*leak|app.*close' tegg/core/vitest/test/ plugins/mock/test/ -A2 -B2
# Check if there's documentation about worker cleanup
find . -name "*.md" -o -name "*.txt" | xargs rg -l 'shared mode|isolate.*false|worker.*cleanup' 2>/dev/null | head -5Repository: eggjs/egg
Length of output: 26743
🏁 Script executed:
#!/bin/bash
# Check if there's documentation or code about how vitest cleanup works
rg -n 'VitestTestRunner|onAfterRunAll|onAfterRun[^a-zA-Z]' tegg/core/vitest/src/ -A3
# Check if app has lifecycle hooks for cleanup
rg -n '@LifecyclePreDestroy|preDestroy|cleanup' tegg/ --type ts -A2 | head -30
# Look for any signal handlers or process exit handlers in the test setup
rg -n 'process\.on|process\.once|\.on\(' plugins/mock/src/setup_vitest.ts tegg/core/vitest/src/Repository: eggjs/egg
Length of output: 3339
🏁 Script executed:
#!/bin/bash
# Check if there's any explicit app cleanup in shared-mode or if it relies on process termination
# Look for how the app is stored and if there's any cleanup on process exit
rg -n 'fileAppMap|EGG_VITEST_ISOLATE|sharedMode' tegg/core/vitest/src/runner.ts -B2 -A5
# Check if there are any known issues or TODOs about this
rg -n 'TODO|FIXME|XXX|HACK|BUG' tegg/core/vitest/src/runner.ts plugins/mock/src/setup_vitest.tsRepository: eggjs/egg
Length of output: 2170
🏁 Script executed:
#!/bin/bash
# Check the VitestTestRunner API to see what hooks are available
# Search for how other code handles final cleanup in shared-mode scenarios
rg -n 'onAfterRunAll|onFinished|afterAll' tegg/ plugins/ --type ts -B1 -A1 | head -40
# Check if there are integration tests showing shared mode behavior
find tegg/core/vitest/test plugins/mock/test -name "*.test.ts" -type f -exec grep -l "isolate.*false\|shared.*mode" {} \;Repository: eggjs/egg
Length of output: 1873
🏁 Script executed:
#!/bin/bash
# Search for vitest config files that use isolate: false
find . -name "vitest.config.*" -o -name "vite.config.*" | xargs cat 2>/dev/null | grep -C3 "isolate"
# Check package.json test configs
rg -n '"test":|"vitest":|isolate' package.json tsconfig.json 2>/dev/null | head -20
# Check if there are shared-mode specific tests
find tegg/core/vitest/test plugins/mock/test -name "*.test.ts" -exec grep -l "isolate\|shared" {} \;Repository: eggjs/egg
Length of output: 368
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or comments about this design decision
cat -n tegg/core/vitest/src/runner.ts | sed -n '85,105p'
# Look for the complete file to see if there's any cleanup at the end
cat -n tegg/core/vitest/src/runner.ts | tail -20
# Check if vitest provides any implicit cleanup or if apps need explicit close
rg -n 'app\.(ready|close|destroy)' plugins/mock/src/setup_vitest.ts -B2 -A2Repository: eggjs/egg
Length of output: 2215
🏁 Script executed:
#!/bin/bash
# Let me check if there's any process-level cleanup or if this truly relies only on process termination
# Look at how apps handle process termination
rg -n 'process\.|SIGTERM|SIGINT|exit.*handler' tegg/ plugins/ --type ts -B2 -A2 | head -50
# Check the app's close() implementation to understand what needs cleanup
rg -n 'app\.close\(' tegg/core/runtime/src/ --type ts -B1 -A3 | head -40Repository: eggjs/egg
Length of output: 3766
Missing explicit teardown for shared-mode apps.
In shared mode (isolate: false), apps stored in fileAppMap are kept alive across files but never explicitly closed. The per-file afterAll hook correctly skips cleanup via the __eggVitestSharedMode guard—this is intentional and prevents destroying tegg runtime state. However, there is no corresponding final teardown when the test worker completes. The comment defers cleanup to "worker thread termination," but this is not explicitly implemented, leaving app instances and their resources unclosed in long-lived worker scenarios.
Consider adding an explicit cleanup hook (e.g., onAfterRunAll) to close all apps in fileAppMap when shared-mode testing finishes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/vitest/src/runner.ts` around lines 99 - 104, Add an explicit final
teardown by overriding onAfterRunAll in the tegg Vitest runner to sweep
fileAppMap when running in shared mode (isolate: false): implement override
onAfterRunAll() to iterate fileAppMap, skip any entries guarded by
__eggVitestSharedMode if still needed, call app.close() (or the existing
closeApp helper) inside a try/catch for each app, log errors, and then clear
fileAppMap so apps aren’t left alive after the worker finishes; ensure you still
call super.onAfterRunAll() at the end.
Summary
--poolflag toegg-bin testcommand withforksandthreadsoptions (default:threads)EGG_VITEST_POOLenvironment variable@eggjs/mocktoworker_threadsstart mode when running in threads pool, so cluster-client uses thread-based IPC instead of process-basedTest plan
--pool threadswith cluster-client fixture🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Behavior
Tests / Examples