-
-
Notifications
You must be signed in to change notification settings - Fork 5
Update e2e tests to support new configuration step order #3389
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3389 +/- ##
=======================================
Coverage 82.58% 82.58%
=======================================
Files 608 608
Lines 35958 35958
Branches 5864 5864
=======================================
Hits 29697 29697
Misses 5383 5383
Partials 878 878 ☔ View full report in Codecov by Sentry. |
1fd6dc5
to
7cb8181
Compare
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.
Pull Request Overview
This PR updates e2e tests to align with the new configuration step order that was introduced in PR #3370 but missed in the test files. The main change reorders the configuration steps in the draft generation workflow, moving reference project configuration before source project configuration.
- Reordered configuration steps in e2e test workflows to match new UI flow
- Commented out screenshot test code that was no longer needed
- Added pull_request trigger to GitHub workflow for better test coverage
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
localized-screenshots.ts | Reordered configuration steps and commented out login screenshot code |
generate-draft.ts | Updated step order with clear comments for reference projects, source project, and training data |
e2e-tests.yml | Added pull_request trigger to GitHub workflow |
const originalViewportSize = await page.viewportSize(); | ||
await page.locator('[data-test-id="configure-button"]').click(); | ||
|
||
const originalViewportSize = await page.viewportSize()!; |
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 non-null assertion operator (!) is unsafe here. The viewportSize() method can return null, and forcing it with ! could cause a runtime error. Use null checking instead: const originalViewportSize = await page.viewportSize(); if (!originalViewportSize) throw new Error('Viewport size not available');
const originalViewportSize = await page.viewportSize()!; | |
const originalViewportSize = await page.viewportSize(); | |
if (!originalViewportSize) throw new Error('Viewport size not available'); |
Copilot uses AI. Check for mistakes.
7cb8181
to
b525f32
Compare
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.
@RaymondLuong3 reviewed 2 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Nateowami)
Thank you @RaymondLuong3! Getting these merged so they actually start passing is going to be really helpful. |
This should have been updated in #3370, but was missed. Nothing is changed except the order of the steps.
This change is