[e2e] Update playwright eslint plugin #7581

unlikelyzero · 2024-03-12T02:00:55Z

Closes #7307

Describe your changes:

updates the eslint-playwright-plugin package
updates the default rules

All Submissions:

Have you followed the guidelines in our Contributing document?
Have you checked to ensure there aren't other open Pull Requests for the same update/change?
Is this a notable change that will require a special callout in the release notes? For example, will this break compatibility with existing APIs or projects that consume these plugins?

Author Checklist

Changes address original issue?
Tests included and/or updated with changes?
Has this been smoke tested?
Have you associated this PR with a type: label? Note: this is not necessarily the same as the original issue.
Have you associated a milestone with this PR? Note: leave blank if unsure.
Is this a breaking change to be called out in the release notes?
Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

Changes appear to address issue?
Reviewer has tested changes by following the provided instructions?
Changes appear not to be breaking changes?
Appropriate automated tests included?
Code style and in-line documentation are appropriate?

…update

codecov · 2024-03-12T02:05:17Z

Codecov Report

Attention: Patch coverage is 53.84615% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 54.95%. Comparing base (8c2558b) to head (349be42).
Report is 5 commits behind head on master.

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

Flag	Coverage Δ
e2e-full	`23.70% <ø> (-18.16%)`	⬇️
e2e-stable	`58.66% <ø> (-0.94%)`	⬇️
unit	`48.52% <53.84%> (ø)`

Files	Coverage Δ
src/plugins/gauge/components/GaugeComponent.vue	`62.06% <ø> (ø)`
src/ui/layout/MctTree.vue	`25.81% <ø> (ø)`
.../plugins/notebook/components/NotebookComponent.vue	`17.84% <80.00%> (ø)`
src/plugins/plot/MctPlot.vue	`36.25% <60.00%> (ø)`
src/api/tooltips/components/TooltipComponent.vue	`0.00% <0.00%> (ø)`

... and 26 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8c2558b...349be42. Read the comment docs.

ozyx

Got some comments.

Do you think we should keep some of the repeat offenders as warnings for now and fix them over time?

ozyx · 2024-03-12T20:49:35Z

.eslintrc.cjs

