-
Notifications
You must be signed in to change notification settings - Fork 424
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
Use sparse checkout for e2e tests #3984
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
4 Skipped Deployments
|
WalkthroughThe changes in the Changes
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (4)
.github/workflows/frontend-e2e-tests.yml
is excluded by!**/*.yml
.github/workflows/monitoring-e2e-tests.yml
is excluded by!**/*.yml
.github/workflows/monitoring-limit-geo-e2e-tests.yml
is excluded by!**/*.yml
.github/workflows/prod-frontend-e2e-tests.yml
is excluded by!**/*.yml
📒 Files selected for processing (1)
packages/e2e/pages/trade-page.ts
(1 hunks)
🔇 Additional comments (2)
packages/e2e/pages/trade-page.ts (2)
68-68
: LGTM: Added logging improves test observability
The added console log follows the established pattern in other methods and improves test debugging capabilities.
Line range hint 1-374
: Verify PR objective alignment
The changes in this file appear to be unrelated to the PR's objective of implementing sparse checkout for e2e tests. The modifications only adjust logging and wait times in the gotoOrdersHistory
method.
Let's verify if there are other relevant changes:
await this.page.waitForTimeout(4000); | ||
await new Promise((f) => setTimeout(f, timeout * 1000)); | ||
const currentUrl = this.page.url(); | ||
console.log(`FE opened at: ${currentUrl}`); |
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.
🛠️ Refactor suggestion
Consider addressing potential timing issues instead of increasing wait time
The timeout increase from 1000ms to 4000ms could be masking underlying timing issues. Additionally, there are multiple sequential waits that could be consolidated.
Consider these improvements:
- Use Playwright's built-in wait mechanisms instead of arbitrary timeouts:
- await this.page.waitForTimeout(4000);
- await new Promise((f) => setTimeout(f, timeout * 1000));
+ await this.page.waitForURL(/.*\/history/, { timeout: timeout * 1000 });
- If the increased timeout is needed due to flakiness, add a comment explaining why 4000ms is required
Committable suggestion skipped: line range outside the PR's diff.
What is the purpose of the change:
Use sparse checkout for e2e tests to save 1-2 minutes on setup.