test: add notify route delete coverage#4956
Conversation
|
@pari-28 is attempting to deploy a commit to the jhasourav07's projects Team on Vercel. A member of the Team first needs to authorize it. |
Aamod-Dev
left a comment
There was a problem hiding this comment.
Thanks for the PR. I am requesting changes because this applies 'theme contrast' coverage to the 'notify' API route. Server-side API routes do not render any interactive DOM elements or CSS properties, making it impossible to test them for light and dark modes. Please target theme contrast tests to frontend UI components exclusively.
Aamod-Dev
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I went through the changes and the overall approach looks good, but there are a few issues that should be addressed before this can be merged. Most of the concerns are related to correctness and maintainability. Please take a look at the points below and update the PR accordingly.
- The test targets an API route, which handles backend logic and does not have a user interface. Testing visual theme contrast here is not applicable.
- It appears the tests rely on asserting local hardcoded variables against themselves rather than exercising the actual application logic.
Once these issues are addressed, I'll be happy to take another look. Thanks again for the contribution.
Aamod-Dev
left a comment
There was a problem hiding this comment.
Thanks for putting this together. The code looks clean and solves the issue effectively.
No concerns from my end. Approved.
Aamod-Dev
left a comment
There was a problem hiding this comment.
This PR is currently marked with the \status:blocked\ label. Please resolve the blockers so we can proceed with a full review and approval.
Aamod-Dev
left a comment
There was a problem hiding this comment.
Thanks for the contribution! I went through the changes and have evaluated them according to the rubric.
e425bca to
af6cb9d
Compare
📦 Next.js Bundle Size Report (Gzipped Sizes)✨ No significant bundle size changes detected. 📊 Summary of Totals
|
Aamod-Dev
left a comment
There was a problem hiding this comment.
Excellent test coverage! I went through the changes and covering the deletion routes for notifications secures user privacy and state consistency.
Labels applied:
- level:advanced: Simulating complex route deletions.
- quality:clean: Perfect assertions.
- ype:feature, ype:testing: Ensures notification lifecycle safety.
|
🎉 Congratulations @pari-28! Your PR has been successfully merged. 🚀 Thank you for contributing to CommitPulse. Your work helps us build a better tool for the community.
Keep building! 💻✨ |
Description
Fixes #4433
Added
app/api/notify/route.delete.test.tsto provide isolated test coverage for the notify DELETE endpoint.Added Test Coverage
Validation
Pillar
Visual Preview
N/A (test-only change)
Checklist before requesting a review:
CONTRIBUTING.mdfile.npm run formatandnpm run lintlocally and resolved all errors.README.mdif required. (Not applicable)