From e9c51c2bcc11e38ac64e67820fe8e662d44f2287 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 6 Sep 2023 13:31:17 -0500 Subject: [PATCH 1/8] minimal repro. lmfao --- app/forms/modal-loop.tsx | 30 ++++++++++++++++++++++++++++++ app/routes.tsx | 2 ++ 2 files changed, 32 insertions(+) create mode 100644 app/forms/modal-loop.tsx diff --git a/app/forms/modal-loop.tsx b/app/forms/modal-loop.tsx new file mode 100644 index 0000000000..5d6676a1d2 --- /dev/null +++ b/app/forms/modal-loop.tsx @@ -0,0 +1,30 @@ +import { useState } from 'react' + +import { Button, Modal, SideModal } from '@oxide/ui' + +export function ModalLoopSideModal() { + const [modalOpen, setModalOpen] = useState(false) + function closeModal() { + if (confirm('close?')) { + setModalOpen(false) + } + } + return ( + {}} title="Modal loop test"> + + + {modalOpen && ( + + No + {}} + actionText="Done" + cancelText="Cancel" + /> + + )} + + + ) +} diff --git a/app/routes.tsx b/app/routes.tsx index 8af6be34c6..f08c75646d 100644 --- a/app/routes.tsx +++ b/app/routes.tsx @@ -19,6 +19,7 @@ import { import { CreateImageFromSnapshotSideModalForm } from './forms/image-from-snapshot' import { CreateImageSideModalForm } from './forms/image-upload' import { CreateInstanceForm } from './forms/instance-create' +import { ModalLoopSideModal } from './forms/modal-loop' import { CreateProjectSideModalForm } from './forms/project-create' import { EditProjectSideModalForm } from './forms/project-edit' import { CreateSiloSideModalForm } from './forms/silo-create' @@ -341,6 +342,7 @@ export const routes = createRoutesFromElements( handle={{ crumb: 'Upload image' }} element={} /> + } /> } From 897dd332ad6c7053ac8e60f8d88af27fca998a01 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 6 Sep 2023 14:26:57 -0500 Subject: [PATCH 2/8] fix the most pressing part of the problem --- libs/ui/lib/modal/Modal.tsx | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/libs/ui/lib/modal/Modal.tsx b/libs/ui/lib/modal/Modal.tsx index 12cd5e7733..03e3e8e021 100644 --- a/libs/ui/lib/modal/Modal.tsx +++ b/libs/ui/lib/modal/Modal.tsx @@ -66,6 +66,12 @@ export function Modal({ children, onDismiss, title, isOpen }: ModalProps) { style={{ transform: y.to((value) => `translate3d(-50%, ${-50 + value}%, 0px)`), }} + // Prevents cancel loop on clicking on background over side + // modal to get out of image upload modal. Canceling out of + // confirm dialog returns focus to the dismissable layer, + // which triggers onDismiss again. And again. + // https://github.com/oxidecomputer/console/issues/1745 + onFocusOutside={(e) => e.preventDefault()} > {title && ( From 0f9503b9472c4fc88c6ae023fbb2576e33752456 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 6 Sep 2023 14:27:12 -0500 Subject: [PATCH 3/8] Revert "minimal repro. lmfao" e9c51c2bcc11e38ac64e67820fe8e662d44f2287 --- app/forms/modal-loop.tsx | 30 ------------------------------ app/routes.tsx | 2 -- 2 files changed, 32 deletions(-) delete mode 100644 app/forms/modal-loop.tsx diff --git a/app/forms/modal-loop.tsx b/app/forms/modal-loop.tsx deleted file mode 100644 index 5d6676a1d2..0000000000 --- a/app/forms/modal-loop.tsx +++ /dev/null @@ -1,30 +0,0 @@ -import { useState } from 'react' - -import { Button, Modal, SideModal } from '@oxide/ui' - -export function ModalLoopSideModal() { - const [modalOpen, setModalOpen] = useState(false) - function closeModal() { - if (confirm('close?')) { - setModalOpen(false) - } - } - return ( - {}} title="Modal loop test"> - - - {modalOpen && ( - - No - {}} - actionText="Done" - cancelText="Cancel" - /> - - )} - - - ) -} diff --git a/app/routes.tsx b/app/routes.tsx index f08c75646d..8af6be34c6 100644 --- a/app/routes.tsx +++ b/app/routes.tsx @@ -19,7 +19,6 @@ import { import { CreateImageFromSnapshotSideModalForm } from './forms/image-from-snapshot' import { CreateImageSideModalForm } from './forms/image-upload' import { CreateInstanceForm } from './forms/instance-create' -import { ModalLoopSideModal } from './forms/modal-loop' import { CreateProjectSideModalForm } from './forms/project-create' import { EditProjectSideModalForm } from './forms/project-edit' import { CreateSiloSideModalForm } from './forms/silo-create' @@ -342,7 +341,6 @@ export const routes = createRoutesFromElements( handle={{ crumb: 'Upload image' }} element={} /> - } /> } From f7804ebabf7aa42b8c8d739c5f8f871c7e8c8e4f Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 6 Sep 2023 15:02:02 -0500 Subject: [PATCH 4/8] using e2e tests for evil again --- app/test/e2e/image-upload.e2e.ts | 92 ++++++++++++++++++-------------- app/test/e2e/utils.ts | 2 + 2 files changed, 53 insertions(+), 41 deletions(-) diff --git a/app/test/e2e/image-upload.e2e.ts b/app/test/e2e/image-upload.e2e.ts index d321a2131b..8456d7ee68 100644 --- a/app/test/e2e/image-upload.e2e.ts +++ b/app/test/e2e/image-upload.e2e.ts @@ -10,7 +10,7 @@ import { expect, test } from '@playwright/test' import { MiB } from '@oxide/util' -import { expectNotVisible, expectRowVisible, expectVisible } from './utils' +import { expectNotVisible, expectRowVisible, expectVisible, sleep } from './utils' async function chooseFile(page: Page, size = 10 * MiB) { const fileChooserPromise = page.waitForEvent('filechooser') @@ -49,6 +49,16 @@ async function expectUploadProcess(page: Page) { await done.click() } +async function fillForm(page: Page, name: string) { + await page.goto('/projects/mock-project/images-new') + + await page.fill('role=textbox[name="Name"]', name) + await page.fill('role=textbox[name="Description"]', 'image description') + await page.fill('role=textbox[name="OS"]', 'Ubuntu') + await page.fill('role=textbox[name="Version"]', 'Dapper Drake') + await chooseFile(page) +} + test.describe('Image upload', () => { test('happy path', async ({ page }) => { await page.goto('/projects/mock-project/images') @@ -61,11 +71,7 @@ test.describe('Image upload', () => { await expectVisible(page, ['role=dialog[name="Upload image"]']) - await page.fill('role=textbox[name="Name"]', 'new-image') - await page.fill('role=textbox[name="Description"]', 'image description') - await page.fill('role=textbox[name="OS"]', 'Ubuntu') - await page.fill('role=textbox[name="Version"]', 'Dapper Drake') - await chooseFile(page) + await fillForm(page, 'new-image') await page.click('role=button[name="Upload image"]') @@ -81,13 +87,7 @@ test.describe('Image upload', () => { }) test('with name taken', async ({ page }) => { - await page.goto('/projects/mock-project/images-new') - - await page.fill('role=textbox[name="Name"]', 'image-1') - await page.fill('role=textbox[name="Description"]', 'image description') - await page.fill('role=textbox[name="OS"]', 'Ubuntu') - await page.fill('role=textbox[name="Version"]', 'Dapper Drake') - await chooseFile(page) + await fillForm(page, 'image-1') await expectNotVisible(page, ['text="Image name already exists"']) await page.click('role=button[name="Upload image"]') @@ -127,13 +127,7 @@ test.describe('Image upload', () => { }) test('cancel', async ({ page }) => { - await page.goto('/projects/mock-project/images-new') - - await page.fill('role=textbox[name="Name"]', 'new-image') - await page.fill('role=textbox[name="Description"]', 'image description') - await page.fill('role=textbox[name="OS"]', 'Ubuntu') - await page.fill('role=textbox[name="Version"]', 'Dapper Drake') - await chooseFile(page) + await fillForm(page, 'new-image') await page.click('role=button[name="Upload image"]') @@ -147,14 +141,13 @@ test.describe('Image upload', () => { // form is disabled and semi-hidden // await expectNotVisible(page, ['role=textbox[name="Name"]']) + const progressModal = page.getByRole('dialog', { name: 'Image upload progress' }) + page.on('dialog', (dialog) => dialog.accept()) // click yes on the are you sure prompt - await page - .getByRole('dialog', { name: 'Image upload progress' }) - .getByRole('button', { name: 'Cancel' }) - .click() + await progressModal.getByRole('button', { name: 'Cancel' }).click() // modal has closed - await expectNotVisible(page, ['role=dialog[name="Image upload progress"]']) + await expect(progressModal).toBeHidden() // form's back await expectVisible(page, ['role=textbox[name="Name"]']) @@ -168,14 +161,39 @@ test.describe('Image upload', () => { // await expectNotVisible(page, ['role=cell[name=tmp]']) }) - test('Image upload cancel and retry', async ({ page }) => { - await page.goto('/projects/mock-project/images-new') + // testing the onFocusOutside fix + test('cancel canceling', async ({ page }) => { + await fillForm(page, 'new-image') - await page.fill('role=textbox[name="Name"]', 'new-image') - await page.fill('role=textbox[name="Description"]', 'image description') - await page.fill('role=textbox[name="OS"]', 'Ubuntu') - await page.fill('role=textbox[name="Version"]', 'Dapper Drake') - await chooseFile(page) + const progressModal = page.getByRole('dialog', { name: 'Image upload progress' }) + + await page.getByRole('button', { name: 'Upload image' }).click() + await expect(progressModal).toBeVisible() + + let confirmCount = 0 + + page.on('dialog', (dialog) => { + confirmCount += 1 + dialog.dismiss() + }) // click cancel on the are you sure prompt + + await progressModal.getByRole('button', { name: 'Cancel' }).click() + + // still visible because we canceled the cancel! + await expect(progressModal).toBeVisible() + expect(confirmCount).toEqual(1) + + // now let's try canceling by clicking out on the background over the side modal + await page.getByLabel('4096').click() + + await sleep(100) + + // without the onFocusOutside fix this is a higher number + expect(confirmCount).toEqual(2) + }) + + test('Image upload cancel and retry', async ({ page }) => { + await fillForm(page, 'new-image') await page.click('role=button[name="Upload image"]') @@ -218,15 +236,7 @@ test.describe('Image upload', () => { for (const { imageName, stepText } of failureCases) { test(`failure ${imageName}`, async ({ page }) => { - await page.goto('/projects/mock-project/images-new') - - // note special image name - await page.fill('role=textbox[name="Name"]', imageName) - - await page.fill('role=textbox[name="Description"]', 'image description') - await page.fill('role=textbox[name="OS"]', 'Ubuntu') - await page.fill('role=textbox[name="Version"]', 'Dapper Drake') - await chooseFile(page) + await fillForm(page, imageName) await page.click('role=button[name="Upload image"]') diff --git a/app/test/e2e/utils.ts b/app/test/e2e/utils.ts index 527b598c19..60c6583cef 100644 --- a/app/test/e2e/utils.ts +++ b/app/test/e2e/utils.ts @@ -140,3 +140,5 @@ export async function expectObscured(locator: Locator) { async () => await locator.click({ trial: true, timeout: 50 }) ).rejects.toThrow(/locator.click: Timeout 50ms exceeded/) } + +export const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)) From 4c1f849c3fa00ed39630bb0a56bf5e49f0af561e Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 6 Sep 2023 15:09:12 -0500 Subject: [PATCH 5/8] increase per-test timeout --- playwright.config.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/playwright.config.ts b/playwright.config.ts index 1f96a98ea2..04a446580f 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -20,7 +20,7 @@ const config: PlaywrightTestConfig = { retries: process.env.CI ? 2 : 0, /* Opt out of parallel tests on CI. */ workers: process.env.CI ? 1 : undefined, - timeout: 60000, + timeout: 2 * 60 * 1000, // 2 minutes, surely overkill fullyParallel: true, use: { trace: 'retain-on-failure', From 4025a0182b327b7802fb0e3289f1e6567a9f0d47 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 7 Sep 2023 10:27:39 -0500 Subject: [PATCH 6/8] fix debug-ci-e2e-fail by filtering for CI workflow runs --- tools/debug-ci-e2e-fail.sh | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/debug-ci-e2e-fail.sh b/tools/debug-ci-e2e-fail.sh index d37d855b05..5af4385e9f 100755 --- a/tools/debug-ci-e2e-fail.sh +++ b/tools/debug-ci-e2e-fail.sh @@ -9,7 +9,8 @@ set -e set -o pipefail # Get the ID of the last github actions run if there was one -RUN_ID=$(gh run list -b $(git rev-parse --abbrev-ref HEAD) -L 1 --json databaseId --jq '.[0].databaseId') +BRANCH=$(git rev-parse --abbrev-ref HEAD) +RUN_ID=$(gh run list -b "$BRANCH" -w CI -L 1 --json databaseId --jq '.[0].databaseId') if [ -z "$RUN_ID" ]; then echo "No action runs found for this branch" From 11e7cbf0ed392d39ba73956daa34ddfe66cfae7a Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 7 Sep 2023 10:45:55 -0500 Subject: [PATCH 7/8] try giving the file upload slightly more time --- app/test/e2e/image-upload.e2e.ts | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/app/test/e2e/image-upload.e2e.ts b/app/test/e2e/image-upload.e2e.ts index 8456d7ee68..97323d6a1e 100644 --- a/app/test/e2e/image-upload.e2e.ts +++ b/app/test/e2e/image-upload.e2e.ts @@ -12,7 +12,7 @@ import { MiB } from '@oxide/util' import { expectNotVisible, expectRowVisible, expectVisible, sleep } from './utils' -async function chooseFile(page: Page, size = 10 * MiB) { +async function chooseFile(page: Page, size = 15 * MiB) { const fileChooserPromise = page.waitForEvent('filechooser') await page.getByText('Image file', { exact: true }).click() const fileChooser = await fileChooserPromise @@ -215,12 +215,10 @@ test.describe('Image upload', () => { await expect(progressModal).toBeHidden() // form's back - await expectVisible(page, [ - 'role=textbox[name="Name"]', - // need to wait for submit button to come back because it's in a loading - // state while the cleanup runs - 'role=button[name="Upload image"]', - ]) + await expect(page.getByRole('textbox', { name: 'Name' })).toBeVisible() + // need to wait for submit button to come back because it's in a loading + // state while the cleanup runs + await expect(page.getByRole('button', { name: 'Upload image' })).toBeVisible() // resubmit and it should work fine await page.getByRole('button', { name: 'Upload image' }).click() From 0ad744d81772b2681eaf10ebb1e439f221f071df Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 7 Sep 2023 11:23:55 -0500 Subject: [PATCH 8/8] split click through networking test in two so safari might not be such a BABY about it --- app/test/e2e/networking.e2e.ts | 28 +++++++--------------------- 1 file changed, 7 insertions(+), 21 deletions(-) diff --git a/app/test/e2e/networking.e2e.ts b/app/test/e2e/networking.e2e.ts index 169b22b9b3..a71c7a00cb 100644 --- a/app/test/e2e/networking.e2e.ts +++ b/app/test/e2e/networking.e2e.ts @@ -5,11 +5,11 @@ * * Copyright Oxide Computer Company */ -import { test } from '@playwright/test' +import { expect, test } from '@playwright/test' import { expectNotVisible, expectVisible } from './utils' -test('Click through networking', async ({ page }) => { +test('Create and edit VPC', async ({ page }) => { await page.goto('/projects/mock-project') await page.click('role=link[name*="Networking"]') @@ -48,13 +48,15 @@ test('Click through networking', async ({ page }) => { // Close toast, it holds up the test for some reason await page.click('role=button[name="Dismiss notification"]') - await expectVisible(page, ['role=link[name="new-vpc"]']) + await expect(page.getByRole('link', { name: 'new-vpc' })).toBeVisible() +}) - await page.click('role=link[name="new-vpc"]') +test('Create and edit subnet', async ({ page }) => { + await page.goto('/projects/mock-project/vpcs/mock-vpc') // VPC detail, subnets tab await expectVisible(page, [ - 'role=heading[name*="new-vpc"]', + 'role=heading[name*="mock-vpc"]', 'role=tab[name="Subnets"]', // 'role=tab[name="System Routes"]', // 'role=tab[name="Routers"]', @@ -92,22 +94,6 @@ test('Click through networking', async ({ page }) => { await expectNotVisible(page, ['role=cell[name="new-subnet"]']) await expectVisible(page, ['role=cell[name="edited-subnet"]']) - // // System routes - // await page.click('role=tab[name="System Routes"]') - // await expectVisible(page, ['role=cell[name="system"]']) - - // // Routers - // await page.click('role=tab[name="Routers"]') - // await expectVisible(page, ['role=cell[name="system"] >> nth=0']) - // await page.click('role=button[name="New router"]') - // await expectVisible(page, [ - // 'role=heading[name="Create VPC Router"]', - // 'role=button[name="Create VPC Router"]', - // ]) - // await page.fill('role=textbox[name="Name"]', 'new-router') - // await page.click('role=button[name="Create VPC Router"]') - // await expectVisible(page, ['role=cell[name="new-router"]', 'role=cell[name="custom"]']) - // Firewall rules await page.click('role=tab[name="Firewall Rules"]') await expectVisible(page, [