test: stabilize cluster unix socket fixture#5907
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughTests now generate ephemeral Unix socket paths using the OS temp directory, Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5907 +/- ##
==========================================
- Coverage 85.04% 85.03% -0.02%
==========================================
Files 665 665
Lines 19100 19100
Branches 3716 3716
==========================================
- Hits 16244 16242 -2
- Misses 2463 2465 +2
Partials 393 393 ☔ 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 updates the test suite to use dynamic Unix domain socket paths in the system's temporary directory, replacing hardcoded paths within the application directory. Key changes include generating a socket path using the process ID in app_worker.test.ts, ensuring the socket file is removed before each test, and passing the path to the application via the EGG_APP_LISTEN_PATH_SOCKET environment variable. Feedback suggests appending a random string to the socket filename to further mitigate collision risks in multi-threaded test environments.
Deploying egg with
|
| Latest commit: |
25fd34c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://05013586.egg-cci.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-a04bf3d7.egg-cci.pages.dev |
Deploying egg-v3 with
|
| Latest commit: |
25fd34c
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://edff6714.egg-v3.pages.dev |
| Branch Preview URL: | https://agent-egg-dev-a04bf3d7.egg-v3.pages.dev |
There was a problem hiding this comment.
Pull request overview
Stabilizes the app-listen-path Unix socket fixture used by @eggjs/cluster tests by allowing the socket path to be injected via env and by cleaning up stale socket files between runs.
Changes:
- Allow the fixture’s
cluster.listen.pathto be overridden viaEGG_APP_LISTEN_PATH_SOCKET. - Update the
listen configtest to use a temp socket path and remove stale socket files before/after each test run.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/cluster/test/fixtures/apps/app-listen-path/config/config.default.js | Adds env override for the Unix socket listen path to support shorter / temp socket locations. |
| packages/cluster/test/app_worker.test.ts | Switches the socket test to use a tmp socket path via env and adds cleanup logic for stale sockets. |
Comments suppressed due to low confidence (2)
packages/cluster/test/app_worker.test.ts:217
afterEachclosesappunconditionally and also callsmm.restore(), butappis a sharedletthat can be undefined if the test fails before assignment. This can throw in cleanup and mask the original failure. Also, this duplicates the file-levelafterEach(() => app && app.close())/afterEach(mm.restore)hooks, leading to double-close/double-restore.
Consider guarding (if (app) await app.close()), and avoid calling mm.restore() here (or otherwise ensure close/restore only happens once, ideally with a single awaited close).
afterEach(async () => {
await app.close();
await mm.restore();
});
packages/cluster/test/app_worker.test.ts:218
rm(sockFile, { recursive: true })is unnecessary for a socket file and widens the blast radius ifsockFileever points at a directory (e.g., via an unexpected value). Usingrm(sockFile, { force: true })(norecursive) is safer and still removes stale socket files.
mm.env('default');
await rm(sockFile, { force: true, recursive: true });
});
afterEach(async () => {
await app.close();
await mm.restore();
});
afterEach(() => rm(sockFile, { force: true, recursive: true }));
killagu
force-pushed
the
agent/egg-dev/a04bf3d7
branch
from
c681bc0 to
25fd34c
Compare
Summary
Verification
Notes
env HOME=/tmp EGG_HOME=/tmp CI=true corepack pnpm exec ut run ciexits before tests becauseut run clean-distcalls a missingcleanscript.env HOME=/tmp EGG_HOME=/tmp CI=true corepack pnpm --filter @eggjs/cluster run test -- --coveragereaches unrelated local runner failures from direct loading of tegg decorator syntax integg/core/background-task/src/BackgroundTaskHelper.ts.Summary by CodeRabbit
New Features
Tests