test: add health endpoint smoke test and make redis test-safe - #26
test: add health endpoint smoke test and make redis test-safe#26alok-kumar0421 wants to merge 2 commits into
Conversation
|
@alok-kumar0421 is attempting to deploy a commit to the Yash Pouranik's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughAdds test-mode support by exporting a dummy Redis client when NODE_ENV === "test", skips environment variable validation during tests, and introduces tests and mocks including a new health-check test and enhanced test setup/mocks for auth tests. Changes
Sequence Diagram(s)(omitted — changes are small and do not introduce a multi-component sequential flow requiring visualization) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
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 unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@backend/config/redis.js`:
- Around line 27-31: The debug line that prints Object.keys(process.env) inside
the REDIS_URL existence check should be removed or replaced because it exposes
sensitive environment variable names; in the REDIS_URL check (the top-level
block that throws "REDIS_URL is not defined in .env") remove the
console.log("DEBUG: ENV KEYS:", Object.keys(process.env)) call and, if you need
runtime context, replace it with a non-sensitive message (e.g., log only
NODE_ENV or a boolean indicating missing vars) so that no environment keys or
secrets are ever printed to logs.
In `@backend/tests/health.test.js`:
- Around line 1-11: The test leaves the server running because app.js calls
app.listen on require; add an afterAll hook that closes the server to avoid port
conflicts. Change the test to require the server instance (e.g., const server =
require("../app");) or adjust app.js to export the server returned by app.listen
(keep symbol name server), then add afterAll(async () => server.close()); (or
call done/return a promise) to properly close the server after tests; reference
afterAll, app, server, and app.listen when making the change.
🧹 Nitpick comments (1)
backend/config/redis.js (1)
34-44: Retry strategy allows the first 3 retries but the delay is computed before the bail-out check.Minor:
delayis computed on every call including whentimes > 3(where it's discarded). This is harmless but the logic reads more clearly if the bail-out check comes first.Also,
maxRetriesPerRequest: nullmeans individual commands will retry indefinitely if Redis is down — confirm this is intentional (e.g., for BullMQ compatibility).
| const request = require("supertest"); | ||
| const app = require("../app"); | ||
|
|
||
| describe("Health Check API", () => { | ||
| it("should return 200 and success message", async () => { | ||
| const res = await request(app).get("/"); | ||
|
|
||
| expect(res.statusCode).toBe(200); | ||
| expect(res.body.status).toBe("success"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find all test files in backend/tests
fd '.test.js' --base-directory backend/tests 2>/dev/null || find backend/tests -name '*.test.js' 2>/dev/null
# Check app.js for app.listen and exports
echo "=== Checking app.js for listen/exports ==="
rg -n 'app\.listen|module\.exports\s*=|exports\s*=' backend/app.js
# Look for auth-related test files
echo "=== Looking for auth test files ==="
fd -e js --base-directory backend/tests | grep -i authRepository: yash-pouranik/urBackend
Length of output: 260
🏁 Script executed:
# Check the context around app.listen() call
sed -n '125,145p' backend/app.js
# Check if there's any conditional wrapping around listen
sed -n '120,170p' backend/app.js | head -50
# Read auth.test.js to see what it tests
cat backend/tests/auth.test.jsRepository: yash-pouranik/urBackend
Length of output: 5194
Add afterAll hook to close the server and prevent port conflicts.
The health check test is correct, but it leaves the server running. Since app.js unconditionally calls app.listen(PORT) on require (line 132), running multiple test files in the same suite can cause port binding conflicts or open-handle warnings in Jest.
Add an afterAll hook to close the server:
Required addition
const request = require("supertest");
const app = require("../app");
describe("Health Check API", () => {
+ let server;
+ afterAll((done) => {
+ if (server) server.close(done);
+ else done();
+ });
+
it("should return 200 and success message", async () => {
- const res = await request(app).get("/");
+ server = app._server || app;
+ const res = await request(app).get("/");
expect(res.statusCode).toBe(200);
expect(res.body.status).toBe("success");
});
});Alternatively, refactor app.js to defer app.listen() until explicitly called (e.g., only in production or when not being required by tests), which is a cleaner long-term fix.
🧰 Tools
🪛 GitHub Actions: CI (Continuous Integration)
[error] 1-1: Test suite failed: Auth API login expected 200 but received 500 during CI run. Command: 'cd backend && npm test'.
🤖 Prompt for AI Agents
In `@backend/tests/health.test.js` around lines 1 - 11, The test leaves the server
running because app.js calls app.listen on require; add an afterAll hook that
closes the server to avoid port conflicts. Change the test to require the server
instance (e.g., const server = require("../app");) or adjust app.js to export
the server returned by app.listen (keep symbol name server), then add
afterAll(async () => server.close()); (or call done/return a promise) to
properly close the server after tests; reference afterAll, app, server, and
app.listen when making the change.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
All backend tests are passing locally and in CI. Please let me know if any further adjustments are needed. |
|
hi mentor,please reeview my pr |
|
Really Sorry i forgot to look into the PR, Just reviewing it. @alok-kumar0421 |
|
@yash-pouranik no problem at all. |
| const request = require("supertest"); | ||
| const app = require("../app"); | ||
|
|
||
| describe("Health Check API", () => { | ||
| it("should return 200 and success message", async () => { | ||
| const res = await request(app).get("/"); | ||
|
|
||
| expect(res.statusCode).toBe(200); | ||
| expect(res.body.status).toBe("success"); | ||
| }); | ||
| }); |
| const requiredEnvVars = ["MONGO_URL", "PORT", "REDIS_URL"]; | ||
|
|
||
| function validateEnv() { | ||
| if (process.env.NODE_ENV === "test") return; |
There was a problem hiding this comment.
@alok-kumar0421 look into ./backend/app.js i prevented the validateEnv function call if env is test
so i think you can remove this. hmm??
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
closing as there's no response |
2 participants
Added a smoke test for the root health endpoint.
The test verifies that:
Additionally, Redis has been made test-safe by providing a lightweight mock implementation when NODE_ENV=test.
This ensures existing test suites run without requiring an active Redis instance.
Summary by CodeRabbit
Tests
Chores