test(packages/renderer): make test run faster #13262
Conversation
|
Warning Rate limit exceeded@eqqe has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 5 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe test case "Check cleanupProviders is called and button is in progress" in the TroubleshootingRepairCleanup.spec.ts file was modified to reduce the artificial wait times associated with the mocked cleanup operation. The delay in the mocked cleanupProviders function was decreased from 2000 milliseconds to 10 milliseconds. Additionally, the waits after clicking the cleanup button and before asserting button states were shortened from 100 milliseconds to 5 milliseconds, and the final wait for the cleanup to finish was reduced from 2000 milliseconds to 10 milliseconds. No changes were made to exported or public entity declarations. Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Hey @eqqe - I've reviewed your changes - here's some feedback:
- Instead of real setTimeouts, consider using Jest’s fake timers (jest.useFakeTimers and jest.runAllTimers) to make the tests deterministic and faster.
- Replace hardcoded delays with testing‐library’s waitFor (or flushPromises) to avoid arbitrary timing and improve test reliability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of real setTimeouts, consider using Jest’s fake timers (jest.useFakeTimers and jest.runAllTimers) to make the tests deterministic and faster.
- Replace hardcoded delays with testing‐library’s waitFor (or flushPromises) to avoid arbitrary timing and improve test reliability.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
not sure about the last part of the title "to avoid blocking ci" IMHO 2s won't block the CI |
It is not blocking but it will improve by approximately 0.3% the speed of every unit test run in the CI. |
|
I would call it misleading title in case we don't hit the cache, the delay penalty is much more than 2s so I would just stick to "make the test faster" |
Codecov ReportAll modified and coverable lines are covered by tests ✅ 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/renderer/src/lib/troubleshooting/TroubleshootingRepairCleanup.spec.ts (1)
96-96: Consider optimizing the second test case for consistency.The second test case still uses a 100ms wait while the first test was optimized to 5ms. Consider reducing this to 5ms for consistency, unless there's a specific reason this test requires more time for error handling.
- await new Promise(resolve => setTimeout(resolve, 100)); + await new Promise(resolve => setTimeout(resolve, 5));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/renderer/src/lib/troubleshooting/TroubleshootingRepairCleanup.spec.ts(1 hunks)
🔇 Additional comments (3)
packages/renderer/src/lib/troubleshooting/TroubleshootingRepairCleanup.spec.ts (3)
46-47: LGTM! Effective timing optimization.The reduction from 2000ms to 10ms significantly improves test performance while maintaining sufficient delay for UI state verification. The 10ms delay should still allow proper testing of the button's state transitions.
54-54: Good optimization for next tick timing.The reduction from 100ms to 5ms is appropriate for waiting for the next tick. This should be sufficient for DOM updates while significantly improving test speed.
62-63: Consistent timing synchronization.The wait time properly matches the mock delay, ensuring the test waits for the cleanup operation to complete before asserting the final button state.
Signed-off-by: Simon Rey <sfbrey+eqqe@gmail.com>
| await fireEvent.click(cleanupButton); | ||
|
|
||
| // wait next tick | ||
| await new Promise(resolve => setTimeout(resolve, 100)); |
There was a problem hiding this comment.
Can't the test be refactored to check using a waitFor to wait for a valid condition instead of waiting with a timeout ?
There was a problem hiding this comment.
Oops sorry I merged without seing your comment, I will take a look thanks
What does this PR do?
I changed the timers value in this test to make it pass in 79ms (on my Mac) instead of 2 200ms.
This is to make the CI faster.
Screenshot / video of UI
What issues does this PR fix or reference?
How to test this PR?