🧪 test: Add test for Arcjet rate limit error response - #11
Conversation
Adds test coverage to `src/arcjet.js` ensuring that the `securityMiddleware` correctly handles rate-limited requests, regular denied requests, allowed requests, and internally thrown errors by mocking `@arcjet/node` responses using the `node:test` framework. Also updates `package.json` adding the `--experimental-test-module-mocks` flag to the `test` script to properly resolve the mocks. Co-authored-by: somyaknotfound <118343482+somyaknotfound@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Warning Rate limit exceeded
⌛ 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: defaults Review profile: CHILL 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
Adds unit test coverage for the Arcjet securityMiddleware (src/arcjet.js) to validate expected HTTP responses for allow/deny/rate-limit and error cases, and wires up an npm test command to run the suite.
Changes:
- Added
node:test-based unit tests forsecurityMiddlewarecovering allow, rate-limit (429), deny (403), and protect-throw (503) behaviors. - Added an
npm testscript using Node’s test runner with experimental ESM module mocking enabled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/arcjet.test.js | New unit tests for Arcjet middleware responses using node:test + mock.module() |
| package.json | Adds npm test script invoking node --test with --experimental-test-module-mocks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| test('securityMiddleware allows request when Arcjet allows', async () => { | ||
| mockArcjetInstance.protect.mock.mockImplementationOnce(async () => ({ | ||
| isDenied: () => false | ||
| })); |
There was a problem hiding this comment.
These tests share a single mockArcjetInstance.protect mock and use mockImplementationOnce(). Since Node’s test runner can execute tests concurrently, the one-time implementations can be consumed by a different test than intended, making the suite flaky. Consider isolating the mock per test (create a new Arcjet instance/middleware per test) or disable concurrency for this file/tests and reset the mock between tests.
| // Suppress console.error for this specific test | ||
| const originalConsoleError = console.error; | ||
| console.error = () => {}; | ||
|
|
||
| t.after(() => { | ||
| console.error = originalConsoleError; | ||
| }); | ||
|
|
There was a problem hiding this comment.
Overriding console.error globally can interfere with other tests (especially if tests run concurrently) and may not be restored if the process exits early. Prefer using the test runner’s mocking utilities (e.g., mock.method(console, 'error', ...) or t.mock.method(...)) so the stub is scoped to the test and automatically restored.
| // Suppress console.error for this specific test | |
| const originalConsoleError = console.error; | |
| console.error = () => {}; | |
| t.after(() => { | |
| console.error = originalConsoleError; | |
| }); | |
| // Suppress console.error for this specific test using scoped mock | |
| mock.method(console, 'error', () => {}); |
| "db:demo": "node src/db-demo.js", | ||
| "test": "node --experimental-test-module-mocks --test" |
There was a problem hiding this comment.
Because the test suite relies on module mocking (mock.module(...)), it requires running Node with --experimental-test-module-mocks. Adding this to the default npm test script may be fine, but consider documenting the required Node version/flags (e.g., via an engines.node constraint or README note) to prevent CI/local failures on older Node versions.
🎯 What: The testing gap addressed is the lack of unit tests for the
securityMiddlewareinsrc/arcjet.js, specifically the testing of its error responses including when a rate limit is hit.📊 Coverage: Scenarios covered:
next()successfully.arcjet.protect()throws return a 503 status code with a Service Unavailable message.✨ Result: Enhanced the testing coverage specifically for our security middleware to handle these varying responses correctly and confidently, adding easy execution from the CLI via
npm test.PR created automatically by Jules for task 7414135327331669581 started by @somyaknotfound