-
Notifications
You must be signed in to change notification settings - Fork 0
Fix ScrapyardItem mocks #48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Reviewer's GuideRefactored test mocks in index.test.js to reference ScrapyardItem and ensure all dependent database methods are mocked (countDocuments and find) for the index route tests. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
| ]), | ||
| countDocuments: jest.fn().mockResolvedValue(2), | ||
| find: jest.fn().mockResolvedValue([ | ||
| { username: 'recentuser', lastActive: new Date().toISOString() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of new Date().toISOString() directly in the test mock (line 15) can introduce flakiness in tests. If the test runs around midnight and the date changes, it could lead to inconsistent results.
Recommendation: Consider using a fixed timestamp or mocking the Date object to return a consistent value during tests to ensure test reliability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @numbpill3d - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| find: jest.fn().mockResolvedValue([ | ||
| { username: 'recentuser', lastActive: new Date().toISOString() } | ||
| ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): Consider using a fixed date string in User.find mock for test predictability.
Using a static date string for lastActive will make test results consistent and prevent flakiness from dynamic timestamps.
| find: jest.fn().mockResolvedValue([ | |
| { username: 'recentuser', lastActive: new Date().toISOString() } | |
| ]) | |
| find: jest.fn().mockResolvedValue([ | |
| { username: 'recentuser', lastActive: '2023-01-01T00:00:00.000Z' } | |
| ]) |
Summary
index.test.jsto referenceScrapyardItemcountDocumentsand other methods to satisfy route dependenciesTesting
npx jest --runTestsByPath tests/server/routes/index.test.js --silentnpm test --silent(fails: fetch failed)https://chatgpt.com/codex/tasks/task_e_68450e581b70832f8ad5f17e9954db19
Summary by Sourcery
Fix failing index route tests by updating mocks to reference ScrapyardItem and mock required methods
Tests: