From d85df4ea6522f97068196a452dd2045bea6079ae Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 25 Jul 2022 14:48:48 -0500 Subject: [PATCH 1/7] stub out basics --- .../project/instances/instance/tabs/NetworkingTab.tsx | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/app/pages/project/instances/instance/tabs/NetworkingTab.tsx b/app/pages/project/instances/instance/tabs/NetworkingTab.tsx index 4f17d6bed..c5281fe23 100644 --- a/app/pages/project/instances/instance/tabs/NetworkingTab.tsx +++ b/app/pages/project/instances/instance/tabs/NetworkingTab.tsx @@ -39,6 +39,16 @@ const SubnetNameFromId = ({ value }: { value: string }) => ( ) +function ExternalIpsFromInstanceName({ value: instanceName }: { value: string }) { + const { orgName, projectName } = useParams('orgName', 'projectName') + const { data } = useApiQuery('instanceExternalIpList', { + orgName, + projectName, + instanceName, + }) + return {data?.items.map((eip) => eip.ip).join(', ')} +} + export function NetworkingTab() { const instanceParams = useParams('orgName', 'projectName', 'instanceName') const queryClient = useApiQueryClient() @@ -121,6 +131,7 @@ export function NetworkingTab() { + From ffff5a460f4eb0f70038c4cfa3af5812ae9f6161 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 25 Jul 2022 17:48:51 -0500 Subject: [PATCH 2/7] display external IPs on primary network interface --- OMICRON_VERSION | 2 +- .../__tests__/instance/networking.e2e.ts | 4 +++- .../instances/instance/tabs/NetworkingTab.tsx | 20 ++++++++++--------- libs/api-mocks/msw/handlers.ts | 16 +++++++++++++++ libs/api/__generated__/Api.ts | 16 ++++++++++++++- libs/api/__generated__/OMICRON_VERSION | 2 +- 6 files changed, 47 insertions(+), 13 deletions(-) diff --git a/OMICRON_VERSION b/OMICRON_VERSION index d63482b80..57054d151 100644 --- a/OMICRON_VERSION +++ b/OMICRON_VERSION @@ -1 +1 @@ -604a35be03783f1506519f7652557626347343be +b9f6c1d35e3901e7ecd2415f3c42ea1bc6ca246c diff --git a/app/pages/__tests__/instance/networking.e2e.ts b/app/pages/__tests__/instance/networking.e2e.ts index 915d2b2ef..479031545 100644 --- a/app/pages/__tests__/instance/networking.e2e.ts +++ b/app/pages/__tests__/instance/networking.e2e.ts @@ -13,6 +13,7 @@ test('Instance networking tab', async ({ page }) => { 'my-nic', 'a network interface', '172.30.0.10', + '123.4.56.7', 'mock-vpc', 'mock-subnet', 'primary', @@ -58,11 +59,12 @@ test('Instance networking tab', async ({ page }) => { 'my-nic', 'a network interface', '172.30.0.10', + '—', 'mock-vpc', 'mock-subnet', '', ]) - await expectRowVisible(page, 'nic-2', ['nic-2', null, null, null, null, 'primary']) + await expectRowVisible(page, 'nic-2', ['nic-2', null, null, null, null, null, 'primary']) // Make an edit to the network interface await page diff --git a/app/pages/project/instances/instance/tabs/NetworkingTab.tsx b/app/pages/project/instances/instance/tabs/NetworkingTab.tsx index c5281fe23..dfd7b2be2 100644 --- a/app/pages/project/instances/instance/tabs/NetworkingTab.tsx +++ b/app/pages/project/instances/instance/tabs/NetworkingTab.tsx @@ -39,14 +39,11 @@ const SubnetNameFromId = ({ value }: { value: string }) => ( ) -function ExternalIpsFromInstanceName({ value: instanceName }: { value: string }) { - const { orgName, projectName } = useParams('orgName', 'projectName') - const { data } = useApiQuery('instanceExternalIpList', { - orgName, - projectName, - instanceName, - }) - return {data?.items.map((eip) => eip.ip).join(', ')} +function ExternalIpsFromInstanceName({ value: primary }: { value: boolean }) { + const instanceParams = useParams('orgName', 'projectName', 'instanceName') + const { data } = useApiQuery('instanceExternalIpList', instanceParams) + const ips = data?.items.map((eip) => eip.ip).join(', ') + return {primary ? ips : <>—} } export function NetworkingTab() { @@ -129,9 +126,14 @@ export function NetworkingTab() { {/* TODO: mark v4 or v6 explicitly? */} + - diff --git a/libs/api-mocks/msw/handlers.ts b/libs/api-mocks/msw/handlers.ts index 753d22314..8af9054d2 100644 --- a/libs/api-mocks/msw/handlers.ts +++ b/libs/api-mocks/msw/handlers.ts @@ -462,6 +462,22 @@ export const handlers = [ } ), + rest.get | GetErr>( + '/api/organizations/:orgName/projects/:projectName/instances/:instanceName/external-ips', + (req, res) => { + const [, err] = lookupInstance(req.params) + if (err) return res(err) + // TODO: proper mock table + const items = [ + { + ip: '123.4.56.7', + kind: 'ephemeral', + } as const, + ] + return res(json({ items })) + } + ), + rest.get | GetErr>( '/api/organizations/:orgName/projects/:projectName/instances/:instanceName/network-interfaces', (req, res) => { diff --git a/libs/api/__generated__/Api.ts b/libs/api/__generated__/Api.ts index 343e784c7..4bed8ee3f 100644 --- a/libs/api/__generated__/Api.ts +++ b/libs/api/__generated__/Api.ts @@ -180,6 +180,20 @@ export type ExternalIp = { */ export type ExternalIpCreate = { poolName?: Name | null; type: 'ephemeral' } +/** + * A single page of results + */ +export type ExternalIpResultsPage = { + /** + * list of items on this page of results + */ + items: ExternalIp[] + /** + * token used to fetch the next page of results (if any) + */ + nextPage?: string | null +} + /** * The name and type information for a field of a timeseries schema. */ @@ -3738,7 +3752,7 @@ export class Api extends HttpClient { { instanceName, orgName, projectName }: InstanceExternalIpListParams, params: RequestParams = {} ) => - this.request({ + this.request({ path: `/organizations/${orgName}/projects/${projectName}/instances/${instanceName}/external-ips`, method: 'GET', ...params, diff --git a/libs/api/__generated__/OMICRON_VERSION b/libs/api/__generated__/OMICRON_VERSION index 7de6b63b3..b6a0e8842 100644 --- a/libs/api/__generated__/OMICRON_VERSION +++ b/libs/api/__generated__/OMICRON_VERSION @@ -1,2 +1,2 @@ # generated file. do not update manually. see docs/update-pinned-api.md -604a35be03783f1506519f7652557626347343be +b9f6c1d35e3901e7ecd2415f3c42ea1bc6ca246c From 95fd19a91a94d95821c8842a589c86c79a860a25 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 26 Jul 2022 11:18:38 -0500 Subject: [PATCH 3/7] assert rows with an object instead of an array --- .../__tests__/instance/networking.e2e.ts | 24 ++------ app/pages/__tests__/org-access.e2e.ts | 18 +++--- app/pages/__tests__/project-access.e2e.ts | 19 +++--- app/pages/__tests__/ssh-keys.e2e.ts | 3 +- app/util/e2e.ts | 58 ++++++++++++------- 5 files changed, 64 insertions(+), 58 deletions(-) diff --git a/app/pages/__tests__/instance/networking.e2e.ts b/app/pages/__tests__/instance/networking.e2e.ts index 479031545..8b12937ea 100644 --- a/app/pages/__tests__/instance/networking.e2e.ts +++ b/app/pages/__tests__/instance/networking.e2e.ts @@ -9,15 +9,9 @@ test('Instance networking tab', async ({ page }) => { // Instance networking tab await page.click('role=tab[name="Networking"]') - await expectRowVisible(page, 'my-nic', [ - 'my-nic', - 'a network interface', - '172.30.0.10', - '123.4.56.7', - 'mock-vpc', - 'mock-subnet', - 'primary', - ]) + + const table = page.locator('table') + await expectRowVisible(table, { name: 'my-nic', primary: 'primary' }) // check VPC link in table points to the right page await expect(page.locator('role=cell >> role=link[name="mock-vpc"]')).toHaveAttribute( @@ -55,16 +49,8 @@ test('Instance networking tab', async ({ page }) => { .locator('role=button[name="Row actions"]') .click() await page.click('role=menuitem[name="Make primary"]') - await expectRowVisible(page, 'my-nic', [ - 'my-nic', - 'a network interface', - '172.30.0.10', - '—', - 'mock-vpc', - 'mock-subnet', - '', - ]) - await expectRowVisible(page, 'nic-2', ['nic-2', null, null, null, null, null, 'primary']) + await expectRowVisible(table, { name: 'my-nic', primary: '' }) + await expectRowVisible(table, { name: 'nic-2', primary: 'primary' }) // Make an edit to the network interface await page diff --git a/app/pages/__tests__/org-access.e2e.ts b/app/pages/__tests__/org-access.e2e.ts index effd8b03a..582ab1739 100644 --- a/app/pages/__tests__/org-access.e2e.ts +++ b/app/pages/__tests__/org-access.e2e.ts @@ -5,13 +5,15 @@ import { expectNotVisible, expectRowVisible, expectVisible } from 'app/util/e2e' test('Click through org access page', async ({ page }) => { await page.goto('/orgs/maze-war') - // page is there, we see AL but not FDR + const table = page.locator('role=table') + + // page is there, we see user 1 but not 2 await page.click('role=link[name*="Access & IAM"]') await expectVisible(page, ['role=heading[name*="Access & IAM"]']) - await expectRowVisible(page, 'user-1', ['user-1', 'Hannah Arendt', 'admin']) + await expectRowVisible(table, { ID: 'user-1', Name: 'Hannah Arendt', Role: 'admin' }) await expectNotVisible(page, ['role=cell[name="user-2"]']) - // Add FDR as collab + // Add user 2 as collab await page.click('role=button[name="Add user to organization"]') await expectVisible(page, ['role=heading[name*="Add user to organization"]']) @@ -32,10 +34,10 @@ test('Click through org access page', async ({ page }) => { await page.click('role=option[name="Collaborator"]') await page.click('role=button[name="Add user"]') - // FDR shows up in the table - await expectRowVisible(page, 'user-2', ['user-2', 'Hans Jonas', 'collaborator']) + // User 2 shows up in the table + await expectRowVisible(table, { ID: 'user-2', Name: 'Hans Jonas', Role: 'collaborator' }) - // now change FDR's role from collab to viewer + // now change user 2's role from collab to viewer await page .locator('role=row', { hasText: 'user-2' }) .locator('role=button[name="Row actions"]') @@ -49,9 +51,9 @@ test('Click through org access page', async ({ page }) => { await page.click('role=option[name="Viewer"]') await page.click('role=button[name="Update role"]') - await expectRowVisible(page, 'user-2', ['user-2', 'Hans Jonas', 'viewer']) + await expectRowVisible(table, { ID: 'user-2', Role: 'viewer' }) - // now delete FDR + // now delete user 2 await page .locator('role=row', { hasText: 'user-2' }) .locator('role=button[name="Row actions"]') diff --git a/app/pages/__tests__/project-access.e2e.ts b/app/pages/__tests__/project-access.e2e.ts index 8cc5f3695..177760d28 100644 --- a/app/pages/__tests__/project-access.e2e.ts +++ b/app/pages/__tests__/project-access.e2e.ts @@ -4,14 +4,15 @@ import { expectNotVisible, expectRowVisible, expectVisible } from 'app/util/e2e' test('Click through project access page', async ({ page }) => { await page.goto('/orgs/maze-war/projects/mock-project') - - // page is there, we see AL but not FDR await page.click('role=link[name*="Access & IAM"]') + + // page is there, we see user 1 but not 2 await expectVisible(page, ['role=heading[name*="Access & IAM"]']) - await expectRowVisible(page, 'user-1', ['user-1', 'Hannah Arendt', 'admin']) + const table = page.locator('table') + await expectRowVisible(table, { ID: 'user-1', Name: 'Hannah Arendt', Role: 'admin' }) await expectNotVisible(page, ['role=cell[name="user-2"]']) - // Add FDR as collab + // Add user 2 as collab await page.click('role=button[name="Add user to project"]') await expectVisible(page, ['role=heading[name*="Add user to project"]']) @@ -32,10 +33,10 @@ test('Click through project access page', async ({ page }) => { await page.click('role=option[name="Collaborator"]') await page.click('role=button[name="Add user"]') - // FDR shows up in the table - await expectRowVisible(page, 'user-2', ['user-2', 'Hans Jonas', 'collaborator']) + // User 2 shows up in the table + await expectRowVisible(table, { ID: 'user-2', Name: 'Hans Jonas', Role: 'collaborator' }) - // now change FDR's role from collab to viewer + // now change user 2 role from collab to viewer await page .locator('role=row', { hasText: 'user-2' }) .locator('role=button[name="Row actions"]') @@ -49,9 +50,9 @@ test('Click through project access page', async ({ page }) => { await page.click('role=option[name="Viewer"]') await page.click('role=button[name="Update role"]') - await expectRowVisible(page, 'user-2', ['user-2', 'Hans Jonas', 'viewer']) + await expectRowVisible(table, { ID: 'user-2', Role: 'viewer' }) - // now delete FDR + // now delete user 2 await page .locator('role=row', { hasText: 'user-2' }) .locator('role=button[name="Row actions"]') diff --git a/app/pages/__tests__/ssh-keys.e2e.ts b/app/pages/__tests__/ssh-keys.e2e.ts index 45240c237..ebccd913c 100644 --- a/app/pages/__tests__/ssh-keys.e2e.ts +++ b/app/pages/__tests__/ssh-keys.e2e.ts @@ -18,7 +18,8 @@ test('SSH keys', async ({ page }) => { // it's there in the table await expectNotVisible(page, ['text="No SSH keys"']) - await expectRowVisible(page, 'my-key', ['my-key', 'definitely a key']) + const table = page.locator('role=table') + await expectRowVisible(table, { Name: 'my-key', Description: 'definitely a key' }) // now delete it await page.click('role=button[name="Row actions"]') diff --git a/app/util/e2e.ts b/app/util/e2e.ts index c411fe636..140952240 100644 --- a/app/util/e2e.ts +++ b/app/util/e2e.ts @@ -1,13 +1,24 @@ import type { Locator, Page } from '@playwright/test' import { expect } from '@playwright/test' -export async function forEach(loc: Locator, fn: (loc0: Locator) => void) { +export async function forEach(loc: Locator, fn: (loc0: Locator, i: number) => void) { const count = await loc.count() for (let i = 0; i < count; i++) { - await fn(loc.nth(i)) + await fn(loc.nth(i), i) } } +export async function map( + loc: Locator, + fn: (loc0: Locator, i: number) => Promise +): Promise { + const result: T[] = [] + await forEach(loc, async (loc0, i) => { + result.push(await fn(loc0, i)) + }) + return result +} + export async function expectVisible(page: Page, selectors: string[]) { for (const selector of selectors) { await expect(page.locator(selector)).toBeVisible() @@ -21,26 +32,31 @@ export async function expectNotVisible(page: Page, selectors: string[]) { } /** - * Assert about the values of a row, identified by `rowSelectorText`. It doesn't - * need to be the entire row; the test will pass as long as the identified row - * exists and the first N cells match the N values in `cellTexts`. Pass `''` for - * a checkbox cell. - * - * @param rowSelectorText Text that should uniquely identify the row, like an ID - * @param cellTexts Text to match in each cell of that row + * Assert that a row matching `expectedRow` is present in `table`. The match + * uses `objectContaining`, so `expectedRow` does not need to contain every + * cell. Works by converting `table` to a list of objects where the keys are + * header cell text and the values are row cell text. */ export async function expectRowVisible( - page: Page, - rowSelectorText: string, - cellTexts: Array + table: Locator, + expectedRow: Record ) { - const row = page.locator(`tr:has-text("${rowSelectorText}")`) - await expect(row).toBeVisible() - for (let i = 0; i < cellTexts.length; i++) { - const text = cellTexts[i] - if (text === null) { - continue - } - await expect(row.locator(`role=cell >> nth=${i}`)).toHaveText(text) - } + await table.waitFor() // sometimes the table is re-rendering and the tests flake + + const headerKeys = await map( + table.locator('thead >> role=cell'), + async (cell) => await cell.textContent() + ) + + const rows = await map(table.locator('tbody >> role=row'), async (row) => { + const rowPairs = await map(row.locator('role=cell'), async (cell, i) => [ + headerKeys[i], + // accessible name would be better but it's not in yet + // https://github.com/microsoft/playwright/issues/13517 + await cell.textContent(), + ]) + return Object.fromEntries(rowPairs.filter(([k]) => k && k.length > 0)) + }) + + await expect(rows).toEqual(expect.arrayContaining([expect.objectContaining(expectedRow)])) } From ec5e75dca030f3e4be7353db04cb4bcf4042e6f5 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 26 Jul 2022 13:36:02 -0500 Subject: [PATCH 4/7] point to omicron main now that the ResultsPage change is merged --- OMICRON_VERSION | 2 +- libs/api/__generated__/Api.ts | 36 +------------------------- libs/api/__generated__/OMICRON_VERSION | 2 +- 3 files changed, 3 insertions(+), 37 deletions(-) diff --git a/OMICRON_VERSION b/OMICRON_VERSION index 57054d151..f5244aa2a 100644 --- a/OMICRON_VERSION +++ b/OMICRON_VERSION @@ -1 +1 @@ -b9f6c1d35e3901e7ecd2415f3c42ea1bc6ca246c +6d8d6a4580db44a885f7a213aa39ada76d8af2d6 diff --git a/libs/api/__generated__/Api.ts b/libs/api/__generated__/Api.ts index 4bed8ee3f..9282f929a 100644 --- a/libs/api/__generated__/Api.ts +++ b/libs/api/__generated__/Api.ts @@ -2834,15 +2834,13 @@ export interface FullRequestParams extends Omit { body?: unknown /** base url */ baseUrl?: string - /** request cancellation token */ - cancelToken?: CancelToken } export type RequestParams = Omit export interface ApiConfig { baseUrl?: string - baseApiParams?: Omit + baseApiParams?: Omit customFetch?: typeof fetch } @@ -2863,8 +2861,6 @@ export type SuccessResponse = Response & { export type ApiResponse = SuccessResponse | ErrorResponse -type CancelToken = Symbol | string | number - const encodeQueryParam = (key: string, value: any) => `${encodeURIComponent(camelToSnake(key))}=${encodeURIComponent(value)}` @@ -2880,7 +2876,6 @@ const toQueryString = (rawQuery?: QueryParamsType): string => export class HttpClient { public baseUrl: string = '' - private abortControllers = new Map() private customFetch = (...fetchParams: Parameters) => fetch(...fetchParams) private baseApiParams: RequestParams = { @@ -2905,35 +2900,11 @@ export class HttpClient { } } - private createAbortSignal = (cancelToken: CancelToken): AbortSignal | undefined => { - if (this.abortControllers.has(cancelToken)) { - const abortController = this.abortControllers.get(cancelToken) - if (abortController) { - return abortController.signal - } - return void 0 - } - - const abortController = new AbortController() - this.abortControllers.set(cancelToken, abortController) - return abortController.signal - } - - public abortRequest = (cancelToken: CancelToken) => { - const abortController = this.abortControllers.get(cancelToken) - - if (abortController) { - abortController.abort() - this.abortControllers.delete(cancelToken) - } - } - public request = async ({ body, path, query, baseUrl, - cancelToken, ...params }: FullRequestParams): Promise> => { const requestParams = this.mergeRequestParams(params) @@ -2951,7 +2922,6 @@ export class HttpClient { 'Content-Type': 'application/json', ...requestParams.headers, }, - signal: cancelToken ? this.createAbortSignal(cancelToken) : void 0, body: JSON.stringify(snakeify(body)), }) @@ -2970,10 +2940,6 @@ export class HttpClient { r.error = e as Error } - if (cancelToken) { - this.abortControllers.delete(cancelToken) - } - if (!r.ok) throw r return r } diff --git a/libs/api/__generated__/OMICRON_VERSION b/libs/api/__generated__/OMICRON_VERSION index b6a0e8842..e43aa56fe 100644 --- a/libs/api/__generated__/OMICRON_VERSION +++ b/libs/api/__generated__/OMICRON_VERSION @@ -1,2 +1,2 @@ # generated file. do not update manually. see docs/update-pinned-api.md -b9f6c1d35e3901e7ecd2415f3c42ea1bc6ca246c +6d8d6a4580db44a885f7a213aa39ada76d8af2d6 From 3c8eafa6ba271b81f17f182fc44f8c0342d2d9c0 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 26 Jul 2022 14:11:12 -0500 Subject: [PATCH 5/7] attempt to steer clear of flake town --- app/util/e2e.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/util/e2e.ts b/app/util/e2e.ts index 140952240..1fddebe10 100644 --- a/app/util/e2e.ts +++ b/app/util/e2e.ts @@ -41,7 +41,12 @@ export async function expectRowVisible( table: Locator, expectedRow: Record ) { - await table.waitFor() // sometimes the table is re-rendering and the tests flake + // wait for header and rows to avoid flake town + const headerLoc = table.locator('thead >> role=cell') + await headerLoc.locator('nth=0').waitFor() // nth=0 bc error if there's more than 1 + + const rowLoc = table.locator('tbody >> role=row') + await rowLoc.locator('nth=0').waitFor() const headerKeys = await map( table.locator('thead >> role=cell'), From ea6973b053c21fc11a29c51fd3afcae590f1f495 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 26 Jul 2022 14:28:19 -0500 Subject: [PATCH 6/7] do the unthinkable. extend the timeout --- playwright.config.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/playwright.config.ts b/playwright.config.ts index 9da957ae4..81e57ef3f 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -15,7 +15,7 @@ const config: PlaywrightTestConfig = { workers: process.env.CI ? 1 : undefined, use: { /* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */ - actionTimeout: 10000, + actionTimeout: 20000, baseURL: 'http://localhost:4009', trace: 'on-first-retry', }, From 72f5d22f3319bbc3d08e373da457530d41b58b69 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 26 Jul 2022 14:35:09 -0500 Subject: [PATCH 7/7] try 3 workers and put the timeout back to 10s --- .github/workflows/lintBuildTest.yml | 2 +- playwright.config.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/lintBuildTest.yml b/.github/workflows/lintBuildTest.yml index ad5246112..d7efcd9c8 100644 --- a/.github/workflows/lintBuildTest.yml +++ b/.github/workflows/lintBuildTest.yml @@ -46,4 +46,4 @@ jobs: - name: Install Playwright run: npx playwright install --with-deps - name: Run Playwright tests - run: yarn playwright test + run: yarn playwright test --workers=3 diff --git a/playwright.config.ts b/playwright.config.ts index 81e57ef3f..9da957ae4 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -15,7 +15,7 @@ const config: PlaywrightTestConfig = { workers: process.env.CI ? 1 : undefined, use: { /* Maximum time each action such as `click()` can take. Defaults to 0 (no limit). */ - actionTimeout: 20000, + actionTimeout: 10000, baseURL: 'http://localhost:4009', trace: 'on-first-retry', },