-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
test(e2e): Major refactor and stabilization of e2e tests #7581
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7581 +/- ##
==========================================
- Coverage 57.15% 56.84% -0.31%
==========================================
Files 674 674
Lines 27276 27283 +7
Branches 2668 2669 +1
==========================================
- Hits 15589 15509 -80
- Misses 11347 11435 +88
+ Partials 340 339 -1
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
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.
Got some comments.
Do you think we should keep some of the repeat offenders as warnings for now and fix them over time?
1. Use [user-facing locators](https://playwright.dev/docs/best-practices#use-locators) (Now a eslint rule!) | ||
|
||
```js | ||
page.getByRole('button', { name: 'Create' } ) | ||
``` | ||
Instead of | ||
```js | ||
page.locator('.c-create-button') | ||
``` | ||
Note: `page.locator()` can be used in performance tests as xk6-browser does not yet support the new `page.getBy` pattern and css lookups can be [1.5x faster](https://serpapi.com/blog/css-selectors-faster-than-getbyrole-playwright/) |
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:
- Prefer using user-facing locators for better readability and maintainability:
// Recommended
page.getByRole('button', { name: 'Create' })
Instead of the less intuitive:
// Not recommended
page.locator('.c-create-button')
Note: In performance tests, page.locator()
may still be used as xk6-browser lacks support for the page.getBy
pattern. Additionally, CSS selectors can be up to 1.5x faster than role-based selectors.
@@ -152,7 +152,7 @@ test.describe('ExportAsJSON Disabled Actions', () => { | |||
test.describe('ExportAsJSON ProgressBar @couchdb', () => { | |||
let folder; | |||
test.beforeEach(async ({ page }) => { | |||
await page.goto('./', { waitUntil: 'networkidle' }); | |||
await page.goto('./', { waitUntil: 'domcontentloaded' }); |
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.
i worry about this breaking the couchdb tests
e2e/tests/functional/plugins/imagery/exampleImagery.e2e.spec.js
Outdated
Show resolved
Hide resolved
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.
1 thing to fix and we need to talk about the Aria label on gauges
await page.getByLabel('Edit Object').click(); | ||
|
||
// Expand the 'My Items' folder in the left tree | ||
page.click('button[title="Show selected item in tree"]'); |
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.
Fix
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.
🫡
@@ -281,29 +281,3 @@ test.describe('Global Time Conductor', () => { | |||
// select an option and verify the offsets are updated correctly | |||
}); | |||
}); | |||
|
|||
test.describe('Time Conductor History', () => { |
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.
Is this totally gone?
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.
it's no longer relevant because you can't set milliseconds in the time conductor anymore. The time conductor history menu is still there, though. Maybe we should keep the suite
|
||
return `${this.expanded ? 'Collapse' : 'Expand'}${name}${type}`; | ||
return `${this.expanded ? 'Collapse' : 'Expand'}${name} Plot Series Options`; |
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.
Nice
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.
LGTM 👀 😏
This PR touches runtime code and needs test instructions in the associated issue(s) |
Closes #7307
Closes #7797
Closes #7763
Describe your changes:
eslint-playwright-plugin
package to v1.5.2@clock
tests to use the clock API.page.click()
to useLocator.click()
insteadpage.dragAndDrop()
to useLocator.dragTo(Locator)
insteadAll Submissions:
Author Checklist
type:
label? Note: this is not necessarily the same as the original issue.Reviewer Checklist