-
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
[e2e] Update remaining tests and add missing comparison coverage #7363
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #7363 +/- ##
==========================================
- Coverage 55.85% 55.84% -0.01%
==========================================
Files 659 659
Lines 26243 26245 +2
Branches 2549 2549
==========================================
- Hits 14658 14657 -1
- Misses 10878 10881 +3
Partials 707 707
... and 7 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Current Playwright Test Results Summary✅ 174 Passing - ❌ 1 Failing - Run may still be in progress, this comment will be updated as current testing workflow or job completes... (Last updated on 01/11/2024 06:48:31am UTC) ❌ Failures📄 functional/plugins/telemetryTable/telemetryTable.e2e.spec.js • 1 FailureTest Case Results
|
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Restricted Notebook with a page locked and with an embed @addinit Disallows embeds to be deleted if page locked @addinit
Retry 1 • Initial Attempt |
6.15% (4)4 / 65 runsfailed over last 7 days |
49.23% (32)32 / 65 runsflaked over last 7 days |
<span class="c-grid-item__metadata__type">{{ item.type.name }}</span> | ||
</div> | ||
</div> | ||
<div class="c-grid-item__controls"> | ||
<div class="is-status__indicator" :title="`This item is ${status}`"></div> | ||
<div | ||
class="is-status__indicator" |
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.
like this
src/ui/layout/AboutDialog.vue
Outdated
@@ -57,7 +60,7 @@ | |||
</p> | |||
</div> | |||
<h2>Version Information</h2> | |||
<ul class="t-info l-info s-info"> | |||
<ul data-testid="versionInfo" class="t-info l-info s-info"> |
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.
@ozyx not really sure what else to add here. I guess I could use an 'id'
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 a ul
so it implicitly has the role of list
. I think we would just need a label.
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 work! Just a few comments
@@ -22,7 +22,7 @@ | |||
<template> | |||
<!-- eslint-disable vue/no-v-html --> | |||
<div class="c-about c-about--splash"> | |||
<div class="c-about__image c-splash-image"></div> | |||
<div class="c-about__image c-splash-image" role="img" alt="Open MCT Splash Logo"></div> |
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 catch
src/ui/layout/AboutDialog.vue
Outdated
@@ -57,7 +60,7 @@ | |||
</p> | |||
</div> | |||
<h2>Version Information</h2> | |||
<ul class="t-info l-info s-info"> | |||
<ul data-testid="versionInfo" class="t-info l-info s-info"> |
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 a ul
so it implicitly has the role of list
. I think we would just need a label.
# VSCode | ||
.vscode/settings.json |
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.
😞
@@ -17,8 +12,7 @@ const config = { | |||
timeout: 200 * 1000, | |||
reuseExistingServer: true //This was originally disabled to prevent differences in local debugging vs. CI. However, it significantly speeds up local debugging. | |||
}, | |||
maxFailures: MAX_FAILURES, //Limits failures to 5 to reduce CI Waste | |||
workers: NUM_WORKERS, //Limit to 2 for CircleCI Agent | |||
workers: '75%', //Limit to 75% of the CPU to support running with dev server |
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.
rad
@@ -155,7 +155,7 @@ test.describe('AppActions', () => { | |||
|
|||
await page.goto('./#/browse/mine'); | |||
//Click the Create button | |||
await page.click('button:has-text("Create")'); | |||
await page.getByRole('button', { name: 'Create' }).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.
about dang time
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.
Awesome work! Just a few comments
type: 'Display Layout', | ||
name: 'Child Layout 2', | ||
parent: parent.uuid | ||
}); | ||
|
||
await page.goto(parent.url); | ||
await page.goto(parent.url, { 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.
we don't need the waitUntil here unless you noticed some different behavior?
@@ -107,7 +106,7 @@ test.describe('Generate Visual Test Data @localStorage @generatedata', () => { | |||
parent: parent.uuid | |||
}); | |||
|
|||
await page.goto(parent.url); | |||
await page.goto(parent.url, { 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.
same
@@ -134,7 +133,7 @@ test.describe('Generate Visual Test Data @localStorage @generatedata', () => { | |||
await page.locator('button[title="More actions"]').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.
missed a spot?
@@ -69,7 +69,7 @@ test.describe('Gantt Chart', () => { | |||
.getByRole('dialog') | |||
.filter({ hasText: 'This action will replace the current Plan. Do you want to continue?' }); | |||
await expect(replaceModal).toBeVisible(); | |||
await page.getByRole('button', { name: 'OK' }).click(); | |||
await page.getByRole('button', { name: 'Ok', exact: true }).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.
It's Ok
and not OK
?
@@ -53,10 +51,10 @@ test.describe('Gauge', () => { | |||
|
|||
// Navigate to the gauge and verify that | |||
// the SWG appears in the elements pool | |||
await page.goto(gauge.url); | |||
await editButtonLocator.click(); | |||
await page.goto(gauge.url, { 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.
don't think we need the waitUntil here
@@ -109,7 +107,7 @@ test.describe('Gauge', () => { | |||
description: 'https://github.com/nasa/openmct/issues/5356' | |||
}); | |||
//Click the Create button | |||
await page.click('button:has-text("Create")'); | |||
await page.getByRole('button', { name: 'Create' }).click(); | |||
|
|||
// Click the object specified by 'type' | |||
await page.click(`li[role='menuitem']:text("Gauge")`); |
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.
missed one?
@@ -8,7 +8,6 @@ | |||
"@axe-core/playwright": "4.8.2", | |||
"@babel/eslint-parser": "7.23.3", | |||
"@braintree/sanitize-url": "6.0.4", | |||
"@deploysentinel/playwright": "0.3.4", |
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.
RIP
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! Nice one
Closes #7305
Addresses #7040 #7307
Describe your changes:
Generally brings all of our visual tests up to speed to address #7307 And add coverage that Charles needs
All Submissions:
Author Checklist
type:
label? Note: this is not necessarily the same as the original issue.Reviewer Checklist