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
[e2e] Update playwright eslint plugin #7581
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7581 +/- ##
==========================================
- Coverage 55.64% 54.95% -0.70%
==========================================
Files 672 672
Lines 26996 26996
Branches 2626 2626
==========================================
- Hits 15023 14836 -187
- Misses 11254 11441 +187
Partials 719 719
... and 26 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?
@@ -4,7 +4,8 @@ module.exports = { | |||
browser: true, | |||
es6: true, | |||
jasmine: true, | |||
amd: true | |||
amd: true, |
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 think we can set this to false or remove it altogether
@@ -4,7 +4,8 @@ module.exports = { | |||
browser: true, | |||
es6: true, | |||
jasmine: true, | |||
amd: true | |||
amd: true, | |||
node: true |
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
ref="tooltip-wrapper" | ||
class="c-menu c-tooltip-wrapper" | ||
:style="toolTipLocationStyle" | ||
role="tooltip" |
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.
hah
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.
@@ -84,7 +84,7 @@ test.describe('Plot Tagging Performance', () => { | |||
await setRealTimeMode(page); | |||
|
|||
// Search for Science | |||
await page.getByRole('searchbox', { name: 'Search Input' }); | |||
await page.getByRole('searchbox', { name: 'Search Input' }).click(); |
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.
huh. do we need this line at all?
test('display layout with plots of swgs, alphanumerics, and condition sets, ', async ({ | ||
page | ||
}) => { | ||
test('display layout with plots of swgs, alphanumerics, and condition sets', async ({ page }) => { |
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.
ahhhhh
@@ -141,7 +141,7 @@ test.describe('Notebook Tests with CouchDB @couchdb', () => { | |||
// 4) The shared worker event from 👆 POST request | |||
notebookElementsRequests = []; | |||
await nbUtils.enterTextEntry(page, 'Fourth Entry'); | |||
page.waitForLoadState('networkidle'); | |||
page.waitForLoadState('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 don't know if these are necessarily 1 to 1 here. The networkidles serve a purpose. I think maybe we need to wait for network events specifically to complete? We should just leave these as warnings for now
@@ -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
@@ -531,7 +531,7 @@ test.describe('Example Imagery in Flexible layout', () => { | |||
|
|||
// Click text=OK | |||
await Promise.all([ | |||
page.waitForNavigation({ waitUntil: 'networkidle' }), | |||
page.waitForNavigation({ 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.
intellisense says this is deprecated
@@ -150,6 +151,7 @@ module.exports = { | |||
'error', | |||
{ | |||
cases: { | |||
camelCase: true, |
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'm not sure exactly why the robot added this
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.
yeah we need to remove this. it's supposed to enforce vue files only i think
@@ -85,6 +85,7 @@ | |||
v-for="(treeItem, index) in visibleItems" | |||
:key="`${treeItem.navigationPath}-${index}-${treeItem.object.name}`" | |||
:node="treeItem" | |||
draggable="true" |
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.
this needs to be bound or it gets registered as a string
:draggable="true"
@click="back()" | ||
></button> | ||
<button | ||
class="c-button icon-reset" | ||
title="Reset pan/zoom" | ||
title="Reset pan and zoom" | ||
aria-label="Reset pan and zoom" |
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 just fixed this on another branch / PR
@@ -98,12 +98,14 @@ | |||
v-if="selectedPage && !selectedPage.isLocked" | |||
:aria-disabled="activeTransaction" | |||
class="c-notebook__drag-area icon-plus" | |||
aria-dropeffect="link" | |||
aria-labelledby="newEntryLabel" |
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.
seems like more of a description than a label but /shrug
:title="gaugeTitle" | ||
:aria-label="`${domainObject.name}`" |
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.
not gauge title?
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.
Gauge title isn't meaningful
Closes #7307
Describe your changes:
All Submissions:
Author Checklist
type:
label? Note: this is not necessarily the same as the original issue.Reviewer Checklist