fix(ci): stabilize mock server emit checks#5976
Conversation
@eggjs/mock created the http server and emitted the `server` event inside
`createServer()`, which runs BEFORE `app.ready()`. But egg core registers its
`once('server', server => this.onServer(server))` listener inside
`Application.load()` (during `app.ready()`), and `onServer` reads loaded config.
So the event was emitted before the listener existed (and before config was
loaded), and `onServer` — which wires up clientError logging, graceful shutdown,
server timeout and websocket support — never ran in single-process mock mode.
Move the `server` emit out of `createServer()` (it now only creates & caches the
server, so `app.server` stays available during boot as before) and emit it once
after `await app.ready()` in both the single and parallel app workers. This
mirrors @eggjs/cluster, which also emits `server` after the app is ready, and
keeps the event emitted exactly once.
Added a regression test asserting `app.server` has a `clientError` listener
after ready.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
`onServer` now actually runs after the mock `server` re-emit, which wired up `graceful()` in every app booted via `@eggjs/mock`. graceful installs a process-wide `uncaughtException` handler that takes over process shutdown; in egg-bin's per-test forked child processes this prevents a clean exit. On Windows (no real POSIX signals, slower pipe/socket teardown) the child then hangs until a kill timeout, so every `Test bin (windows)` case times out at 60s. Skip graceful when `env === 'unittest'`; clientError logging and serverTimeout still apply. graceful is a production resilience mechanism and is undesirable in a test runner regardless. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The vitest 4.1.8 pin was an attempt to fix the Windows `Test bin` timeouts, but the real cause was graceful running in unittest (fixed in the prior commit). Restore the `^4.0.15` catalog range for vitest and @vitest/coverage-v8. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This PR should not modify pnpm-workspace.yaml. Restore @oclif/core to ^4.2.0 so the file matches next with no diff. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
|
Dependency limit exceeded — report not shown. This pull request scan exceeded the 10,000-dependency limit applied to this scan, so the results are incomplete and may be inaccurate. To avoid reporting false positives, Socket has not posted a report. Upgrade your plan to raise the dependency limit and get complete reports, or view the partial scan in the dashboard. Socket is always free for open source. If this is a non-commercial open source project, contact us to request a free Team account. |
![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 removes the environment check that skipped graceful shutdown handling during unit tests in Application.onServer, and pins the @oclif/core dependency version. Feedback is provided to add a defensive check in the graceful error handler to ensure the thrown error is a non-null object before accessing or modifying its properties, preventing potential runtime TypeErrors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| error: (err: Error, throwErrorCount: number) => { | ||
| const originMessage = err.message; | ||
| if (originMessage) { | ||
| // shouldjs will override error property but only getter | ||
| // https://github.com/shouldjs/should.js/blob/889e22ebf19a06bc2747d24cf34b25cc00b37464/lib/assertion-error.js#L26 | ||
| Object.defineProperty(err, 'message', { | ||
| get() { | ||
| return `${originMessage} (uncaughtException throw ${throwErrorCount} times on pid: ${process.pid})`; | ||
| }, | ||
| configurable: true, | ||
| enumerable: false, | ||
| }); | ||
| } | ||
| this.coreLogger.error(err); | ||
| }, |
There was a problem hiding this comment.
In JavaScript/TypeScript, any value (including primitives like strings, numbers, or null/undefined) can be thrown as an exception. If a non-object or null value is passed to the error callback, accessing err.message or calling Object.defineProperty(err, ...) will throw a runtime TypeError, crashing the uncaught exception handler itself. Adding a defensive check to ensure err is a non-null object before accessing its properties or modifying them prevents this secondary crash.
error: (err: Error, throwErrorCount: number) => {
if (err && typeof err === 'object') {
const originMessage = err.message;
if (originMessage) {
// shouldjs will override error property but only getter
// https://github.com/shouldjs/should.js/blob/889e22ebf19a06bc2747d24cf34b25cc00b37464/lib/assertion-error.js#L26
Object.defineProperty(err, 'message', {
get() {
return `${originMessage} (uncaughtException throw ${throwErrorCount} times on pid: ${process.pid})`;
},
configurable: true,
enumerable: false,
});
}
}
this.coreLogger.error(err);
},
elrrrrrrr
force-pushed
the
codex/pr-5969-ci
branch
from
ba694a6 to
0bd329c
Compare
Deploying egg-v3 with
|
| Latest commit: |
1c6db71
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://36984e1e.egg-v3.pages.dev |
| Branch Preview URL: | https://codex-pr-5969-ci.egg-v3.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5976 +/- ##
==========================================
+ Coverage 85.50% 85.82% +0.31%
==========================================
Files 669 669
Lines 19825 19826 +1
Branches 3917 3917
==========================================
+ Hits 16952 17016 +64
+ Misses 2481 2428 -53
+ Partials 392 382 -10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Deploying egg with
|
| Latest commit: |
1c6db71
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d8986ef7.egg-cci.pages.dev |
| Branch Preview URL: | https://codex-pr-5969-ci.egg-cci.pages.dev |
elrrrrrrr
force-pushed
the
codex/pr-5969-ci
branch
from
0bd329c to
1c6db71
Compare
elrrrrrrr
changed the title
1 participant
Summary
createServer()afterapp.ready()so there is no defensive uncovered branchsetCustomLoader(app)after config is available@oclif/coreto4.11.4, matching the latest greennextWindows bin run and avoiding the4.11.7fork timeout regressionserveremit pathCI investigation
Test bin (windows-latest, 24)timed out while forkedegg-binCLI child processes waited to exit, including--help/--versioncases that do not boot an Egg app.@oclif/core@4.11.7; latest greennextrun used@oclif/core@4.11.4.@oclif/core@4.11.4had the Windows bin job green, while pinning Vitest alone did not fix the hang.packages/egg/src/lib/application.tsunittest/graceful workaround is removed from the final diff; it was not on theegg-bin --help/--versionpath and caused Codecov patch risk.Local verification
utx utoo@1.0.32 install --from pnpm->8 workspaces, 1 overrides, 214 catalogs@oclif/core@4.11.4,rolldown-vite@7.3.1,vitest@4.1.9,@vitest/coverage-v8@4.1.9utx utoo@1.0.32 run build -- --workspace ./tools/egg-binutx utoo@1.0.32 run ci --workspace @eggjs/binutx utoo@1.0.32 run test -- plugins/mock/test/app.test.tsutx utoo@1.0.32 run typecheck --workspace @eggjs/mockRemote verification
Draft PR #5976 is green on commit
1c6db712e3fec02ed27fd2e3d15dabfe3a19d499.27836491515: passed, includingTest bin (windows-latest, 24)in 3m23s and all normal test matrix jobs27836491493: passed (examplesandcnpmcore)codecov/patchpassed andcodecov/projectpassed