Conversation
Runtime fixes: - guard callSite.getScriptHash() for Bun (tegg/core/common-util) - explicit removeHeader on 204/304/205 responses (packages/koa) - AbortSignal.timeout fallback for httpclient timeout (packages/egg) - pnpm virtual store path in framework resolution (packages/utils) - node:test mock API compat shim for Bun (tegg/core/test-util) CI: - add test-bun job to GitHub Actions (ubuntu + macos, Bun latest) Known issues: - urllib needs Bun adapter (fetch instead of undiciRequest) for MockAgent - Bun zlib stream decompression bug (body_parser gzip hang) - Bun HTTP behavior differences documented in test skips Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces Bun runtime support across the Egg.js framework by adding a CI job to test with Bun, creating a test mocking compatibility shim, conditionally skipping incompatible tests in Bun environments, and implementing Bun-specific timeout handling for HTTP clients. Core framework features like empty response header management and module resolution paths are also adjusted for cross-runtime compatibility. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| cache: 'pnpm' | ||
|
|
||
| - name: Set up Bun | ||
| uses: oven-sh/setup-bun@v2 |
Check warning
Code scanning / CodeQL
Unpinned tag for a non-immutable Action in workflow Medium
Deploying egg with
|
| Latest commit: |
756d81e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://200a7daa.egg-cci.pages.dev |
| Branch Preview URL: | https://feat-bun.egg-cci.pages.dev |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #5843 +/- ##
==========================================
- Coverage 85.62% 79.62% -6.01%
==========================================
Files 665 634 -31
Lines 13004 12480 -524
Branches 1495 1466 -29
==========================================
- Hits 11135 9937 -1198
- Misses 1745 2367 +622
- Partials 124 176 +52 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces compatibility for the Bun runtime across several packages. Key changes include Bun-specific workarounds for HTTP client timeouts, explicit header management in Koa for empty responses, and adjusted module resolution for pnpm virtual stores. Additionally, a mock_compat utility was added to polyfill node:test for Bun. Feedback was provided regarding a logic flaw in the mock_compat reset function, which should restore mocks in reverse order to correctly handle nested mocks.
| reset(): void { | ||
| for (const { obj, key, original } of originals) { | ||
| obj[key] = original; | ||
| } | ||
| originals.length = 0; | ||
| }, |
There was a problem hiding this comment.
The reset function has a flaw in its logic for restoring mocks. When the same method is mocked multiple times, the current for...of loop restores them in the order they were applied. This can result in the method being left in an intermediate mocked state, rather than its original state.
To ensure the original implementation is always restored, the mocks should be reverted in the reverse order of their application. Using Array.prototype.pop() in a while loop is a good way to achieve this LIFO (Last-In, First-Out) behavior.
reset(): void {
// Restore in reverse order to handle nested mocks correctly.
while (originals.length) {
const { obj, key, original } = originals.pop()!;
obj[key] = original;
}
},
Deploying egg-v3 with
|
| Latest commit: |
756d81e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://717feb35.egg-v3.pages.dev |
| Branch Preview URL: | https://feat-bun.egg-v3.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
packages/egg/test/app/middleware/body_parser.test.ts (1)
89-93: Use a specific Bun issue link for this skip condition.The generic tracker URL makes it hard to know when this case should be re-enabled; a concrete issue ID keeps the skip actionable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/test/app/middleware/body_parser.test.ts` around lines 89 - 93, Update the skip comment that references a generic Bun tracker by replacing the generic URL with the specific Bun issue/PR URL and issue number so the skip is actionable; locate the test using isBun and the it.skipIf call for "should 400 when GET with invalid body" and change the comment block above it to cite the concrete Bun issue ID/URL (e.g., https://github.com/oven-sh/bun/issues/NNN) and optionally add the issue title and date.tegg/core/test-util/src/mock_compat.ts (1)
30-35:reset()doesn't handle property descriptors for potentialgetter/settermocks.If
mock.getteris added, the currentreset()implementation using simple assignment (obj[key] = original) won't correctly restore property descriptors. Consider storing and restoring full descriptors:🔧 Proposed fix for robust reset
+ type OriginalEntry = + | { type: 'value'; obj: any; key: string | symbol; original: any } + | { type: 'descriptor'; obj: any; key: string | symbol; original: PropertyDescriptor | undefined }; + - const originals: Array<{ obj: any; key: string | symbol; original: any }> = []; + const originals: OriginalEntry[] = []; // In method(): - originals.push({ obj, key, original: obj[key] }); + originals.push({ type: 'value', obj, key, original: obj[key] }); // In reset(): reset(): void { - for (const { obj, key, original } of originals) { - obj[key] = original; + for (const entry of originals) { + if (entry.type === 'value') { + entry.obj[entry.key] = entry.original; + } else if (entry.original) { + Object.defineProperty(entry.obj, entry.key, entry.original); + } else { + delete entry.obj[entry.key]; + } } originals.length = 0; },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tegg/core/test-util/src/mock_compat.ts` around lines 30 - 35, The reset() routine currently restores mocks by simple assignment (obj[key] = original) which breaks getter/setter descriptors; change reset() to restore the full property descriptor using Object.defineProperty(obj, key, originalDescriptor) and ensure the stored "originals" entries record the full descriptor (from Object.getOwnPropertyDescriptor or a created descriptor) rather than a value; update any mock-recording code that pushes into originals to save the descriptor under a field like originalDescriptor so reset() can call Object.defineProperty with that descriptor and then clear originals..github/workflows/ci.yml (1)
239-240: Consider removing--bail 1to get full test visibility in CI.
--bail 1stops the test run after the first failure. While this saves CI time, it reduces visibility into the full scope of failures, making it harder to assess Bun compatibility progress. Given the PR objective mentions ~147 failing tests, seeing all failures per shard would be more informative.Alternatively, keep
--bail 1but add a comment explaining the rationale (e.g., to save CI minutes during active development).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 239 - 240, The CI step named "Run tests with Bun" currently passes the `--bail 1` flag to Vitest which stops on the first failure; either remove `--bail 1` from the command (`bun ./node_modules/vitest/vitest.mjs run ...`) so the full shard test output is shown, or if you must keep it, add an inline comment in the same workflow step explaining the rationale (e.g., to save CI minutes during active development) so reviewers understand why `--bail 1` is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 231-234: The workflow step named "Set up Bun" currently uses the
floating tag oven-sh/setup-bun@v2; replace that with a specific commit SHA to
pin the action (e.g., oven-sh/setup-bun@<commit-sha>) so the action cannot
change unexpectedly. Update the uses value for the "Set up Bun" step to
reference the exact commit hash from the oven-sh/setup-bun repository (grab the
latest stable commit SHA from the action's repo/releases) and commit that
change.
In `@packages/egg/src/lib/core/httpclient.ts`:
- Around line 60-62: The current computation of timeoutMs using
Math.min(...rawTimeout.filter(...)) can produce Infinity when the filtered array
is empty; update the logic around the rawTimeout -> timeoutMs computation so you
first collect the positive numeric values (e.g., const positives =
Array.isArray(rawTimeout) ? rawTimeout.filter((t): t is number => typeof t ===
'number' && t > 0) : null), then if positives is empty set timeoutMs to
undefined (or a safe default) instead of calling Math.min, otherwise set
timeoutMs = Math.min(...positives); ensure the rest of the code that uses
timeoutMs (e.g., AbortSignal.timeout(timeoutMs)) guards against
undefined/infinite values.
In `@packages/koa/src/application.ts`:
- Around line 271-275: The code unconditionally calls
res.removeHeader('Content-Type'/'Content-Length'/'Transfer-Encoding'), which can
throw ERR_HTTP_HEADERS_SENT if headers were already sent; update the
raw-response header cleanup to first check if (!res.headersSent) before calling
res.removeHeader for each header (mirror the existing guard used in the HEAD
method branch) so header removal only occurs when safe.
In `@tegg/core/runtime/test/EggObject.test.ts`:
- Line 4: The test fails on Bun because mock.getter is not implemented in the
Bun polyfill; update the mock_compat.ts polyfill by adding a mock.getter
signature to the MockContext interface and an implementation that defines a
getter on the target object (e.g., using Object.defineProperty to set the
property getter or wiring the underlying test framework’s getter-mocking
behavior) so calls like mock.getter(bar, 'foo', () => foo) work, or
alternatively conditionally skip the EggObject.test.ts block on Bun; ensure the
new symbol is named mock.getter and behaves consistently with the existing
mock.method/mock.fn semantics.
In `@tegg/core/test-util/package.json`:
- Line 32: The package export for "./mock_compat" is declared but the tsdown
build config only defines the "index" entry, so mock_compat.ts will not be
built; update tegg/core/test-util/tsdown.config.ts to add an entry for
"mock_compat" (pointing to "src/mock_compat.ts") alongside the existing "index"
entry so the build emits dist/mock_compat.js and matches the
publishConfig.exports; ensure the entry key name matches the export path
("mock_compat") and that tsdown's entries object includes both "index":
"src/index.ts" and "mock_compat": "src/mock_compat.ts".
In `@tegg/core/test-util/src/mock_compat.ts`:
- Around line 5-9: The MockContext interface and Bun polyfill lack mock.getter
and mock.setter causing runtime failures; add two methods to the MockContext
interface named getter(obj: object, key: PropertyKey, impl: () => any): void and
setter(obj: object, key: PropertyKey, impl: (v: any) => any): void, implement
corresponding functions in the Bun polyfill alongside method, fn, and reset (use
Object.getOwnPropertyDescriptor/Object.defineProperty to replace property
accessors and store original descriptors), and update reset() to restore
original property descriptors rather than only replacing values so
getters/setters are properly cleaned up (refer to MockContext, method, fn, and
reset to find the existing implementation to extend).
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 239-240: The CI step named "Run tests with Bun" currently passes
the `--bail 1` flag to Vitest which stops on the first failure; either remove
`--bail 1` from the command (`bun ./node_modules/vitest/vitest.mjs run ...`) so
the full shard test output is shown, or if you must keep it, add an inline
comment in the same workflow step explaining the rationale (e.g., to save CI
minutes during active development) so reviewers understand why `--bail 1` is
present.
In `@packages/egg/test/app/middleware/body_parser.test.ts`:
- Around line 89-93: Update the skip comment that references a generic Bun
tracker by replacing the generic URL with the specific Bun issue/PR URL and
issue number so the skip is actionable; locate the test using isBun and the
it.skipIf call for "should 400 when GET with invalid body" and change the
comment block above it to cite the concrete Bun issue ID/URL (e.g.,
https://github.com/oven-sh/bun/issues/NNN) and optionally add the issue title
and date.
In `@tegg/core/test-util/src/mock_compat.ts`:
- Around line 30-35: The reset() routine currently restores mocks by simple
assignment (obj[key] = original) which breaks getter/setter descriptors; change
reset() to restore the full property descriptor using Object.defineProperty(obj,
key, originalDescriptor) and ensure the stored "originals" entries record the
full descriptor (from Object.getOwnPropertyDescriptor or a created descriptor)
rather than a value; update any mock-recording code that pushes into originals
to save the descriptor under a field like originalDescriptor so reset() can call
Object.defineProperty with that descriptor and then clear originals.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88d26adf-15e4-48e6-b412-0b7e73ed7e40
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
.github/workflows/ci.yml.gitignorepackages/cluster/test/https.test.tspackages/cluster/test/options.test.tspackages/core/test/loader/file_loader.test.tspackages/core/test/loader/mixin/load_extend.test.tspackages/egg/src/lib/core/httpclient.tspackages/egg/test/agent.test.tspackages/egg/test/app/extend/response.test.tspackages/egg/test/app/middleware/body_parser.test.tspackages/egg/test/app/middleware/meta.test.tspackages/egg/test/cluster1/app_worker.test.tspackages/egg/test/lib/core/dnscache_httpclient.test.tspackages/egg/test/lib/core/messenger/local.test.tspackages/egg/test/lib/plugins/multipart.test.tspackages/koa/src/application.tspackages/koa/test/application/response.test.tspackages/koa/test/response/status.test.tspackages/supertest/test/supertest.test.tspackages/utils/src/framework.tspackages/utils/test/import.test.tstegg/core/aop-runtime/test/aop-runtime.test.tstegg/core/common-util/src/StackUtil.tstegg/core/dal-runtime/package.jsontegg/core/dal-runtime/test/DataSource.test.tstegg/core/eventbus-runtime/test/EventBus.test.tstegg/core/runtime/test/EggObject.test.tstegg/core/runtime/test/EggObjectUtil.test.tstegg/core/runtime/test/LoadUnitInstance.test.tstegg/core/runtime/test/QualifierLoadUnitInstance.test.tstegg/core/test-util/package.jsontegg/core/test-util/src/EggTestContext.tstegg/core/test-util/src/mock_compat.ts
| - name: Set up Bun | ||
| uses: oven-sh/setup-bun@v2 | ||
| with: | ||
| bun-version: ${{ matrix.bun }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest commit hash for oven-sh/setup-bun v2?
💡 Result:
The latest commit hash for oven-sh/setup-bun v2 is 0c5077e. This is the commit associated with the latest v2 release, v2.2.0, released on 2026-03-14. GitHub Actions typically reference releases by tag (e.g., oven-sh/setup-bun@v2), which points to this commit. The repository's main branch last push was on 2026-03-14T09:38:41Z, matching the release time, suggesting no later commits on main.
Citations:
Pin the oven-sh/setup-bun action to a commit hash for supply chain security.
Using @v2 instead of a pinned commit hash allows the action to be updated without your knowledge, which is a supply chain risk. Other actions in this workflow are properly pinned.
🔧 Proposed fix
- name: Set up Bun
- uses: oven-sh/setup-bun@v2
+ uses: oven-sh/setup-bun@0c5077e # v2.2.0
with:
bun-version: ${{ matrix.bun }}📝 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.
| - name: Set up Bun | |
| uses: oven-sh/setup-bun@v2 | |
| with: | |
| bun-version: ${{ matrix.bun }} | |
| - name: Set up Bun | |
| uses: oven-sh/setup-bun@0c5077e # v2.2.0 | |
| with: | |
| bun-version: ${{ matrix.bun }} |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 232-232: Unpinned tag for a non-immutable Action in workflow
Unpinned 3rd party Action 'CI' step Uses Step uses 'oven-sh/setup-bun' with ref 'v2', not a pinned commit hash
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 231 - 234, The workflow step named
"Set up Bun" currently uses the floating tag oven-sh/setup-bun@v2; replace that
with a specific commit SHA to pin the action (e.g.,
oven-sh/setup-bun@<commit-sha>) so the action cannot change unexpectedly. Update
the uses value for the "Set up Bun" step to reference the exact commit hash from
the oven-sh/setup-bun repository (grab the latest stable commit SHA from the
action's repo/releases) and commit that change.
| const timeoutMs = Array.isArray(rawTimeout) | ||
| ? Math.min(...rawTimeout.filter((t): t is number => typeof t === 'number' && t > 0)) | ||
| : rawTimeout; |
There was a problem hiding this comment.
Edge case: Math.min with empty spread returns Infinity.
If rawTimeout is an array containing only non-positive values (e.g., [0, -1]), the filter returns an empty array, and Math.min(...[]) evaluates to Infinity. Since Infinity > 0 is true, this would create AbortSignal.timeout(Infinity).
Consider adding a guard for this edge case:
Proposed fix
const timeoutMs = Array.isArray(rawTimeout)
- ? Math.min(...rawTimeout.filter((t): t is number => typeof t === 'number' && t > 0))
+ ? rawTimeout.filter((t): t is number => typeof t === 'number' && t > 0).reduce((a, b) => Math.min(a, b), 0)
: rawTimeout;
+ if (typeof timeoutMs === 'number' && timeoutMs > 0 && Number.isFinite(timeoutMs)) {
- if (typeof timeoutMs === 'number' && timeoutMs > 0) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/egg/src/lib/core/httpclient.ts` around lines 60 - 62, The current
computation of timeoutMs using Math.min(...rawTimeout.filter(...)) can produce
Infinity when the filtered array is empty; update the logic around the
rawTimeout -> timeoutMs computation so you first collect the positive numeric
values (e.g., const positives = Array.isArray(rawTimeout) ?
rawTimeout.filter((t): t is number => typeof t === 'number' && t > 0) : null),
then if positives is empty set timeoutMs to undefined (or a safe default)
instead of calling Math.min, otherwise set timeoutMs = Math.min(...positives);
ensure the rest of the code that uses timeoutMs (e.g.,
AbortSignal.timeout(timeoutMs)) guards against undefined/infinite values.
| // explicitly remove content headers from the raw response | ||
| // to ensure they are not sent (some runtimes like Bun don't strip them automatically) | ||
| res.removeHeader('Content-Type'); | ||
| res.removeHeader('Content-Length'); | ||
| res.removeHeader('Transfer-Encoding'); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js HTTP ServerResponse, what is the behavior of response.removeHeader(name) if headers have already been sent? Does it throw ERR_HTTP_HEADERS_SENT?
💡 Result:
Yes, in Node.js HTTP ServerResponse, calling response.removeHeader(name) after headers have already been sent throws Error [ERR_HTTP_HEADERS_SENT]: Cannot remove headers after they are sent to the client.
Citations:
- 1: https://stackoverflow.com/questions/74557285/error-cannot-remove-headers-after-they-are-sent-to-the-client
- 2: ERR_HTTP_HEADERS_SENT : Cannot remove headers after they are sent to the client directus/directus#14888
- 3: [BUG] Error [ERR_HTTP_HEADERS_SENT]: Cannot remove headers after they are sent to the client immich-app/immich#5744
- 4: https://github.com/nodejs/node/blob/main/lib/_http_outgoing.js
- 5: https://www.headless.app
🏁 Script executed:
cd packages/koa && wc -l src/application.tsRepository: eggjs/egg
Length of output: 75
🏁 Script executed:
cd packages/koa && sed -n '250,300p' src/application.tsRepository: eggjs/egg
Length of output: 1460
🏁 Script executed:
cd packages/koa && sed -n '200,250p' src/application.tsRepository: eggjs/egg
Length of output: 1633
Guard raw header removal with !res.headersSent check.
At lines 273-275, calling res.removeHeader() unconditionally will throw ERR_HTTP_HEADERS_SENT if headers were already sent during middleware execution, breaking response finalization in the empty-status path. The HEAD method branch (line 284) already demonstrates this pattern with a guard check.
Suggested fix
- res.removeHeader('Content-Type');
- res.removeHeader('Content-Length');
- res.removeHeader('Transfer-Encoding');
+ if (!res.headersSent) {
+ res.removeHeader('Content-Type');
+ res.removeHeader('Content-Length');
+ res.removeHeader('Transfer-Encoding');
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/koa/src/application.ts` around lines 271 - 275, The code
unconditionally calls
res.removeHeader('Content-Type'/'Content-Length'/'Transfer-Encoding'), which can
throw ERR_HTTP_HEADERS_SENT if headers were already sent; update the
raw-response header cleanup to first check if (!res.headersSent) before calling
res.removeHeader for each header (mirror the existing guard used in the HEAD
method branch) so header removal only occurs when safe.
| import { mock } from 'node:test'; | ||
|
|
||
| import { EggPrototypeFactory } from '@eggjs/metadata'; | ||
| import { mock } from '@eggjs/module-test-util/mock_compat'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for all mock.getter usages across tegg tests to assess scope of missing implementation
rg -n 'mock\.getter' tegg/Repository: eggjs/egg
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Locate and inspect the mock_compat.ts file
fd -t f 'mock_compat' tegg/Repository: eggjs/egg
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Read the mock_compat.ts file to verify implementation
cat -n tegg/core/test-util/src/mock_compat.tsRepository: eggjs/egg
Length of output: 1739
🏁 Script executed:
#!/bin/bash
# Read the EggObject.test.ts file around line 152 to see the context
sed -n '145,160p' tegg/core/runtime/test/EggObject.test.tsRepository: eggjs/egg
Length of output: 601
mock.getter is missing from the Bun polyfill — this test will fail on Bun.
The mock_compat.ts polyfill only implements method, fn, and reset. However, line 152 calls mock.getter(bar, 'foo', () => foo), which will throw TypeError: mock.getter is not a function on Bun.
Add mock.getter to the MockContext interface and polyfill implementation in tegg/core/test-util/src/mock_compat.ts, or skip this test block on Bun.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/runtime/test/EggObject.test.ts` at line 4, The test fails on Bun
because mock.getter is not implemented in the Bun polyfill; update the
mock_compat.ts polyfill by adding a mock.getter signature to the MockContext
interface and an implementation that defines a getter on the target object
(e.g., using Object.defineProperty to set the property getter or wiring the
underlying test framework’s getter-mocking behavior) so calls like
mock.getter(bar, 'foo', () => foo) work, or alternatively conditionally skip the
EggObject.test.ts block on Bun; ensure the new symbol is named mock.getter and
behaves consistently with the existing mock.method/mock.fn semantics.
| "types": "./dist/index.d.ts", | ||
| "exports": { | ||
| ".": "./src/index.ts", | ||
| "./mock_compat": "./src/mock_compat.ts", |
There was a problem hiding this comment.
Build configuration mismatch — mock_compat.ts won't be built for publish.
The publishConfig.exports expects ./dist/mock_compat.js, but tegg/core/test-util/tsdown.config.ts only defines index: 'src/index.ts' as an entry point. When building for publish, mock_compat.js will not be generated, causing module resolution failures for consumers.
Update tegg/core/test-util/tsdown.config.ts to include mock_compat:
🔧 Proposed fix
import { defineConfig } from 'tsdown';
export default defineConfig({
entry: {
index: 'src/index.ts',
+ mock_compat: 'src/mock_compat.ts',
},
});Also applies to: 38-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/test-util/package.json` at line 32, The package export for
"./mock_compat" is declared but the tsdown build config only defines the "index"
entry, so mock_compat.ts will not be built; update
tegg/core/test-util/tsdown.config.ts to add an entry for "mock_compat" (pointing
to "src/mock_compat.ts") alongside the existing "index" entry so the build emits
dist/mock_compat.js and matches the publishConfig.exports; ensure the entry key
name matches the export path ("mock_compat") and that tsdown's entries object
includes both "index": "src/index.ts" and "mock_compat": "src/mock_compat.ts".
| interface MockContext { | ||
| method<T extends object>(obj: T, key: keyof T, impl: (...args: any[]) => any): void; | ||
| fn(impl?: (...args: any[]) => any): (...args: any[]) => any; | ||
| reset(): void; | ||
| } |
There was a problem hiding this comment.
Missing mock.getter and mock.setter in interface and implementation.
The MockContext interface and Bun polyfill implementation are missing mock.getter (and mock.setter), which are used in tests like EggObject.test.ts line 152. This will cause runtime failures on Bun.
🔧 Proposed fix to add getter support
interface MockContext {
method<T extends object>(obj: T, key: keyof T, impl: (...args: any[]) => any): void;
fn(impl?: (...args: any[]) => any): (...args: any[]) => any;
+ getter<T extends object>(obj: T, key: keyof T, impl: () => any): void;
reset(): void;
}And in the Bun polyfill (around line 29):
fn(impl?: (...args: any[]) => any): (...args: any[]) => any {
const calls: any[][] = [];
const mockFn = (...args: any[]) => {
calls.push(args);
return impl?.(...args);
};
(mockFn as any).mock = { calls };
return mockFn;
},
+ getter<T extends object>(obj: T, key: keyof T, impl: () => any): void {
+ const descriptor = Object.getOwnPropertyDescriptor(obj, key) ||
+ Object.getOwnPropertyDescriptor(Object.getPrototypeOf(obj), key) || {};
+ originals.push({ obj, key, original: descriptor });
+ Object.defineProperty(obj, key, {
+ get: impl,
+ configurable: true,
+ });
+ },
reset(): void {Note: The reset logic would also need adjustment to handle property descriptors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tegg/core/test-util/src/mock_compat.ts` around lines 5 - 9, The MockContext
interface and Bun polyfill lack mock.getter and mock.setter causing runtime
failures; add two methods to the MockContext interface named getter(obj: object,
key: PropertyKey, impl: () => any): void and setter(obj: object, key:
PropertyKey, impl: (v: any) => any): void, implement corresponding functions in
the Bun polyfill alongside method, fn, and reset (use
Object.getOwnPropertyDescriptor/Object.defineProperty to replace property
accessors and store original descriptors), and update reset() to restore
original property descriptors rather than only replacing values so
getters/setters are properly cleaned up (refer to MockContext, method, fn, and
reset to find the existing implementation to extend).
There was a problem hiding this comment.
Pull request overview
This PR is a WIP effort to improve Bun runtime compatibility across the Egg.js monorepo by adding targeted runtime guards/shims, adjusting framework/module resolution, and updating tests/CI to account for Bun behavior differences.
Changes:
- Add a
node:testmock compatibility shim and migrate tegg tests/utilities to use it. - Patch runtime differences (e.g., V8 callsite API guard, Koa empty-status header stripping, urllib/httpclient timeout enforcement under Bun).
- Add Bun CI coverage and update/skip tests where Bun currently differs or is blocked by upstream issues.
Reviewed changes
Copilot reviewed 32 out of 34 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tegg/core/test-util/src/mock_compat.ts |
Adds Bun polyfill for node:test mock API. |
tegg/core/test-util/src/EggTestContext.ts |
Switches to the new mock shim. |
tegg/core/test-util/package.json |
Exposes ./mock_compat via package exports (src + dist). |
tegg/core/runtime/test/QualifierLoadUnitInstance.test.ts |
Uses shimmed mock import. |
tegg/core/runtime/test/LoadUnitInstance.test.ts |
Uses shimmed mock import. |
tegg/core/runtime/test/EggObjectUtil.test.ts |
Uses shimmed mock import. |
tegg/core/runtime/test/EggObject.test.ts |
Uses shimmed mock import. |
tegg/core/eventbus-runtime/test/EventBus.test.ts |
Uses shimmed mock import. |
tegg/core/dal-runtime/test/DataSource.test.ts |
Uses shimmed mock import (and relies on call tracking). |
tegg/core/dal-runtime/package.json |
Adds @eggjs/module-test-util as a devDependency. |
tegg/core/common-util/src/StackUtil.ts |
Guards missing callSite.getScriptHash() on Bun. |
tegg/core/aop-runtime/test/aop-runtime.test.ts |
Uses shimmed mock import. |
pnpm-lock.yaml |
Lockfile update for workspace/importer changes. |
packages/utils/test/import.test.ts |
Loosens error-message matching for Bun/Node variance. |
packages/utils/src/framework.ts |
Adds pnpm virtual-store lookup to framework resolution. |
packages/supertest/test/supertest.test.ts |
Skips/adjusts assertions for Bun HTTP/HTTPS behavior differences. |
packages/koa/test/response/status.test.ts |
Adjusts empty-status header expectations under Bun. |
packages/koa/test/application/response.test.ts |
Skips http2-specific mutation test under Bun. |
packages/koa/src/application.ts |
Explicitly removes content headers on empty status responses. |
packages/egg/test/lib/plugins/multipart.test.ts |
Skips multipart test suite on Bun. |
packages/egg/test/lib/core/messenger/local.test.ts |
Converts callback-style assertion to async/promise style. |
packages/egg/test/lib/core/dnscache_httpclient.test.ts |
Skips dnscache httpclient tests on Bun. |
packages/egg/test/cluster1/app_worker.test.ts |
Skips a request-packet test on Bun. |
packages/egg/test/app/middleware/meta.test.ts |
Skips keep-alive header expectation on Bun. |
packages/egg/test/app/middleware/body_parser.test.ts |
Skips invalid-gzip test on Bun (hang issue). |
packages/egg/test/app/extend/response.test.ts |
Skips rawHeaders case-sensitivity assertion on Bun. |
packages/egg/test/agent.test.ts |
Skips IPC/log timing-dependent tests on Bun. |
packages/egg/src/lib/core/httpclient.ts |
Adds Bun-specific timeout enforcement via AbortSignal.timeout. |
packages/core/test/loader/mixin/load_extend.test.ts |
Loosens module/package error matching for Bun variance. |
packages/core/test/loader/file_loader.test.ts |
Loosens class-constructor error matching for Bun variance. |
packages/cluster/test/options.test.ts |
Adapts assertions/paths and adds pnpm virtual-store expectation. |
packages/cluster/test/https.test.ts |
Skips HTTPS suite on Bun due to certificate handling differences. |
.gitignore |
Ignores Bun runtime cache directory. |
.github/workflows/ci.yml |
Adds test-bun job (ubuntu + macOS, sharded) and gates merge on it. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| interface MockContext { | ||
| method<T extends object>(obj: T, key: keyof T, impl: (...args: any[]) => any): void; | ||
| fn(impl?: (...args: any[]) => any): (...args: any[]) => any; | ||
| reset(): void; | ||
| } | ||
|
|
||
| let mockCompat: MockContext; | ||
|
|
||
| if (process.versions.bun) { | ||
| const originals: Array<{ obj: any; key: string | symbol; original: any }> = []; | ||
|
|
||
| mockCompat = { | ||
| method<T extends object>(obj: T, key: keyof T, impl: (...args: any[]) => any): void { | ||
| originals.push({ obj, key, original: obj[key] }); | ||
| (obj as any)[key] = impl; | ||
| }, | ||
| fn(impl?: (...args: any[]) => any): (...args: any[]) => any { | ||
| const calls: any[][] = []; | ||
| const mockFn = (...args: any[]) => { | ||
| calls.push(args); | ||
| return impl?.(...args); | ||
| }; | ||
| (mockFn as any).mock = { calls }; | ||
| return mockFn; | ||
| }, | ||
| reset(): void { | ||
| for (const { obj, key, original } of originals) { | ||
| obj[key] = original; | ||
| } | ||
| originals.length = 0; | ||
| }, |
There was a problem hiding this comment.
In the Bun polyfill branch, mockCompat.method() currently returns void and only assigns obj[key] = impl. Several tests rely on the real node:test behavior where mock.method() returns a tracker with .mock.callCount() (e.g. const tracker = mock.method(...); tracker.mock.callCount()). This will throw under Bun. Also, reset() replays originals in insertion order, which fails to fully restore when the same method is mocked multiple times in one test (the last restore wins and can leave the method mocked). Update the polyfill so method() returns a tracker compatible with existing test usage and ensure reset restores the true original (e.g. restore in reverse order and/or dedupe per obj+key).
| const rawTimeout = options.timeout ?? this.#app.config.httpclient?.request?.timeout; | ||
| // urllib supports timeout as number or [connectTimeout, responseTimeout]. | ||
| // Use the shorter (connect) timeout for AbortSignal — if headers haven't | ||
| // arrived within connectTimeout the request should fail, matching Node's | ||
| // headersTimeout semantics as closely as possible. | ||
| const timeoutMs = Array.isArray(rawTimeout) | ||
| ? Math.min(...rawTimeout.filter((t): t is number => typeof t === 'number' && t > 0)) | ||
| : rawTimeout; | ||
| if (typeof timeoutMs === 'number' && timeoutMs > 0) { | ||
| options.signal = AbortSignal.timeout(timeoutMs); | ||
| try { | ||
| return await super.request<T>(url, options); |
There was a problem hiding this comment.
When rawTimeout is an array, Math.min(...rawTimeout.filter(...)) will produce Infinity if the filtered array is empty (e.g. [0, 0], [], or all non-positive values). That would pass the typeof timeoutMs === 'number' && timeoutMs > 0 check and create AbortSignal.timeout(Infinity), which can behave unexpectedly (clamping or never timing out depending on runtime). Consider explicitly handling the “no valid positive timeout” case by checking the filtered array length before calling Math.min().
| for (const dir of [initCwd, baseDir]) { | ||
| const pnpmVirtualDir = path.join(dir, 'node_modules/.pnpm/node_modules'); | ||
| if (existsSync(pnpmVirtualDir)) { | ||
| moduleDirs.add(pnpmVirtualDir); |
There was a problem hiding this comment.
The added pnpm virtual-store lookup is described as a Bun-specific workaround, but it currently runs unconditionally for all runtimes. If this lookup is only needed for Bun, consider guarding it with process.versions.bun to avoid subtly changing framework resolution/error output on Node.js. If it’s intentionally runtime-agnostic, the comment should be updated to reflect that and explain why it’s safe/desired on Node as well.
| for (const dir of [initCwd, baseDir]) { | |
| const pnpmVirtualDir = path.join(dir, 'node_modules/.pnpm/node_modules'); | |
| if (existsSync(pnpmVirtualDir)) { | |
| moduleDirs.add(pnpmVirtualDir); | |
| if ('bun' in process.versions) { | |
| for (const dir of [initCwd, baseDir]) { | |
| const pnpmVirtualDir = path.join(dir, 'node_modules/.pnpm/node_modules'); | |
| if (existsSync(pnpmVirtualDir)) { | |
| moduleDirs.add(pnpmVirtualDir); | |
| } |
Summary
Add Bun runtime support for the Egg.js monorepo. Current status: 94.9% tests passing (2771/2918 non-skip) under Bun 1.3.5.
Runtime fixes
callSite.getScriptHash()guard — Bun doesn't have this V8 API; blocked all tegg-dependent testsremoveHeader— Bun HTTP server doesn't strip content headers on empty status; koa_respondnow explicitly removes themAbortSignal.timeoutfallback — Bun's undici doesn't honorheadersTimeout/bodyTimeout; egg HttpClient addsAbortSignal.timeoutas fallback, supportstimeout: number | number[].pnpm/node_modulesas search pathnode:testmock compat shim — Bun'snode:testmock is incomplete; createdmock_compat.tswith method/fn/reset supportCI
test-bunjob: ubuntu + macos, Bun latest, 3 shards, Redis + MySQLKnown issues (blocked on urllib)
import 'undici'with a built-in stub that has an empty MockAgent. Need urllib to usefetch()instead ofundiciRequest()on Bun so that HTTP mocking can workTest results (local, Bun 1.3.5)
Most failures are mock_httpclient (~50) and view-nunjucks (~20), both blocked on the urllib undici issue.
Test plan
test-bunjob passes on ubuntu and macos🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Chores
Bug Fixes