test: use fresh request(app) inside describe('Logout') blocks (#3472)#3490
Conversation
The shared supertest `agent` is cookie-stateful per Express app. When a
prior `describe` block signs up a user, then deletes it in afterEach
(`UserService.remove(user)`), the agent keeps the now-stale `TOKEN`
cookie. Subsequent describe blocks that reuse the same agent can then
hit 401s / socket hang ups on the first auth-middleware call — the
symptom reported downstream on trawl_node `historys.integration.tests.js`.
Canonical pattern (already used in `tasks.integration.tests.js`):
describe('Logout', () => {
test(..., async () => {
await request(app).get('/api/...').expect(401);
});
});
Applied to:
- modules/home/tests/home.integration.tests.js — Logout block now uses
`request(app)` for every public endpoint call; admin/health cases
still use explicit `set('Cookie', TOKEN=...)` with JWTs.
- modules/users/tests/user.account.integration.tests.js — Logout block
switched to `request(app)` for all 6 unauthenticated assertions.
Both files now expose `app = init.app` in `beforeAll` so later blocks
can build fresh requests without relying on agent cookie state.
`tasks.integration.tests.js` was already correct — kept as the
canonical source-of-truth pattern for downstream propagation.
Fixes #3472
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 43 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
There was a problem hiding this comment.
Pull request overview
Updates integration tests to prevent stateful Supertest agent cookies from leaking across describe('Logout') blocks, eliminating a known source of full-suite flakiness (#3472) in devkit tests that propagate downstream.
Changes:
- Introduce an
appreference captured frombootstrap()so tests can use freshrequest(app)instances. - Replace
agentusage withrequest(app)insidedescribe('Logout')blocks in the affected test suites. - Add inline comments documenting the “fresh request per test” pattern and rationale.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| modules/users/tests/user.account.integration.tests.js | Captures app from bootstrap and switches Logout-block tests to request(app) to avoid stale auth cookies. |
| modules/home/tests/home.integration.tests.js | Captures app from bootstrap and switches public/Logout-block tests to request(app) while keeping explicit JWT cookie headers where needed. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3490 +/- ##
=======================================
Coverage 85.85% 85.85%
=======================================
Files 115 115
Lines 2919 2919
Branches 809 809
=======================================
Hits 2506 2506
Misses 327 327
Partials 86 86 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
This PR cannot be merged in its current state as it contains no code changes. The stated objective is to resolve intermittent 401 errors in CI by moving from stateful Supertest agents to fresh request instances in 'Logout' blocks. However, the files intended for modification—modules/home/tests/home.integration.tests.js and modules/users/tests/user.account.integration.tests.js—remain untouched in the diff. Please ensure all commits are pushed to the branch before proceeding.
About this PR
- The PR does not contain any code changes. While the intent to prevent state leakage in integration tests is documented, the implementation in
modules/home/tests/home.integration.tests.jsandmodules/users/tests/user.account.integration.tests.jsis missing.
Test suggestions
- Verify 'Logout' block in home.integration.tests.js uses fresh request(app) instead of shared agent
- Verify 'Logout' block in user.account.integration.tests.js uses fresh request(app) instead of shared agent
- Verify that authentication-dependent tests within those blocks use explicit Cookie headers
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify 'Logout' block in home.integration.tests.js uses fresh request(app) instead of shared agent
2. Verify 'Logout' block in user.account.integration.tests.js uses fresh request(app) instead of shared agent
3. Verify that authentication-dependent tests within those blocks use explicit Cookie headers
🗒️ Improve review quality by adding custom instructions
Up to standards ✅🟢 Issues
|
Summary
describe('Logout')blocks, replace the shared stateful supertestagentwith a freshrequest(app)per test so no cookie state leaks into later describe blocks.modules/home/tests/home.integration.tests.jsandmodules/users/tests/user.account.integration.tests.js.tasks.integration.tests.jsalready followed the correct pattern — kept as canonical source-of-truth.Why
The shared
agentis cookie-stateful per Express app. When a priordescribeblock signs up a user and then deletes it inafterEach(UserService.remove(user)), the agent keeps a staleTOKENcookie. Subsequent describe blocks that reuse the same agent can then hit 401s or socket hang ups on the first auth-middleware call — the exact symptom the downstream trawl_nodehistorys.integration.tests.js:199was reporting.Because jest drops the DB once via globalSetup but doesn't reset in-memory agent state, the failure only surfaces in full-suite runs (not when the test file is run in isolation). Fresh Mongo containers per CI run mask it; local dev sees it as flaky noise.
Pattern applied
Auth-required sub-tests inside
Logoutblocks (e.g. admin health in home tests) continue to use explicitset('Cookie', 'TOKEN=\${jwt}')from JWT tokens minted at the suite level — no reliance on agent cookies.Propagation
This is the devkit source-of-truth.
/update-all-projectswill propagate to the 6+ downstream test files that inherit this structure in:comes_node,trawl_node,montaine_node,ism_node,pierreb_nodeTest plan
npm run lint— cleannpm run test:unit— 508 tests passing (unchanged)user.account.integration+user.admin.integrationaround stale-user signups is unrelated — reproduces on master without these changes)Closes #3472