@@ -4,7 +4,8 @@ module.exports = {
    browser: true,
    es6: true,
    jasmine: true,
-    amd: true
+    amd: true,


i think we can set this to false or remove it altogether

ozyx · 2024-03-12T20:50:10Z

.eslintrc.cjs

@@ -4,7 +4,8 @@ module.exports = {
    browser: true,
    es6: true,
    jasmine: true,
-    amd: true
+    amd: true,
+    node: true


ozyx · 2024-03-12T20:50:48Z

src/api/tooltips/components/TooltipComponent.vue

+    ref="tooltip-wrapper"
+    class="c-menu c-tooltip-wrapper"
+    :style="toolTipLocationStyle"
+    role="tooltip"


ozyx · 2024-03-12T20:54:40Z

e2e/README.md

+  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/)


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.

ozyx · 2024-03-12T21:02:35Z

e2e/tests/performance/tagging.perf.spec.js

@@ -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();


huh. do we need this line at all?

ozyx · 2024-03-12T21:03:25Z

e2e/tests/performance/memory/navigation.memory.perf.spec.js

-  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 }) => {


ozyx · 2024-03-12T21:06:41Z

e2e/tests/functional/plugins/notebook/notebookWithCouchDB.e2e.spec.js

@@ -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');


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

ozyx · 2024-03-12T21:07:20Z

e2e/tests/functional/plugins/importAndExportAsJSON/exportAsJson.e2e.spec.js

@@ -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' });


i worry about this breaking the couchdb tests

ozyx · 2024-03-12T21:08:10Z

e2e/tests/functional/plugins/imagery/exampleImagery.e2e.spec.js

@@ -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' }),


intellisense says this is deprecated

unlikelyzero · 2024-03-13T23:33:57Z

.eslintrc.cjs

@@ -150,6 +151,7 @@ module.exports = {
      'error',
      {
        cases: {
+          camelCase: true,


i'm not sure exactly why the robot added this

yeah we need to remove this. it's supposed to enforce vue files only i think

ozyx · 2024-03-13T23:55:04Z

src/ui/layout/MctTree.vue

@@ -85,6 +85,7 @@
            v-for="(treeItem, index) in visibleItems"
            :key="`${treeItem.navigationPath}-${index}-${treeItem.object.name}`"
            :node="treeItem"
+            draggable="true"


this needs to be bound or it gets registered as a string

:draggable="true"

ozyx · 2024-03-13T23:55:41Z

src/plugins/plot/MctPlot.vue

                @click="back()"
              ></button>
              <button
                class="c-button icon-reset"
-                title="Reset pan/zoom"
+                title="Reset pan and zoom"
+                aria-label="Reset pan and zoom"


I just fixed this on another branch / PR

ozyx · 2024-03-13T23:56:37Z

src/plugins/notebook/components/NotebookComponent.vue

@@ -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"


seems like more of a description than a label but /shrug

ozyx · 2024-03-13T23:57:03Z

src/plugins/gauge/components/GaugeComponent.vue

    :title="gaugeTitle"
+    :aria-label="`${domainObject.name}`"


not gauge title?

Gauge title isn't meaningful

unlikelyzero added 3 commits February 26, 2024 04:28

update eslint package

0590b50

first pass of lint fixes

2897ca6

Merge branch 'master' of https://github.com/nasa/openmct into eslint_…

df2d9ab

…update

unlikelyzero added the type:maintenance tests, chores, or project maintenance label Mar 12, 2024

unlikelyzero added this to the Target:4.0.0 milestone Mar 12, 2024

unlikelyzero requested a review from ozyx March 12, 2024 02:01

unlikelyzero added 3 commits March 12, 2024 11:24

update package

82326dc

change ruleset

8933baf

update component tests to match linting rules

a393bfb

ozyx requested changes Mar 12, 2024

View reviewed changes

unlikelyzero added 6 commits March 13, 2024 16:15

driveby

d8df0e1

start to factor out bad locators

6e1272d

update gauge component

b7b7bc2

update notebook snapshot drop area

444c9ff

Update plot aria

6edb3b2

add draggable true to tree items

349be42

unlikelyzero commented Mar 13, 2024

View reviewed changes

ozyx reviewed Mar 13, 2024

View reviewed changes

unlikelyzero modified the milestones: Target:4.0.0, Target:4.0.1 Mar 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[e2e] Update playwright eslint plugin #7581

[e2e] Update playwright eslint plugin #7581

unlikelyzero commented Mar 12, 2024 •

edited

codecov bot commented Mar 12, 2024 •

edited

ozyx left a comment

ozyx Mar 12, 2024

ozyx Mar 12, 2024

ozyx Mar 12, 2024

ozyx Mar 12, 2024

ozyx Mar 12, 2024

ozyx Mar 12, 2024

ozyx Mar 12, 2024

ozyx Mar 12, 2024

ozyx Mar 12, 2024

unlikelyzero Mar 13, 2024

ozyx Mar 13, 2024

ozyx Mar 13, 2024

ozyx Mar 13, 2024

ozyx Mar 13, 2024

ozyx Mar 13, 2024

unlikelyzero Mar 14, 2024

[e2e] Update playwright eslint plugin #7581

Are you sure you want to change the base?

[e2e] Update playwright eslint plugin #7581

Conversation

unlikelyzero commented Mar 12, 2024 • edited

Describe your changes:

All Submissions:

Author Checklist

Reviewer Checklist

codecov bot commented Mar 12, 2024 • edited

Codecov Report

ozyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unlikelyzero commented Mar 12, 2024 •

edited

codecov bot commented Mar 12, 2024 •

edited