feat: add tegg vitest adapter workspace#401
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:
📝 WalkthroughWalkthroughAdds a new Changes
Sequence DiagramsequenceDiagram
participant Vitest as Vitest
participant Runner as TeggVitestRunner
participant App as EggApp
participant Store as ctxStorage
Vitest->>Runner: collect file
Runner->>App: getApp()/resolve
App-->>Runner: app ready (cached per file)
Vitest->>Runner: onBeforeRunSuite
Runner->>Store: capture suite store
Runner->>Store: enterWith(suite store)
Vitest->>Runner: onBeforeRunTask
Runner->>Runner: createHeldScope(task)
Runner->>Store: set task store / enterWith(task store)
Vitest->>Store: test executes with task store
Vitest->>Runner: onAfterRunTask
Runner->>Runner: releaseHeldScope -> restore previous store
Vitest->>Runner: onAfterRunSuite
Runner->>Store: restore suite store / cleanup
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
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)
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 |
Summary of ChangesHello @zhizhizhina, 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 introduces a new Vitest adapter for the Egg.js/Tegg ecosystem, enabling developers to write tests using Vitest while maintaining the familiar context injection and lifecycle management provided by 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
|
![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 new package, @eggjs/tegg-vitest-adapter, to integrate tegg with vitest. The implementation cleverly uses vitest hooks to manage tegg context by monkey-patching app.ctxStorage.getStore(), effectively emulating egg-mock's behavior for Mocha. The code is well-structured, and the inclusion of comprehensive tests for context semantics, error handling, and lifecycle is commendable. I have a few suggestions to improve the documentation, package configuration, and code clarity.
| { | ||
| "extends": "../../tsconfig.json", | ||
| "compilerOptions": { | ||
| "outDir": "dist", | ||
| "baseUrl": "./" | ||
| }, | ||
| "exclude": [ | ||
| "dist", | ||
| "node_modules", | ||
| "test" | ||
| ] | ||
| } |
There was a problem hiding this comment.
This file, tsconfig.pub.json, has the exact same content as tsconfig.json. This duplication can lead to maintenance issues. Consider removing this file and updating the tsc:pub script in package.json to use tsconfig.json. If there's a reason for two separate files, one should probably extend the other to avoid repeating configuration.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@core/vitest-adapter/package.json`:
- Around line 45-47: The package.json currently sets the Node engine to "node":
">=14.0.0" which is too low for Vitest 1.6; update the "engines" entry so the
node requirement is ">=18.0.0" (modify the "engines" block in
core/vitest-adapter/package.json) to prevent installs on unsupported Node
versions and ensure compatibility with Vitest 1.6.
In `@core/vitest-adapter/README.md`:
- Around line 77-80: Remove the "Command Output (local)" section from
core/vitest-adapter/README.md since it contains local dev notes and stale info;
locate the section header "## Command Output (local)" and delete that header
plus its bullet list so the README contains only consumer-facing documentation.
In `@core/vitest-adapter/src/index.ts`:
- Around line 130-143: safeGetApp currently swallows the thrown error from
getApp and logs only a generic message; update the catch in safeGetApp to
include the caught error (err) in the warning so diagnostics are clearer—when
getApp() throws, set STATE.disabled and STATE.warned as before but change the
console.warn call inside the !STATE.warned block to include the error (e.g.,
include err or err.stack) in the log message so the failure details are visible
for debugging.
🧹 Nitpick comments (7)
core/vitest-adapter/tsconfig.json (1)
1-12:tsconfig.jsonandtsconfig.pub.jsonare identical.Both files have the same content. Typically
tsconfig.jsonincludestest/for IDE type-checking during development, whiletsconfig.pub.jsonexcludes it for publishing. Consider removing"test"from theexcludelist here so that editors pick up test files for diagnostics.Suggested diff
"exclude": [ "dist", - "node_modules", - "test" + "node_modules" ]core/vitest-adapter/package.json (1)
48-56:vitestshould be apeerDependency, not just adevDependency.The adapter imports Vitest's
beforeAll/beforeEach/afterEach/afterAllhooks, so consumers must have Vitest installed. Declaring it as apeerDependencymakes this contract explicit and avoids version mismatches.Suggested change
"dependencies": { "egg-mock": "^5.5.0" }, + "peerDependencies": { + "vitest": "^1.6.0 || ^2.0.0" + }, "devDependencies": { "@types/node": "^20.2.4", "typescript": "^5.0.4", "vitest": "^1.6.0",core/vitest-adapter/test/fake_app.ts (1)
36-45:mockContextmutatesgetStorein place — scope inference depends on call order.After the first call,
this.ctxStorage.getStoreis replaced (line 42), so subsequent calls always see a truthy store and assignscope: 'test'. This is fine for the current test scenarios but could be confusing if tests are added that callmockContextmultiple times expecting independent scope detection. A comment clarifying this would help future contributors.core/vitest-adapter/test/get_store_restore.test.ts (1)
57-57:forEachcallback should not implicitly return a value.The concise arrow body causes the return value of
assert.notStrictEqualto be returned from the callback. Use a block body to satisfy the linter and clarify intent.Proposed fix
- testCtxList.forEach(ctx => assert.notStrictEqual(ctx, suiteCtx)); + testCtxList.forEach(ctx => { assert.notStrictEqual(ctx, suiteCtx); });core/vitest-adapter/test/hooks.test.ts (1)
65-78: VariableitCtxListshadows the outer declaration on line 21.The inner
itCtxList(array) shadows the outeritCtxList(Record) declared on line 21. While the scoping works correctly, consider renaming to something likemultiItCtxListto avoid confusion.core/vitest-adapter/src/index.ts (2)
52-58: Module-levelSTATEis shared across all calls tosetupTeggVitestAdapter.If
setupTeggVitestAdapteris invoked more than once in the same file (e.g. at module level and inside adescribe), the sharedSTATEwill cause both registrations to stomp on each other. Consider adding a guard to warn or no-op on duplicate calls.Proposed guard
+let _installed = false; + export function setupTeggVitestAdapter(options: TeggVitestAdapterOptions = {}) { + if (_installed) { + console.warn('[tegg-vitest-adapter] setupTeggVitestAdapter() called more than once; ignoring.'); + return; + } + _installed = true; + const parallel = options.parallel ?? defaultIsParallel();
180-194:afterEachcallssafeGetApp()again — redundant for the common case.Every
afterEachinvocation callssafeGetApp(), which re-invokes the user-providedgetAppfunction. For the defaultrequire('egg-mock/bootstrap').appthis is cheap (cachedrequire), but a customgetAppwith side effects or async work will be called2N + 2times across N tests. Consider caching the resolved app inSTATEfrombeforeAllinstead of re-resolving each time.
killagu-claw
force-pushed
the
feat/vitest-adapter
branch
from
00d9a55 to
05a0ac8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@core/vitest-adapter/src/runner.ts`:
- Around line 52-73: createHeldScope can hang if ctx.beginModuleScope rejects
before it invokes the inner callback because scopeReady() is never called;
modify createHeldScope to race scopePromise against readyPromise (e.g., await
Promise.race([scopePromise.catch(e => { throw e }), readyPromise])) so that if
beginModuleScope rejects the error propagates immediately instead of blocking,
and ensure scopePromise still refers to the original ctx.beginModuleScope result
(so gate/endScope behavior is preserved) and that any rejection is
awaited/handled to avoid unhandled rejections (reference createHeldScope,
ctx.beginModuleScope, scopePromise, readyPromise, scopeReady, gate, endScope).
- Around line 109-114: TeggVitestRunner's per-file state (fileAppMap,
disabledFiles, and warned) is accumulating across watch-mode reruns because
importFile repopulates them but they are never cleared; update the runner to
reset or selectively clear these collections at the start/end of a run: either
clear fileAppMap.clear(), disabledFiles.clear(), and warned = false at the
beginning of the collection phase (e.g. in the method that runs before
importFile), or add logic in onAfterRunSuite/onAfterRunTask to remove entries
for files not present in the current run; locate the importFile implementation
and the TeggVitestRunner lifecycle hooks (onAfterRunSuite, onAfterRunTask) and
implement the clearing or conditional re-population there so state does not
persist incorrectly between watch-mode reruns.
In `@core/vitest-adapter/test/get_store_restore.test.ts`:
- Around line 56-67: The afterAll block performs multiple assertions then calls
cleanup (await app.close(), await mm.restore()), so if any assertion throws the
cleanup is skipped; modify the afterAll handler (the function passed to
afterAll) to run the assertions inside a try block and call await app.close()
and await mm.restore() in a finally block to guarantee cleanup; reference
getCtx(), suiteCtx, testCtxList, afterEachCtxList and the existing assertions so
you wrap those checks in try { ...assertions... } finally { await app.close();
await mm.restore(); }.
In `@core/vitest-adapter/test/hooks.test.ts`:
- Around line 32-42: In afterAll, wrap the assertions and any code that may
throw in a try block and move the cleanup calls into a finally block so
app.close() and mm.restore() always run; specifically, keep the existing
assertions involving getCtx(), beforeCtx, afterCtx, itCtxList, beforeEachCtxList
and afterEachCtxList inside the try, and put app.close() and mm.restore() in the
finally to guarantee cleanup even if assertions fail.
🧹 Nitpick comments (9)
core/vitest-adapter/test/setup.ts (1)
17-21: Mutatingprocess.execArgvin-place may affect the host process unexpectedly.
process.execArgvis the shared, live array for the Node process. Pushing to it in-place means any other code that readsexecArgv(e.g., Vitest's worker spawning logic) sees the mutation. This is likely intentional for child-worker ts-node registration, but worth a brief comment in code to clarify intent.core/vitest-adapter/src/runner.ts (3)
147-154: Single-warning flag suppresses errors from subsequent files.
this.warned = truemeans that ifgetApp()fails for multiple files with potentially different errors, only the first error is logged. Consider always logging the error but perhaps throttling the full stack trace.Proposed fix — always log the error, throttle the preamble
} catch (err) { this.disabledFiles.add(filepath); - if (!this.warned) { - this.warned = true; - // eslint-disable-next-line no-console - console.warn('[tegg-vitest-adapter] getApp failed, skip context injection.', err); - } + // eslint-disable-next-line no-console + console.warn(`[tegg-vitest-adapter] getApp failed for ${filepath}, skip context injection.`, err);
120-159:importFile— communication viaglobalThis.__teggVitestConfigis pragmatic but fragile.The convention of setting
globalThis.__teggVitestConfigduring module evaluation (fromconfigureTeggRunner) and reading it here works becausesuper.importFileexecutes the module synchronously (within the same task). This is worth a brief doc comment since it's a non-obvious contract — if Vitest ever changes to async module evaluation, this would break silently.
161-192: No error handling aroundcreateHeldScopecalls in hooks.If
createHeldScopethrows (e.g.,beginModuleScoperejects), the error propagates out ofonBeforeRunSuite/onBeforeRunTaskand may leave partial state in the maps (e.g.,fileScopeMaportaskScopeMap). Consider wrapping in try/catch to clean up partial state and still call thesuperhook.Example for onBeforeRunSuite
if (typeof suiteCtx.beginModuleScope === 'function') { - suiteScope = await createHeldScope(suiteCtx); - debugLog('suite held scope created'); + try { + suiteScope = await createHeldScope(suiteCtx); + debugLog('suite held scope created'); + } catch (err) { + controller.restore(); + throw err; + } }Also applies to: 214-238
core/vitest-adapter/src/shared.ts (2)
4-13:ctxStoragetype: optional method markers are redundant given the intersection withAsyncLocalStorage<any>.
AsyncLocalStorage<any>already definesgetStore()andenterWith(). The?markers on these methods in the object-literal part of the intersection don't actually make them optional on the resulting type — the intersection withAsyncLocalStorage<any>guarantees they exist. If the intent is to handle cases wherectxStoragemight be a partial/duck-typed object (not a realAsyncLocalStorage), consider using a union or a standalone interface instead of the intersection.
54-58:defaultGetAppisasyncbut performs no async work.The
require()call is synchronous. Theasynckeyword here is harmless (it wraps the return in a resolved Promise), but it's slightly misleading. It does match thePromise<EggMockApp | undefined>return type in the interface, so this is a minor style point.core/vitest-adapter/test/fixture_app.test.ts (1)
8-11: MixingcreateRequire(import.meta.url)with__dirname.Line 8 creates a CJS-style
requirefromimport.meta.url(ESM pattern), but lines 10 and 14 use__dirname, which is a CJS global. Vitest may shim__dirnamefor you, but the inconsistency is fragile — if the project switches to native ESM or changes Vitest's config,__dirnamewill beundefined. Consider deriving the directory fromimport.meta.urlconsistently:import { fileURLToPath } from 'node:url'; const __dirname = path.dirname(fileURLToPath(import.meta.url));core/vitest-adapter/test/hooks.test.ts (1)
7-10: Cross-package fixture dependency is fragile.This test reaches into
plugin/tegg/test/fixtures/apps/egg-app(three directories up). If that fixture is refactored or moved, this test silently breaks. Consider either copying a minimal fixture intocore/vitest-adapter/test/fixtures/(asfixture_app.test.tsdoes withdemo-app) or adding a comment documenting the dependency.core/vitest-adapter/test/get_store_restore.test.ts (1)
7-10: Same cross-package fixture dependency ashooks.test.ts.Both this file and
hooks.test.tsreach intoplugin/tegg/test/fixtures/apps/egg-app. Consider extracting a shared local fixture or a shared test helper to reduce duplication and fragility.
| async function createHeldScope(ctx: any): Promise<HeldScope> { | ||
| let endScope!: () => void; | ||
| const gate = new Promise<void>(resolve => { | ||
| endScope = resolve; | ||
| }); | ||
|
|
||
| let scopeReady!: () => void; | ||
| const readyPromise = new Promise<void>(resolve => { | ||
| scopeReady = resolve; | ||
| }); | ||
|
|
||
| const scopePromise = ctx.beginModuleScope(async () => { | ||
| // init() has completed at this point, signal readiness | ||
| scopeReady(); | ||
| await gate; | ||
| }); | ||
|
|
||
| // Wait for init() inside beginModuleScope to finish | ||
| await readyPromise; | ||
|
|
||
| return { ctx, scopePromise, endScope }; | ||
| } |
There was a problem hiding this comment.
createHeldScope can hang if beginModuleScope rejects before invoking the inner function.
If ctx.beginModuleScope throws (or rejects) during its initialization phase—before calling the inner callback—scopeReady() is never invoked, so await readyPromise (line 70) will block forever. Meanwhile, scopePromise rejects with no handler, causing an unhandled rejection.
Race scopePromise against readyPromise so that an init failure propagates immediately:
Proposed fix
- // Wait for init() inside beginModuleScope to finish
- await readyPromise;
+ // Wait for init() inside beginModuleScope to finish,
+ // but bail immediately if beginModuleScope itself rejects.
+ await Promise.race([readyPromise, scopePromise]);If scopePromise rejects first, the Promise.race will propagate the error. If readyPromise resolves first, we continue as before (and scopePromise remains pending, held open by the gate).
📝 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.
| async function createHeldScope(ctx: any): Promise<HeldScope> { | |
| let endScope!: () => void; | |
| const gate = new Promise<void>(resolve => { | |
| endScope = resolve; | |
| }); | |
| let scopeReady!: () => void; | |
| const readyPromise = new Promise<void>(resolve => { | |
| scopeReady = resolve; | |
| }); | |
| const scopePromise = ctx.beginModuleScope(async () => { | |
| // init() has completed at this point, signal readiness | |
| scopeReady(); | |
| await gate; | |
| }); | |
| // Wait for init() inside beginModuleScope to finish | |
| await readyPromise; | |
| return { ctx, scopePromise, endScope }; | |
| } | |
| async function createHeldScope(ctx: any): Promise<HeldScope> { | |
| let endScope!: () => void; | |
| const gate = new Promise<void>(resolve => { | |
| endScope = resolve; | |
| }); | |
| let scopeReady!: () => void; | |
| const readyPromise = new Promise<void>(resolve => { | |
| scopeReady = resolve; | |
| }); | |
| const scopePromise = ctx.beginModuleScope(async () => { | |
| // init() has completed at this point, signal readiness | |
| scopeReady(); | |
| await gate; | |
| }); | |
| // Wait for init() inside beginModuleScope to finish, | |
| // but bail immediately if beginModuleScope itself rejects. | |
| await Promise.race([readyPromise, scopePromise]); | |
| return { ctx, scopePromise, endScope }; | |
| } |
🤖 Prompt for AI Agents
In `@core/vitest-adapter/src/runner.ts` around lines 52 - 73, createHeldScope can
hang if ctx.beginModuleScope rejects before it invokes the inner callback
because scopeReady() is never called; modify createHeldScope to race
scopePromise against readyPromise (e.g., await
Promise.race([scopePromise.catch(e => { throw e }), readyPromise])) so that if
beginModuleScope rejects the error propagates immediately instead of blocking,
and ensure scopePromise still refers to the original ctx.beginModuleScope result
(so gate/endScope behavior is preserved) and that any rejection is
awaited/handled to avoid unhandled rejections (reference createHeldScope,
ctx.beginModuleScope, scopePromise, readyPromise, scopeReady, gate, endScope).
| export default class TeggVitestRunner extends VitestTestRunner { | ||
| private fileScopeMap = new Map<string, FileScopeState>(); | ||
| private taskScopeMap = new Map<string, TaskScopeState>(); | ||
| private fileAppMap = new Map<string, FileAppState>(); | ||
| private disabledFiles = new Set<string>(); | ||
| private warned = false; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the runner.ts file to understand the full context
cat -n core/vitest-adapter/src/runner.ts | head -150Repository: eggjs/tegg
Length of output: 5326
🏁 Script executed:
# Search for how TeggVitestRunner is instantiated or used
rg -n "TeggVitestRunner\|new.*Runner" --type=ts -C 3 core/vitest-adapter/Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Look for lifecycle cleanup methods like destroy, cleanup, or reset
rg -n "destroy\|cleanup\|reset\|teardown" --type=ts core/vitest-adapter/src/Repository: eggjs/tegg
Length of output: 36
🌐 Web query:
Vitest runner instance lifecycle in watch mode
💡 Result:
In Vitest watch mode (i.e. vitest / vitest watch), there isn’t a single “runner instance” that’s torn down and recreated on every rerun. Instead, Vitest keeps the main process (and Vite dev server/module graph) alive and triggers new test runs on changes, with different lifetimes depending on which layer you’re talking about:
1) Main process (Vitest/Vite server)
- Initialized once when you start watch mode; it stays alive between reruns. The “initialization” phase can run again only if the config (or its imports) changes. [1]
globalSetupruns once per Vitest process start, and itsteardownruns only when the process exits, not between watch reruns. [1][2]
2) Workers / pool processes (where test files actually execute)
- Vitest spawns workers based on your
pool(forks,threads,vmForks,vmThreads, etc.). [2] - By default, workers are not reused (to preserve isolation). Workers are reused only when:
isolateis disabled, or- you use VM pools (
vmThreads/vmForks) where the VM layer provides isolation. [2]
3) Per-rerun behavior (what repeats when you edit files / press r)
- A rerun starts a new test run, and Vitest will only re-run affected test files (based on the module graph, HMR-style). [1][3]
- For each test file that runs in that rerun:
setupFilesexecute before the test file (again for those files). [1][2]- file-level code re-imports according to isolation/module caching rules (notably, if
isolateis disabled, imported modules can remain cached even thoughsetupFilesstill re-run). [2]
4) If you need “global setup/teardown per rerun” in watch mode
Vitest doesn’t currently re-run globalSetup/teardown on every watch rerun. The supported workaround is a custom reporter using onWatcherRerun (and onInit/onFinished). [4][5]
Sources:
[1] Vitest “Test Run Lifecycle” (watch mode + phases) (vitest.dev)
[2] Vitest lifecycle details on worker reuse + teardown in watch mode (cn.vitest.dev)
[3] Vitest “Features” guide (watch mode reruns related tests via module graph) (main.vitest.dev)
[4] Vitest GitHub Discussion #6719 (maintainer: use reporter onWatcherRerun) (github.com)
[5] Vitest Reporters API (includes onWatcherRerun, onInit, etc.) (main.vitest.dev)
Citations:
- 1: https://vitest.dev/guide/lifecycle?utm_source=openai
- 2: https://cn.vitest.dev/guide/lifecycle?utm_source=openai
- 3: https://main.vitest.dev/guide/features.html?utm_source=openai
- 4: Global setup/teardown functions for all tests in watch mode vitest-dev/vitest#6719
- 5: https://main.vitest.dev/advanced/api/reporters?utm_source=openai
🏁 Script executed:
# Get the complete runner.ts file to check for lifecycle cleanup methods
wc -l core/vitest-adapter/src/runner.tsRepository: eggjs/tegg
Length of output: 91
🏁 Script executed:
# Read the rest of the file to look for cleanup/destroy/reset methods
tail -200 core/vitest-adapter/src/runner.ts | cat -nRepository: eggjs/tegg
Length of output: 7523
🏁 Script executed:
# Search for any methods that might clear state maps
rg -n "fileScopeMap\|taskScopeMap\|fileAppMap\|disabledFiles" --type=ts core/vitest-adapter/src/runner.ts -A 2 -B 2Repository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Check what methods VitestTestRunner base class might provide
rg -n "override|class.*VitestTestRunner" --type=ts core/vitest-adapter/src/ -A 5Repository: eggjs/tegg
Length of output: 593
🏁 Script executed:
# Check if there are any lifecycle hooks like onRun, reset, or other cleanup mechanisms
rg -n "onRun|reset|clear|teardown" --type=ts core/vitest-adapter/src/runner.tsRepository: eggjs/tegg
Length of output: 36
🏁 Script executed:
# Look for config files or setup that might instantiate or reset the runner
find core/vitest-adapter -name "*.ts" -o -name "*.js" | head -20Repository: eggjs/tegg
Length of output: 692
🏁 Script executed:
# Check the package exports to see how runner is used
cat -n core/vitest-adapter/src/index.tsRepository: eggjs/tegg
Length of output: 841
🏁 Script executed:
# Check the vitest config to understand how the runner is configured
cat -n core/vitest-adapter/vitest.config.tsRepository: eggjs/tegg
Length of output: 1629
🏁 Script executed:
# Look at the test setup to understand collection behavior across runs
cat -n core/vitest-adapter/test/setup.tsRepository: eggjs/tegg
Length of output: 997
State maps accumulate across runs — fileAppMap, disabledFiles, and warned never clear in watch mode.
The Vitest runner instance persists across watch-mode reruns. During each collection phase, importFile populates fileAppMap (line 85), adds to disabledFiles (line 89), and may set warned = true (line 91), but these are never cleared between runs. This causes:
fileAppMapto accumulate stale entries for files across rerunsdisabledFilesentries to persist permanently (disabled files cannot recover)warnedflag to suppress warnings even if later test files fail with getApp
While fileScopeMap and taskScopeMap are properly cleaned in onAfterRunSuite and onAfterRunTask, the importFile-populated collections need reset or conditional re-population logic in watch mode.
🤖 Prompt for AI Agents
In `@core/vitest-adapter/src/runner.ts` around lines 109 - 114, TeggVitestRunner's
per-file state (fileAppMap, disabledFiles, and warned) is accumulating across
watch-mode reruns because importFile repopulates them but they are never
cleared; update the runner to reset or selectively clear these collections at
the start/end of a run: either clear fileAppMap.clear(), disabledFiles.clear(),
and warned = false at the beginning of the collection phase (e.g. in the method
that runs before importFile), or add logic in onAfterRunSuite/onAfterRunTask to
remove entries for files not present in the current run; locate the importFile
implementation and the TeggVitestRunner lifecycle hooks (onAfterRunSuite,
onAfterRunTask) and implement the clearing or conditional re-population there so
state does not persist incorrectly between watch-mode reruns.
| afterAll(async () => { | ||
| // After all tests, suite context is restored | ||
| assert.strictEqual(getCtx(), suiteCtx); | ||
| assert.strictEqual(testCtxList.length, afterEachCtxList.length); | ||
| testCtxList.forEach((ctx, index) => { | ||
| assert.notStrictEqual(ctx, suiteCtx); | ||
| assert.strictEqual(ctx, afterEachCtxList[index]); | ||
| }); | ||
| assert.notStrictEqual(testCtxList[0], testCtxList[1]); | ||
| await app.close(); | ||
| await mm.restore(); | ||
| }); |
There was a problem hiding this comment.
Same afterAll cleanup risk as in hooks.test.ts — assertions can prevent app.close().
Same pattern: if any assertion on lines 58–64 throws, app.close() and mm.restore() on lines 65–66 are skipped. Wrap assertions in try/finally to guarantee cleanup.
Proposed fix
afterAll(async () => {
- // After all tests, suite context is restored
- assert.strictEqual(getCtx(), suiteCtx);
- assert.strictEqual(testCtxList.length, afterEachCtxList.length);
- testCtxList.forEach((ctx, index) => {
- assert.notStrictEqual(ctx, suiteCtx);
- assert.strictEqual(ctx, afterEachCtxList[index]);
- });
- assert.notStrictEqual(testCtxList[0], testCtxList[1]);
- await app.close();
- await mm.restore();
+ try {
+ assert.strictEqual(getCtx(), suiteCtx);
+ assert.strictEqual(testCtxList.length, afterEachCtxList.length);
+ testCtxList.forEach((ctx, index) => {
+ assert.notStrictEqual(ctx, suiteCtx);
+ assert.strictEqual(ctx, afterEachCtxList[index]);
+ });
+ assert.notStrictEqual(testCtxList[0], testCtxList[1]);
+ } finally {
+ await app.close();
+ await mm.restore();
+ }
});🤖 Prompt for AI Agents
In `@core/vitest-adapter/test/get_store_restore.test.ts` around lines 56 - 67, The
afterAll block performs multiple assertions then calls cleanup (await
app.close(), await mm.restore()), so if any assertion throws the cleanup is
skipped; modify the afterAll handler (the function passed to afterAll) to run
the assertions inside a try block and call await app.close() and await
mm.restore() in a finally block to guarantee cleanup; reference getCtx(),
suiteCtx, testCtxList, afterEachCtxList and the existing assertions so you wrap
those checks in try { ...assertions... } finally { await app.close(); await
mm.restore(); }.
| afterAll(async () => { | ||
| afterCtx = getCtx(); | ||
| assert(beforeCtx); | ||
| assert(beforeCtx !== itCtxList.foo); | ||
| assert(itCtxList.foo !== itCtxList.bar); | ||
| assert.strictEqual(afterCtx, beforeCtx); | ||
| assert.strictEqual(beforeEachCtxList.foo, afterEachCtxList.foo); | ||
| assert.strictEqual(beforeEachCtxList.foo, itCtxList.foo); | ||
| await app.close(); | ||
| await mm.restore(); | ||
| }); |
There was a problem hiding this comment.
Assertions before cleanup in afterAll risk leaking the app on failure.
If any assertion on lines 34–39 throws, app.close() and mm.restore() on lines 40–41 are never reached. This can leak the Egg app process/connections and affect subsequent test files.
Proposed fix: move cleanup into a finally block
afterAll(async () => {
- afterCtx = getCtx();
- assert(beforeCtx);
- assert(beforeCtx !== itCtxList.foo);
- assert(itCtxList.foo !== itCtxList.bar);
- assert.strictEqual(afterCtx, beforeCtx);
- assert.strictEqual(beforeEachCtxList.foo, afterEachCtxList.foo);
- assert.strictEqual(beforeEachCtxList.foo, itCtxList.foo);
- await app.close();
- await mm.restore();
+ try {
+ afterCtx = getCtx();
+ assert(beforeCtx);
+ assert(beforeCtx !== itCtxList.foo);
+ assert(itCtxList.foo !== itCtxList.bar);
+ assert.strictEqual(afterCtx, beforeCtx);
+ assert.strictEqual(beforeEachCtxList.foo, afterEachCtxList.foo);
+ assert.strictEqual(beforeEachCtxList.foo, itCtxList.foo);
+ } finally {
+ await app.close();
+ await mm.restore();
+ }
});📝 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.
| afterAll(async () => { | |
| afterCtx = getCtx(); | |
| assert(beforeCtx); | |
| assert(beforeCtx !== itCtxList.foo); | |
| assert(itCtxList.foo !== itCtxList.bar); | |
| assert.strictEqual(afterCtx, beforeCtx); | |
| assert.strictEqual(beforeEachCtxList.foo, afterEachCtxList.foo); | |
| assert.strictEqual(beforeEachCtxList.foo, itCtxList.foo); | |
| await app.close(); | |
| await mm.restore(); | |
| }); | |
| afterAll(async () => { | |
| try { | |
| afterCtx = getCtx(); | |
| assert(beforeCtx); | |
| assert(beforeCtx !== itCtxList.foo); | |
| assert(itCtxList.foo !== itCtxList.bar); | |
| assert.strictEqual(afterCtx, beforeCtx); | |
| assert.strictEqual(beforeEachCtxList.foo, afterEachCtxList.foo); | |
| assert.strictEqual(beforeEachCtxList.foo, itCtxList.foo); | |
| } finally { | |
| await app.close(); | |
| await mm.restore(); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In `@core/vitest-adapter/test/hooks.test.ts` around lines 32 - 42, In afterAll,
wrap the assertions and any code that may throw in a try block and move the
cleanup calls into a finally block so app.close() and mm.restore() always run;
specifically, keep the existing assertions involving getCtx(), beforeCtx,
afterCtx, itCtxList, beforeEachCtxList and afterEachCtxList inside the try, and
put app.close() and mm.restore() in the finally to guarantee cleanup even if
assertions fail.
05a0ac8 to
e16ea66
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@core/vitest-adapter/package.json`:
- Around line 50-51: The peerDependencies claim compatibility with Vitest v1–v3
but only v1 is tested; update either the CI to test vitest@^2 and vitest@^3 (add
matrix entries in your workflow) to verify TeggVitestRunner (which extends
VitestTestRunner and invokes lifecycle methods like super.onTaskUpdate and
super.extendTaskContext) works with the changed payload/signature (onTaskUpdate
payload shape, extendTaskContext signature), or tighten peerDependencies in
package.json to only the tested range (e.g., ^1.6.0) until you add those CI
jobs; ensure devDependencies include the same tested vitest versions so local
dev/tests match CI.
🧹 Nitpick comments (1)
core/vitest-adapter/src/runner.ts (1)
119-123:source as anycast hides a potential type mismatch across vitest versions.The
as anycast on line 123 suggests thesourceparameter type doesn't match the base class signature. Given the broad peer dependency range (^1.6.0 || ^2.0.0 || ^3.0.0), verify thatimportFile's signature is compatible across all declared versions.
| "peerDependencies": { | ||
| "vitest": "^1.6.0 || ^2.0.0 || ^3.0.0" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the vitest-adapter implementation
find . -path "*/core/vitest-adapter*" -type f -name "*.ts" -o -name "*.js" | head -20Repository: eggjs/tegg
Length of output: 1192
🏁 Script executed:
# Check the runner.ts implementation
find . -path "*/vitest-adapter*" -name "runner.ts" -o -path "*/vitest-adapter*" -name "runner.js"Repository: eggjs/tegg
Length of output: 121
🏁 Script executed:
# Read the vitest-adapter package.json to confirm the peer dependencies and devDependencies
find . -path "*/core/vitest-adapter/package.json" -exec cat {} \;Repository: eggjs/tegg
Length of output: 1354
🌐 Web query:
vitest VitestTestRunner API breaking changes v1 v2 v3 vitest/runners
💡 Result:
Vitest VitestTestRunner / vitest/runners + VitestRunner API breaking changes (v1 → v2 → v3)
v1 → v2 (Runner API)
-
onTaskUpdatepayload shape- v1:
onTaskUpdate?: (task: [string, TaskResult | undefined][]) => Promise<void>(2-tuple items) [1] - v2: same 2-tuple shape (still no task meta in the callback payload) [2]
- v1:
-
Filetask gainedlocal?: booleanin v2- v2
File: includeslocal?: boolean(“initiated without running any tests… populate state on the server side”) [2] - (This field is not present in v1’s doc page; it appears as part of the v2 Runner Tasks model.) [2]
- v2
v2 → v3 (Runner API)
-
onTaskUpdatepayload became a 3-tuple (meta added)- v2:
onTaskUpdate?: (task: [string, TaskResult | undefined][]) => Promise<void>[2] - v3:
onTaskUpdate?: (task: [string, TaskResult | undefined, TaskMeta | undefined][]) => Promise<void>(breaking) [3]
- v2:
-
extendTaskContextsignature changed (generic TaskContext → TestContext)- v2:
extendTaskContext?: <T extends Test | Custom>(context: TaskContext<T>) => TaskContext<T>[2] - v3:
extendTaskContext?: (context: TestContext) => TestContext(breaking) [3]
- v2:
-
New runner hooks / fields in v3
injectValue?: (key: string) => unknown(used withtest.extend(..., { injected: true })) [3]pool?: stringexposed on the runner instance [3]
-
Runner “File task” shape changed
- v2
File: haslocal?: booleanand describesprojectNameas “workspace project” [2] - v3
File: removeslocal?: booleanand describesprojectNameas “test project” [4]
- v2
Notes about VitestTestRunner / vitest/runners
- Across v1/v2/v3 docs, Vitest recommends extending
VitestTestRunnerfromvitest/runnersto retain snapshot support and related runner features [1] [2] [4].
🏁 Script executed:
# Read the runner.ts implementation to see what vitest APIs are actually used
cat -n ./core/vitest-adapter/src/runner.tsRepository: eggjs/tegg
Length of output: 9228
🏁 Script executed:
# Also check the main index.ts for any vitest API usage
cat -n ./core/vitest-adapter/src/index.tsRepository: eggjs/tegg
Length of output: 841
🏁 Script executed:
# Check what's in shared.ts
cat -n ./core/vitest-adapter/src/shared.tsRepository: eggjs/tegg
Length of output: 2589
Peer dependency claims v1–v3 compatibility, but only v1 is tested.
peerDependencies declares ^1.6.0 || ^2.0.0 || ^3.0.0, yet devDependencies only pins ^1.6.0. Vitest's vitest/runners API has documented breaking changes between v2 and v3 (e.g., onTaskUpdate payload shape, extendTaskContext signature). The TeggVitestRunner class extends VitestTestRunner and calls multiple super.*() lifecycle methods, which may be affected by these API changes. Add CI matrix entries for vitest v2 and v3 to verify compatibility, or narrow the peer dependency range until tested.
🤖 Prompt for AI Agents
In `@core/vitest-adapter/package.json` around lines 50 - 51, The peerDependencies
claim compatibility with Vitest v1–v3 but only v1 is tested; update either the
CI to test vitest@^2 and vitest@^3 (add matrix entries in your workflow) to
verify TeggVitestRunner (which extends VitestTestRunner and invokes lifecycle
methods like super.onTaskUpdate and super.extendTaskContext) works with the
changed payload/signature (onTaskUpdate payload shape, extendTaskContext
signature), or tighten peerDependencies in package.json to only the tested range
(e.g., ^1.6.0) until you add those CI jobs; ensure devDependencies include the
same tested vitest versions so local dev/tests match CI.
e16ea66 to
50b038e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/vitest-adapter/src/runner.ts (1)
89-126:importFileresolves app during collection — consider error resilience.Line 91 deletes
__teggVitestConfigbefore the import, and lines 97-98 consume and delete it after. This is a clean approach for per-file config isolation. However, ifconfig.getApp()throws (lines 108-121), the file is silently skipped with no entry infileAppMap, meaning all lifecycle hooks gracefully no-op — which is the correct behavior.One minor note: after the first
getAppfailure,this.warned = truesuppresses warnings for all subsequent files, even if they use differentgetAppfunctions. This could hide distinct failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/vitest-adapter/src/runner.ts` around lines 89 - 126, importFile currently sets a single boolean this.warned when config.getApp() throws, which suppresses warnings for all subsequent files and can hide distinct failures; change this to track warnings per-file (e.g., a Set or Map like this.warnedFiles) or include the filepath in the console.warn and avoid flipping a global flag so each failing getApp call logs at least once; update the error handling in importFile (the try/catch around config.getApp()) to use this.warnedFiles.has(filepath) / this.warnedFiles.add(filepath) or simply remove the global this.warned and always include filepath in the warning message, referencing importFile, config.getApp, this.warned (replace), and fileAppMap to locate and adjust the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/vitest-adapter/test/get_app_throw.test.ts`:
- Around line 17-19: The comment in get_app_throw.test.ts incorrectly states
that getApp is called in onBeforeRunSuite; update the comment to reflect that
config.getApp() is invoked during the import/collect phase (importFile) in
runner.ts (see importFile and getApp references) rather than in
onBeforeRunSuite, and adjust any explanatory text or assertion comment
accordingly so it accurately mentions importFile/collect phase as the location
where getApp is called.
---
Duplicate comments:
In `@core/vitest-adapter/package.json`:
- Around line 1-59: The review contains a duplicate approval/comment and a
lingering note about the vitest peer dependency range; remove the duplicated
review message and update the PR description to explicitly state testing was
only done against vitest v1 (or alternatively adjust the "peerDependencies"
vitest range string to match the actually tested versions), referencing the
"peerDependencies" entry and the vitest version range "^1.6.0 || ^2.0.0 ||
^3.0.0" so reviewers know whether to broaden the test coverage or tighten the
declared range.
In `@core/vitest-adapter/src/runner.ts`:
- Around line 79-83: The fileAppMap and warned fields accumulate across
watch-mode reruns; update the runner to clear these at the start of each
collection cycle by resetting this.fileAppMap = new Map() and this.warned =
false when a collection begins (e.g., at the top of importFile when the incoming
parameter source === 'collect' or by adding/overriding an onBeforeCollect-style
hook); locate the logic in TeggVitestRunner (class name) and ensure the reset
happens before any per-file processing so state does not persist across reruns.
- Around line 42-63: createHeldScope can hang if ctx.beginModuleScope rejects
before the inner callback runs because scopeReady never gets called; fix by
awaiting a race between readyPromise and scopePromise while ensuring a rejection
from scopePromise triggers scopeReady so readyPromise doesn't hang: create
scopePromise via ctx.beginModuleScope(...) as before, then await
Promise.race([readyPromise, scopePromise.catch(err => { scopeReady(); throw err;
})]) (or equivalent) so any rejection from scopePromise resolves readyPromise
and propagates the error; keep references to
gate/endScope/scopeReady/readyPromise/scopePromise as in createHeldScope.
---
Nitpick comments:
In `@core/vitest-adapter/src/runner.ts`:
- Around line 89-126: importFile currently sets a single boolean this.warned
when config.getApp() throws, which suppresses warnings for all subsequent files
and can hide distinct failures; change this to track warnings per-file (e.g., a
Set or Map like this.warnedFiles) or include the filepath in the console.warn
and avoid flipping a global flag so each failing getApp call logs at least once;
update the error handling in importFile (the try/catch around config.getApp())
to use this.warnedFiles.has(filepath) / this.warnedFiles.add(filepath) or simply
remove the global this.warned and always include filepath in the warning
message, referencing importFile, config.getApp, this.warned (replace), and
fileAppMap to locate and adjust the logic.
| it('should not crash suite when getApp throws', () => { | ||
| // The runner calls getApp in onBeforeRunSuite, so it should have been called | ||
| assert(getAppCalls > 0); |
There was a problem hiding this comment.
Inaccurate comment: getApp is called during importFile, not onBeforeRunSuite.
Looking at runner.ts, config.getApp() is invoked at line 109 inside importFile (during the collect phase), not in onBeforeRunSuite.
Suggested fix
it('should not crash suite when getApp throws', () => {
- // The runner calls getApp in onBeforeRunSuite, so it should have been called
+ // The runner calls getApp in importFile (collect phase), so it should have been called
assert(getAppCalls > 0);📝 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.
| it('should not crash suite when getApp throws', () => { | |
| // The runner calls getApp in onBeforeRunSuite, so it should have been called | |
| assert(getAppCalls > 0); | |
| it('should not crash suite when getApp throws', () => { | |
| // The runner calls getApp in importFile (collect phase), so it should have been called | |
| assert(getAppCalls > 0); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/vitest-adapter/test/get_app_throw.test.ts` around lines 17 - 19, The
comment in get_app_throw.test.ts incorrectly states that getApp is called in
onBeforeRunSuite; update the comment to reflect that config.getApp() is invoked
during the import/collect phase (importFile) in runner.ts (see importFile and
getApp references) rather than in onBeforeRunSuite, and adjust any explanatory
text or assertion comment accordingly so it accurately mentions
importFile/collect phase as the location where getApp is called.
50b038e to
5870d78
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/vitest-adapter/src/shared.ts (2)
40-44:defaultGetAppsilently throws ifegg-mockis not installed.The bare
require('egg-mock/bootstrap')will throw aMODULE_NOT_FOUNDerror that propagates as an unhandled rejection. The caller in the runner may or may not catch this gracefully. Consider wrapping in atry/catchreturningundefinedto match the declared return type, consistent with how the adapter is expected to bail whengetAppfails.Proposed fix
export async function defaultGetApp(): Promise<EggMockApp | undefined> { - // eslint-disable-next-line `@typescript-eslint/no-var-requires` - const bootstrap = require('egg-mock/bootstrap'); - return bootstrap?.app; + try { + // eslint-disable-next-line `@typescript-eslint/no-var-requires` + const bootstrap = require('egg-mock/bootstrap'); + return bootstrap?.app; + } catch { + return undefined; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/vitest-adapter/src/shared.ts` around lines 40 - 44, defaultGetApp currently does a bare require('egg-mock/bootstrap') which throws if egg-mock isn't installed; wrap the require call in a try/catch inside defaultGetApp (catching MODULE_NOT_FOUND and other errors) and return undefined on failure to match the Promise<EggMockApp | undefined> signature and avoid unhandled rejections; ensure you still return bootstrap.app when successful and keep the existing eslint comment if needed.
46-54: Same unguardedrequirepattern inrestoreEggMocksIfNeeded.If
egg-mockis not installed,require('egg-mock')on line 49 will throw. Atry/catchwould be consistent with the defensivemm?.restorecheck already present.Proposed fix
export async function restoreEggMocksIfNeeded(restoreMocks: boolean) { if (!restoreMocks) return; - // eslint-disable-next-line `@typescript-eslint/no-var-requires` - const eggMock = require('egg-mock'); - const mm = eggMock?.default || eggMock; - if (mm?.restore) { - await mm.restore(); + try { + // eslint-disable-next-line `@typescript-eslint/no-var-requires` + const eggMock = require('egg-mock'); + const mm = eggMock?.default || eggMock; + if (mm?.restore) { + await mm.restore(); + } + } catch { + // egg-mock not available, nothing to restore } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/vitest-adapter/src/shared.ts` around lines 46 - 54, The unguarded require in restoreEggMocksIfNeeded (eggMock / mm) can throw if the optional dependency isn't installed; wrap the require('egg-mock') in a try/catch inside restoreEggMocksIfNeeded, and on catch simply return (or no-op) so execution continues safely; then keep the existing mm?.restore check and await mm.restore() when present. Ensure you reference eggMock and mm exactly as in the diff so the change is applied in the right spot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/vitest-adapter/src/runner.ts`:
- Around line 177-194: The StoreController override leaks if createHeldScope
throws: in onBeforeRunSuite (and similarly in onBeforeTryTask) call
createHeldScope inside a try/catch immediately after setActiveCtx and, on any
error, call controller.restore() to undo the monkey-patch before rethrowing the
error; ensure you only set fileScopeMap (or the analogous task map) after
successful scope creation and that the catch restores the controller and
propagates the exception so cleanup always runs even if createHeldScope rejects.
- Line 133: The override of importFile is using `source: string` and casting to
any before calling super.importFile; change the method signature to async
importFile(filepath: string, source: VitestRunnerImportSource): Promise<unknown>
and remove the `as any` cast when calling super.importFile so that you call
super.importFile(filepath, source); also add the needed import for
VitestRunnerImportSource from 'vitest' or 'vitest/runners' at the top of the
file to satisfy the type.
---
Duplicate comments:
In `@core/vitest-adapter/src/runner.ts`:
- Around line 48-69: The current createHeldScope can hang because awaiting
readyPromise blocks forever if ctx.beginModuleScope rejects before the callback
calls scopeReady; change the waiting step to race the readiness against the
scope promise so a rejection short-circuits instead of hanging. Specifically, in
createHeldScope replace the single await readyPromise with awaiting
Promise.race([readyPromise, scopePromise]) (so scopePromise rejection propagates
and prevents the deadlock), keeping scopePromise, scopeReady, gate and endScope
logic unchanged.
- Around line 119-123: Instance-level collections fileScopeMap, taskScopeMap,
fileAppMap and the warned flag are never cleared between watch-mode reruns,
causing stale state; fix by clearing these at the start (or end) of each
test-run lifecycle: add logic to reset fileScopeMap.clear(),
taskScopeMap.clear(), fileAppMap.clear() and warned = false in the runner's run
lifecycle hook (e.g., at the beginning of the overridden
run()/handleRun()/onRunStart method inside TeggVitestRunner) so each rerun
starts with a fresh state.
In `@core/vitest-adapter/test/get_app_throw.test.ts`:
- Around line 17-19: The inline comment in the test is incorrect: update the
comment in get_app_throw.test.ts to state that the runner calls config.getApp()
during importFile (collect phase) rather than onBeforeRunSuite; locate the test
block containing getApp and getAppCalls and replace the misleading text ("The
runner calls getApp in onBeforeRunSuite") with a brief accurate note referencing
importFile/collect phase (or remove the comment entirely) so it correctly
reflects where getApp is invoked.
In `@core/vitest-adapter/test/get_store_restore.test.ts`:
- Around line 101-112: The assertions inside the afterAll closure (references:
getCtx, suiteCtx, testCtxList, afterEachCtxList) may throw and prevent cleanup
calls app.close() and mm.restore() from executing; wrap the assertion block in a
try/finally where the try contains all current asserts (including the forEach
and notStrictEqual checks) and the finally always awaits app.close() and
mm.restore() so cleanup runs regardless of assertion failures.
---
Nitpick comments:
In `@core/vitest-adapter/src/shared.ts`:
- Around line 40-44: defaultGetApp currently does a bare
require('egg-mock/bootstrap') which throws if egg-mock isn't installed; wrap the
require call in a try/catch inside defaultGetApp (catching MODULE_NOT_FOUND and
other errors) and return undefined on failure to match the Promise<EggMockApp |
undefined> signature and avoid unhandled rejections; ensure you still return
bootstrap.app when successful and keep the existing eslint comment if needed.
- Around line 46-54: The unguarded require in restoreEggMocksIfNeeded (eggMock /
mm) can throw if the optional dependency isn't installed; wrap the
require('egg-mock') in a try/catch inside restoreEggMocksIfNeeded, and on catch
simply return (or no-op) so execution continues safely; then keep the existing
mm?.restore check and await mm.restore() when present. Ensure you reference
eggMock and mm exactly as in the diff so the change is applied in the right
spot.
5870d78 to
26c0ff7
Compare
- Custom VitestTestRunner using AsyncLocalStorage.enterWith() for context injection - Each file suite gets its own mock context with held beginModuleScope - Each test gets isolated context via onBeforeTryTask, restored after test - Supports nested and concurrent ctxStorage.run() without conflicts - Retry-safe: releases previous scope before creating new one - Graceful degradation when getApp() fails Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rename core/vitest-adapter to core/vitest, package @eggjs/tegg-vitest-adapter to @eggjs/tegg-vitest - Fix createHeldScope hang when beginModuleScope rejects before callback by racing readyPromise against scopePromise - Fix watch-mode stale state by clearing fileAppMap per-file before re-collection and after suite completion - Fix onBeforeTryTask signature to match VitestTestRunner base class - Wrap afterAll assertions in try/finally to guarantee cleanup in tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add trailing comma in fixture_app.test.ts - Add comment to empty arrow function in get_app_throw.test.ts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace `source: string` with proper `VitestRunnerImportSource` type and remove unnecessary `as any` cast on super call. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
killagu-claw
force-pushed
the
feat/vitest-adapter
branch
from
cf68818 to
a1855ec
Compare
VitestRunnerImportSource is not re-exported from vitest/runners, use Parameters<VitestTestRunner['importFile']>[1] instead. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
eggjs#401) - Rename package from @eggjs/tegg-vitest-adapter to @eggjs/tegg-vitest - Use Promise.race for error handling in createHeldScope - Add watch mode support (clear stale state on re-collection) - Fix optional options parameter in onBeforeTryTask - Update debug env var to DEBUG_TEGG_VITEST cherry-pick from master c853090 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
eggjs#401) - Rename package from @eggjs/tegg-vitest-adapter to @eggjs/tegg-vitest - Use Promise.race for error handling in createHeldScope - Add watch mode support (clear stale state on re-collection) - Fix optional options parameter in onBeforeTryTask - Update debug env var to DEBUG_TEGG_VITEST cherry-pick from master c853090 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
## Summary - Rename `core/vitest-adapter` to `core/vitest` (package `@eggjs/tegg-vitest-adapter` → `@eggjs/tegg-vitest`) with code improvements from master (#401) - Upgrade CodeQL Action from v2 to v3 and actions/checkout from v3 to v4 (#405) ## Changes **core/vitest (renamed from core/vitest-adapter):** - Use `Promise.race` for error handling in `createHeldScope` to avoid hanging on init failure - Add watch mode support (clear stale state on re-collection) - Fix optional `options` parameter in `onBeforeTryTask` - Update debug env var to `DEBUG_TEGG_VITEST` **CodeQL workflow:** - `actions/checkout` v3 → v4 - `github/codeql-action/*` v2 → v3 ## Test plan - [ ] CI passes on next branch - [ ] vitest adapter tests pass with renamed package 🤖 Generated with [Claude Code](https://claude.ai/claude-code) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: killa <killa07071201@gmail.com> Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
2 participants
Adds new workspace package @eggjs/tegg-vitest-adapter to support Vitest runs with tegg/egg-mock-like context semantics.
Key points:
How to run:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores