From 58584c450e3647436094b5d346ac809918171249 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 12 Aug 2026 14:31:59 +0200 Subject: [PATCH 1/8] Use App Management API instead of browser for E2E teardown checks The E2E teardown drove the Dev Dashboard and store admin UI for work the App Management API can answer directly: - App lookup: replace the browser pagination in findAppOnDevDashboard with appByKey (client_id) and an appsConnection title search fallback. - Install gating: replace the isStoreAppsEmpty page scrape and the disabled-Delete-button probing with installCount polling. The same check gates both store deletion and app deletion. - Store uninstall: drop the browser click-through fallback; the Admin API path (from #8309) is now the only one in teardown. The browser is only used for the final delete-app click, which has no API mutation. The cleanup scripts keep their browser paths: org-wide sweeps have no local app dir to mint Admin API tokens from. API calls reuse the worker's CLI session via cli-kit, following the cleanup-stores.ts pattern. Co-Authored-By: Claude Fable 5 --- packages/e2e/setup/app-management-api.ts | 164 +++++++++++++++ packages/e2e/setup/app.ts | 38 +--- packages/e2e/setup/teardown.ts | 209 ++++++++++---------- packages/e2e/tests/app-deploy.spec.ts | 6 +- packages/e2e/tests/app-dev-server.spec.ts | 3 +- packages/e2e/tests/app-scaffold.spec.ts | 9 +- packages/e2e/tests/dev-hot-reload.spec.ts | 9 +- packages/e2e/tests/multi-config-dev.spec.ts | 6 +- packages/e2e/tests/toml-config.spec.ts | 6 +- 9 files changed, 286 insertions(+), 164 deletions(-) create mode 100644 packages/e2e/setup/app-management-api.ts diff --git a/packages/e2e/setup/app-management-api.ts b/packages/e2e/setup/app-management-api.ts new file mode 100644 index 00000000000..f2e3283492f --- /dev/null +++ b/packages/e2e/setup/app-management-api.ts @@ -0,0 +1,164 @@ +/* eslint-disable no-restricted-globals, no-await-in-loop, @nx/enforce-module-boundaries -- the + harness calls the App Management API directly with minimal queries, like admin-api.ts + does for the Admin API; cli-kit is only used to reuse the CLI session that + global setup created, following the cleanup-stores.ts pattern */ +import {loadtestHeaderRecord} from '../helpers/loadtest-header.js' +import {ensureAuthenticatedAppManagementAndBusinessPlatform} from '../../cli-kit/dist/public/node/session.js' + +const APP_MANAGEMENT_URL = 'https://app.shopify.com/app_management/unstable/graphql.json' + +/** Minimal app identity as returned by the App Management API. */ +export interface AppManagementApp { + /** GID, e.g. gid://organization/App/123 — input for install-count lookups. */ + id: string + /** API key (the app's client_id) — last segment of the Dev Dashboard app URL. */ + key: string +} + +const XDG_KEYS = ['XDG_DATA_HOME', 'XDG_CONFIG_HOME', 'XDG_STATE_HOME', 'XDG_CACHE_HOME'] as const + +let sessionEnvReady = false + +/** + * Point cli-kit's session storage at an authenticated CLI session before its + * first use in this process. cli-kit resolves its storage directory from + * process.env at first access and caches it, so this runs once and the dirs + * must not change afterwards — workers keep one XDG dir set for their lifetime. + */ +function ensureSessionEnv(sessionEnv: NodeJS.ProcessEnv): void { + if (sessionEnvReady) return + for (const key of XDG_KEYS) { + const value = sessionEnv[key] + if (value) process.env[key] = value + } + sessionEnvReady = true +} + +let cachedToken: string | undefined + +async function appManagementToken(sessionEnv: NodeJS.ProcessEnv, forceRefresh = false): Promise { + ensureSessionEnv(sessionEnv) + if (cachedToken && !forceRefresh) return cachedToken + const {appManagementToken: token} = await ensureAuthenticatedAppManagementAndBusinessPlatform({ + noPrompt: true, + forceRefresh, + }) + // eslint-disable-next-line require-atomic-updates + cachedToken = token + return token +} + +interface GraphQLPayload { + errors?: {message?: string}[] + data?: unknown +} + +/** + * Run a GraphQL query against the App Management API using the CLI session + * from `sessionEnv`. Retries once with a refreshed token on 401. + */ +async function appManagementQuery( + sessionEnv: NodeJS.ProcessEnv, + query: string, + variables: {[key: string]: string}, +): Promise { + let token = await appManagementToken(sessionEnv) + + for (let attempt = 1; ; attempt++) { + const response = await fetch(APP_MANAGEMENT_URL, { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Authorization: `Bearer ${token}`, + ...loadtestHeaderRecord(), + }, + body: JSON.stringify({query, variables}), + }) + + if (response.status === 401 && attempt === 1) { + token = await appManagementToken(sessionEnv, true) + continue + } + if (!response.ok) { + throw new Error(`App Management API request failed (status ${response.status}): ${await response.text()}`) + } + + const payload = (await response.json()) as GraphQLPayload + if (payload.errors?.length) { + throw new Error(`App Management API returned errors: ${payload.errors.map((error) => error.message).join(', ')}`) + } + return payload.data + } +} + +/** + * Look up an app by its API key (client_id). Returns undefined when the app + * does not exist — for teardown that means it was already deleted. + */ +export async function findAppByClientId( + sessionEnv: NodeJS.ProcessEnv, + clientId: string, +): Promise { + const data = (await appManagementQuery( + sessionEnv, + 'query appByKey($key: String!) { appByKey(key: $key) { id key } }', + { + key: clientId, + }, + )) as {appByKey?: AppManagementApp | null} + return data.appByKey ?? undefined +} + +/** + * Search the org's apps by exact name. Returns undefined when no app matches. + * + * `organizationId` is not declared in the operation on purpose: the App + * Management API reads it from the variables map for request routing, exactly + * as the CLI's own `appsForOrg` does. + */ +export async function findAppByName( + sessionEnv: NodeJS.ProcessEnv, + appName: string, + orgId: string, +): Promise { + const data = (await appManagementQuery( + sessionEnv, + 'query listApps($query: String) { appsConnection(query: $query, first: 50) { edges { node { id key activeRelease { version { name } } } } } }', + {query: `title:${appName}`, organizationId: orgId}, + )) as { + appsConnection?: {edges: {node: AppManagementApp & {activeRelease: {version: {name: string}}}}[]} | null + } + return data.appsConnection?.edges.map((edge) => edge.node).find((node) => node.activeRelease.version.name === appName) +} + +/** Total install count for an app across all stores. */ +export async function appInstallCount(sessionEnv: NodeJS.ProcessEnv, appId: string): Promise { + const data = (await appManagementQuery( + sessionEnv, + 'query AppInstallCount($appId: ID!) { app(id: $appId) { installCount } }', + {appId}, + )) as {app?: {installCount?: number | null} | null} + return data.app?.installCount ?? 0 +} + +/** + * Poll until the app reports zero installs. Uninstall records clear + * asynchronously after an Admin API uninstall, usually within seconds. + * Returns false when installs remain after the timeout. + */ +export async function waitForZeroInstalls( + sessionEnv: NodeJS.ProcessEnv, + appId: string, + options: {timeoutMs?: number; pollIntervalMs?: number} = {}, +): Promise { + const timeoutMs = options.timeoutMs ?? 30_000 + const pollIntervalMs = options.pollIntervalMs ?? 2_000 + const deadline = Date.now() + timeoutMs + + while ((await appInstallCount(sessionEnv, appId)) > 0) { + if (Date.now() >= deadline) return false + + await new Promise((resolve) => setTimeout(resolve, pollIntervalMs)) + } + return true +} diff --git a/packages/e2e/setup/app.ts b/packages/e2e/setup/app.ts index 1f8a3b94fb4..7df1077a2e5 100644 --- a/packages/e2e/setup/app.ts +++ b/packages/e2e/setup/app.ts @@ -1,6 +1,6 @@ -/* eslint-disable no-restricted-imports, no-await-in-loop */ +/* eslint-disable no-restricted-imports */ import {authFixture} from './auth.js' -import {getLastPageStatus, isVisibleWithin, navigateToDashboard, refreshIfPageError} from './browser.js' +import {getLastPageStatus, isVisibleWithin} from './browser.js' import {CLI_TIMEOUT, BROWSER_TIMEOUT} from './constants.js' import * as toml from '@iarna/toml' import * as path from 'path' @@ -320,41 +320,9 @@ export async function configLink( } // --------------------------------------------------------------------------- -// Dev dashboard browser actions — find and delete apps +// Dev dashboard browser actions — delete apps // --------------------------------------------------------------------------- -/** Search dev dashboard for an app by name. Returns the app URL or null. */ -export async function findAppOnDevDashboard(page: Page, appName: string, orgId?: string): Promise { - const org = orgId ?? (process.env.E2E_ORG_ID ?? '').trim() - const email = process.env.E2E_ACCOUNT_EMAIL - - await navigateToDashboard({browserPage: page, email, orgId: org}) - - // Scan current page + pagination for the app - while (true) { - const allLinks = await page.locator('a[href*="/apps/"]').all() - for (const link of allLinks) { - const text = (await link.textContent()) ?? '' - if (text.includes(appName)) { - const href = await link.getAttribute('href') - if (href) return href.startsWith('http') ? href : `https://dev.shopify.com${href}` - } - } - - // Check for next page - const nextLink = page.locator('a[href*="next_cursor"]').first() - if (!(await isVisibleWithin(nextLink, BROWSER_TIMEOUT.medium))) break - const nextHref = await nextLink.getAttribute('href') - if (!nextHref) break - const nextUrl = nextHref.startsWith('http') ? nextHref : `https://dev.shopify.com${nextHref}` - await page.goto(nextUrl, {waitUntil: 'domcontentloaded'}) - await page.waitForTimeout(BROWSER_TIMEOUT.medium) - await refreshIfPageError(page) - } - - return null -} - /** * Delete an app from its dev dashboard settings page. Returns true if deleted. * diff --git a/packages/e2e/setup/teardown.ts b/packages/e2e/setup/teardown.ts index fcfb0aa32f5..08806952080 100644 --- a/packages/e2e/setup/teardown.ts +++ b/packages/e2e/setup/teardown.ts @@ -1,119 +1,111 @@ /* eslint-disable no-await-in-loop */ import {uninstallAppWithAdminApi} from './admin-api.js' -import {findAppOnDevDashboard, deleteAppFromDevDashboard} from './app.js' -import {refreshIfPageError} from './browser.js' +import {findAppByClientId, findAppByName, appInstallCount, waitForZeroInstalls} from './app-management-api.js' +import {deleteAppFromDevDashboard, extractClientId} from './app.js' import {createLogger, e2eSection} from './env.js' import {BROWSER_TIMEOUT} from './constants.js' -import {uninstallAppFromStore, deleteDevStoreWithCli, isStoreAppsEmpty, dismissDevConsole} from './store.js' +import {deleteDevStoreWithCli} from './store.js' +import type {AppManagementApp} from './app-management-api.js' import type {CLIProcess} from './cli.js' +import type {E2EEnv} from './env.js' import type {Page} from '@playwright/test' -const log = createLogger('browser') +const log = createLogger('teardown') interface BaseTeardownCtx { browserPage: Page appName: string - /** Direct Dev Dashboard app URL. Prefer this when available to avoid slow org-wide pagination. */ + env: E2EEnv + /** Direct Dev Dashboard app URL. Prefer this when available to avoid an app search by name. */ appUrl?: string - /** Local app directory. When present, uninstall goes through the Admin API instead of the store admin UI. */ - appDir?: string - workerIndex?: number } type TeardownCtx = BaseTeardownCtx & - ({storeFqdn: string; orgId: string; cli: CLIProcess} | {storeFqdn?: undefined; orgId?: string; cli?: CLIProcess}) + ( + | {storeFqdn: string; cli: CLIProcess; appDir: string | undefined} + | {storeFqdn?: undefined; cli?: CLIProcess; appDir?: string} + ) /** * Best-effort per-test teardown. Each phase retries up to 3 times. * * App + store flow: - * Phase 1: uninstall app from store admin - * Phase 2: delete store (skipped if phase 1 failed) - * Phase 3: delete app from dev dashboard (skipped if phase 1 failed) + * Phase 1: uninstall app from store over the Admin API + * Phase 2: delete store (skipped until the app reports zero installs) + * Phase 3: delete app from dev dashboard (browser — the App Management API + * has no delete mutation) * * App-only flow: * Phase 3 only + * + * The app's identity and install state come from the App Management API; the + * browser is only used for the final delete click. */ export async function teardownAll(ctx: TeardownCtx): Promise { - const wCtx = {workerIndex: ctx.workerIndex ?? 0} - const page = ctx.browserPage + const wCtx = {workerIndex: ctx.env.workerIndex} + const sessionEnv = ctx.env.processEnv + + // Resolve the app via the App Management API. `undefined` app after a + // successful lookup means it does not exist (already deleted). + let app: AppManagementApp | undefined + let appResolved = false + const clientId = resolveClientId(ctx) + for (let attempt = 1; attempt <= 3; attempt++) { + try { + if (clientId) { + app = await findAppByClientId(sessionEnv, clientId) + } else if (ctx.env.orgId) { + app = await findAppByName(sessionEnv, ctx.appName, ctx.env.orgId) + } + appResolved = true + break + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (err) { + log.log(wCtx, `(${attempt}/3) app lookup failed: ${err instanceof Error ? err.message : err}`) + } + } - // Phase 1 + 2: Store cleanup (app+store tests only) + // Phase 1: Uninstall app from store over the Admin API (app+store tests + // only). No browser fallback: an API failure must surface loudly so it gets + // fixed instead of hiding behind a flaky store-admin click-through. if (ctx.storeFqdn) { - const storeSlug = ctx.storeFqdn.replace('.myshopify.com', '') e2eSection(wCtx, `Teardown: store ${ctx.storeFqdn}`) - - // Phase 1: Uninstall app from store — Admin API when the app dir is known. - // No browser fallback: an API failure must surface loudly so it gets fixed - // instead of hiding behind the flaky store-admin click-through. - let uninstalled = false if (ctx.appDir) { log.log(wCtx, 'uninstalling app via admin API') await uninstallAppWithAdminApi({cli: ctx.cli, appDir: ctx.appDir, storeFqdn: ctx.storeFqdn}) - uninstalled = true log.log(wCtx, 'app uninstalled via admin API') + } else { + // The test failed before createApp finished — nothing was installed. + log.log(wCtx, 'no app dir, skipping uninstall') } - if (!uninstalled) { - log.log(wCtx, 'uninstalling app from store') - for (let attempt = 1; attempt <= 3; attempt++) { - try { - uninstalled = await uninstallAppFromStore(page, storeSlug, ctx.appName) - if (uninstalled) { - log.log(wCtx, 'app uninstalled') - break - } - log.log(wCtx, `(${attempt}/3) app uninstall attempt failed, app still visible`) - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (err) { - log.log(wCtx, `(${attempt}/3) app uninstall attempt failed: ${err instanceof Error ? err.message : err}`) - } - } - } - if (!uninstalled) { - log.error(wCtx, 'app uninstall failed after 3 attempts') - } - - // Phase 2: Delete store - log.log(wCtx, 'deleting store') - let storeDeletionRequested = false - let safeToDelete = false - - // Gate: confirm the store has zero apps before attempting delete store. - try { - await page.goto(`https://admin.shopify.com/store/${storeSlug}/settings/apps`, { - waitUntil: 'domcontentloaded', - }) - await page.waitForTimeout(BROWSER_TIMEOUT.long) + } - if (page.url().includes('access_account')) { - log.log(wCtx, 'store already deleted') - storeDeletionRequested = true - } else { - await dismissDevConsole(page) - // Reload once in case the page is stale (Phase 1 just uninstalled) - if (!(await isStoreAppsEmpty(page))) { - await page.reload({waitUntil: 'domcontentloaded'}) - await page.waitForTimeout(BROWSER_TIMEOUT.long) - await dismissDevConsole(page) - } - if (await isStoreAppsEmpty(page)) { - safeToDelete = true - } else { - log.error(wCtx, 'store has apps installed, skipping delete') - } - } - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (err) { - log.error(wCtx, `store empty state unclear, skipping delete: ${err instanceof Error ? err.message : err}`) + // Install state gates both store deletion (an install record would strand + // the app in the Dev Dashboard) and app deletion (the Delete button stays + // disabled while installs exist, so the browser flow would just spin). + // Uninstall records clear asynchronously, hence the poll after uninstall. + let installsCleared = false + if (appResolved) { + if (!app) { + installsCleared = true + } else if (ctx.storeFqdn) { + installsCleared = await waitForZeroInstalls(sessionEnv, app.id) + } else { + installsCleared = (await appInstallCount(sessionEnv, app.id)) === 0 } + } - if (safeToDelete) { + // Phase 2: Delete store + if (ctx.storeFqdn) { + if (appResolved && installsCleared) { + log.log(wCtx, 'deleting store') + let storeDeletionRequested = false for (let attempt = 1; attempt <= 3; attempt++) { try { const deletionConfirmed = await deleteDevStoreWithCli({ cli: ctx.cli, storeFqdn: ctx.storeFqdn, - orgId: ctx.orgId, + orgId: ctx.env.orgId, }) log.log(wCtx, deletionConfirmed ? 'store deletion confirmed by CLI' : 'store deletion requested with CLI') storeDeletionRequested = true @@ -126,38 +118,33 @@ export async function teardownAll(ctx: TeardownCtx): Promise { if (!storeDeletionRequested) { log.error(wCtx, 'store deletion request failed after 3 attempts') } - } - - // Gate: confirm the store has zero apps before attempting delete app. - if (!uninstalled) { - log.log(wCtx, 'skipping app delete — uninstall failed, run `pnpm test:e2e-cleanup-apps` after') - return + } else { + const reason = appResolved ? 'app still reports installs' : 'install state unknown (app lookup failed)' + log.error(wCtx, `${reason}, skipping store delete`) } } // Phase 3: Delete app from dev dashboard e2eSection(wCtx, `Teardown: app ${ctx.appName}`) + if (!appResolved) { + log.error(wCtx, 'skipping app delete — app lookup failed, run `pnpm test:e2e-cleanup-apps` after') + return + } + if (!app) { + log.log(wCtx, 'app already deleted') + return + } + if (!installsCleared) { + log.log(wCtx, 'app delete skipped — still has installs, run `pnpm test:e2e-cleanup-apps` after') + return + } + + const appUrl = ctx.appUrl ?? `https://dev.shopify.com/dashboard/${ctx.env.orgId}/apps/${app.key}` log.log(wCtx, 'deleting app') let appDeleted = false - let stillHasInstalls = false for (let attempt = 1; attempt <= 3; attempt++) { try { - const appUrl = ctx.appUrl ?? (await findAppOnDevDashboard(page, ctx.appName, ctx.orgId)) - log.log(wCtx, ctx.appUrl ? 'using direct app URL for delete' : 'using dashboard search for delete') - if (!appUrl) { - // null could mean "app not in the list" OR "pagination ended on a stuck error page" - // — findAppOnDevDashboard's refresh-on-error doesn't cover every iteration. - // Detect and retry so we don't misclassify an error page as "already deleted". - if (await refreshIfPageError(page)) { - log.log(wCtx, `page error, refreshing...`) - continue - } - log.log(wCtx, 'app already deleted') - appDeleted = true - break - } - log.log(wCtx, 'app found, deleting') - const deleted = await deleteAppFromDevDashboard(page, appUrl) + const deleted = await deleteAppFromDevDashboard(ctx.browserPage, appUrl) if (deleted) { log.log(wCtx, 'app deleted') appDeleted = true @@ -166,17 +153,33 @@ export async function teardownAll(ctx: TeardownCtx): Promise { log.log(wCtx, `(${attempt}/3) app deletion failed`) // eslint-disable-next-line no-catch-all/no-catch-all } catch (err) { - // Fail fast: Delete button stays disabled while installs exist — retries won't help. - // cleanup-apps.ts reaps the orphan. + // Defense in depth: the API said zero installs, but the dashboard can + // still show the Delete button disabled if a record lags behind. if (err instanceof Error && err.message === 'STILL_HAS_INSTALLS') { log.log(wCtx, 'app delete skipped — still has installs, run `pnpm test:e2e-cleanup-apps` after') - stillHasInstalls = true - break + return } log.log(wCtx, `(${attempt}/3) app deletion failed: ${err instanceof Error ? err.message : err}`) } + await ctx.browserPage.waitForTimeout(BROWSER_TIMEOUT.medium) } - if (!appDeleted && !stillHasInstalls) { + if (!appDeleted) { log.error(wCtx, 'app deletion failed after 3 attempts') } } + +/** + * The app's client_id: read from the local TOML when the app dir is known, + * otherwise take the last segment of the Dev Dashboard app URL. + */ +function resolveClientId(ctx: TeardownCtx): string | undefined { + if (ctx.appDir) { + try { + return extractClientId(ctx.appDir) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + // TOML may be missing when the test failed before app creation. + } + } + return ctx.appUrl?.split('/').filter(Boolean).at(-1) +} diff --git a/packages/e2e/tests/app-deploy.spec.ts b/packages/e2e/tests/app-deploy.spec.ts index a8c1c0fac37..2174eedc7ed 100644 --- a/packages/e2e/tests/app-deploy.spec.ts +++ b/packages/e2e/tests/app-deploy.spec.ts @@ -190,15 +190,13 @@ test.describe('App deploy', () => { browserPage, appName, appUrl: primaryAppUrl, - orgId: env.orgId, - workerIndex: env.workerIndex, + env, }) await teardownAll({ browserPage, appName: secondaryAppName, appUrl: secondaryAppUrl, - orgId: env.orgId, - workerIndex: env.workerIndex, + env, }) } } diff --git a/packages/e2e/tests/app-dev-server.spec.ts b/packages/e2e/tests/app-dev-server.spec.ts index 19bbf43ec57..a286ff42cc0 100644 --- a/packages/e2e/tests/app-dev-server.spec.ts +++ b/packages/e2e/tests/app-dev-server.spec.ts @@ -60,9 +60,8 @@ test.describe('App dev server', () => { appName, appUrl, appDir, - orgId: env.orgId, + env, storeFqdn, - workerIndex: env.workerIndex, }) fs.rmSync(parentDir, {recursive: true, force: true}) } diff --git a/packages/e2e/tests/app-scaffold.spec.ts b/packages/e2e/tests/app-scaffold.spec.ts index 5a62c8659f2..072566f7092 100644 --- a/packages/e2e/tests/app-scaffold.spec.ts +++ b/packages/e2e/tests/app-scaffold.spec.ts @@ -54,8 +54,7 @@ test.describe('App scaffold', () => { browserPage, appName, appUrl, - orgId: env.orgId, - workerIndex: env.workerIndex, + env, }) } } @@ -92,8 +91,7 @@ test.describe('App scaffold', () => { browserPage, appName, appUrl, - orgId: env.orgId, - workerIndex: env.workerIndex, + env, }) } } @@ -150,8 +148,7 @@ test.describe('App scaffold', () => { browserPage, appName, appUrl, - orgId: env.orgId, - workerIndex: env.workerIndex, + env, }) } } diff --git a/packages/e2e/tests/dev-hot-reload.spec.ts b/packages/e2e/tests/dev-hot-reload.spec.ts index f976451aaf5..da08a6b195c 100644 --- a/packages/e2e/tests/dev-hot-reload.spec.ts +++ b/packages/e2e/tests/dev-hot-reload.spec.ts @@ -95,9 +95,8 @@ test.describe('Dev hot reload', () => { appName, appUrl, appDir, - orgId: env.orgId, + env, storeFqdn, - workerIndex: env.workerIndex, }) fs.rmSync(parentDir, {recursive: true, force: true}) } @@ -153,9 +152,8 @@ test.describe('Dev hot reload', () => { appName, appUrl, appDir, - orgId: env.orgId, + env, storeFqdn, - workerIndex: env.workerIndex, }) fs.rmSync(parentDir, {recursive: true, force: true}) } @@ -217,9 +215,8 @@ test.describe('Dev hot reload', () => { appName, appUrl, appDir, - orgId: env.orgId, + env, storeFqdn, - workerIndex: env.workerIndex, }) fs.rmSync(parentDir, {recursive: true, force: true}) } diff --git a/packages/e2e/tests/multi-config-dev.spec.ts b/packages/e2e/tests/multi-config-dev.spec.ts index d2559fc783e..2fbf9ae26ea 100644 --- a/packages/e2e/tests/multi-config-dev.spec.ts +++ b/packages/e2e/tests/multi-config-dev.spec.ts @@ -98,9 +98,8 @@ extensions_summary = "E2E staging app extensions" appName, appUrl, appDir, - orgId: env.orgId, + env, storeFqdn, - workerIndex: env.workerIndex, }) fs.rmSync(parentDir, {recursive: true, force: true}) } @@ -179,9 +178,8 @@ extensions_summary = "E2E staging app extensions" appName, appUrl, appDir, - orgId: env.orgId, + env, storeFqdn, - workerIndex: env.workerIndex, }) fs.rmSync(parentDir, {recursive: true, force: true}) } diff --git a/packages/e2e/tests/toml-config.spec.ts b/packages/e2e/tests/toml-config.spec.ts index 3f93816cb95..6b58ebae62b 100644 --- a/packages/e2e/tests/toml-config.spec.ts +++ b/packages/e2e/tests/toml-config.spec.ts @@ -44,8 +44,7 @@ test.describe('TOML config regression', () => { browserPage, appName, appUrl, - orgId: env.orgId, - workerIndex: env.workerIndex, + env, }) } } @@ -91,9 +90,8 @@ test.describe('TOML config regression', () => { appName, appUrl, appDir, - orgId: env.orgId, + env, storeFqdn, - workerIndex: env.workerIndex, }) fs.rmSync(parentDir, {recursive: true, force: true}) } From abf1cc38b52ae95a3a55027e4ac76d12f0ee47e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 12 Aug 2026 15:57:04 +0200 Subject: [PATCH 2/8] Fetch App Management token in a tsx subprocess MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Importing cli-kit's dist ESM inside Playwright's transpiled harness crashes Node's require(esm) path on CI's Node version ("Unexpected module status 3"). tsx's loader handles the interop — the same reason the cleanup scripts import cli-kit under tsx without issues. This also removes the process.env XDG mutation: the session dirs are passed to the subprocess environment directly. Co-Authored-By: Claude Fable 5 --- packages/e2e/setup/app-management-api.ts | 46 ++++++++----------- .../e2e/setup/get-app-management-token.ts | 20 ++++++++ 2 files changed, 40 insertions(+), 26 deletions(-) create mode 100644 packages/e2e/setup/get-app-management-token.ts diff --git a/packages/e2e/setup/app-management-api.ts b/packages/e2e/setup/app-management-api.ts index f2e3283492f..ccd67cca592 100644 --- a/packages/e2e/setup/app-management-api.ts +++ b/packages/e2e/setup/app-management-api.ts @@ -1,9 +1,12 @@ -/* eslint-disable no-restricted-globals, no-await-in-loop, @nx/enforce-module-boundaries -- the +/* eslint-disable no-restricted-globals, no-await-in-loop, no-restricted-imports -- the harness calls the App Management API directly with minimal queries, like admin-api.ts - does for the Admin API; cli-kit is only used to reuse the CLI session that - global setup created, following the cleanup-stores.ts pattern */ + does for the Admin API */ import {loadtestHeaderRecord} from '../helpers/loadtest-header.js' -import {ensureAuthenticatedAppManagementAndBusinessPlatform} from '../../cli-kit/dist/public/node/session.js' +import {execa} from 'execa' +import * as path from 'path' +import {fileURLToPath} from 'url' + +const __dirname = path.dirname(fileURLToPath(import.meta.url)) const APP_MANAGEMENT_URL = 'https://app.shopify.com/app_management/unstable/graphql.json' @@ -15,34 +18,25 @@ export interface AppManagementApp { key: string } -const XDG_KEYS = ['XDG_DATA_HOME', 'XDG_CONFIG_HOME', 'XDG_STATE_HOME', 'XDG_CACHE_HOME'] as const - -let sessionEnvReady = false +let cachedToken: string | undefined /** - * Point cli-kit's session storage at an authenticated CLI session before its - * first use in this process. cli-kit resolves its storage directory from - * process.env at first access and caches it, so this runs once and the dirs - * must not change afterwards — workers keep one XDG dir set for their lifetime. + * Get an App Management API token for the CLI session in `sessionEnv`'s XDG + * dirs. Delegates to get-app-management-token.ts in a tsx subprocess: cli-kit + * reuses the session cleanly there, while importing its dist into Playwright's + * transpiled harness crashes Node's require(esm) path on CI's Node version. */ -function ensureSessionEnv(sessionEnv: NodeJS.ProcessEnv): void { - if (sessionEnvReady) return - for (const key of XDG_KEYS) { - const value = sessionEnv[key] - if (value) process.env[key] = value - } - sessionEnvReady = true -} - -let cachedToken: string | undefined - async function appManagementToken(sessionEnv: NodeJS.ProcessEnv, forceRefresh = false): Promise { - ensureSessionEnv(sessionEnv) if (cachedToken && !forceRefresh) return cachedToken - const {appManagementToken: token} = await ensureAuthenticatedAppManagementAndBusinessPlatform({ - noPrompt: true, - forceRefresh, + const script = path.join(__dirname, 'get-app-management-token.ts') + const result = await execa('tsx', [script, ...(forceRefresh ? ['--force-refresh'] : [])], { + env: sessionEnv, + extendEnv: false, + preferLocal: true, + localDir: path.resolve(__dirname, '..'), + timeout: 60_000, }) + const {token} = JSON.parse(result.stdout) as {token: string} // eslint-disable-next-line require-atomic-updates cachedToken = token return token diff --git a/packages/e2e/setup/get-app-management-token.ts b/packages/e2e/setup/get-app-management-token.ts new file mode 100644 index 00000000000..84e7f32bb65 --- /dev/null +++ b/packages/e2e/setup/get-app-management-token.ts @@ -0,0 +1,20 @@ +/* eslint-disable @nx/enforce-module-boundaries -- see app-management-api.ts */ +/** + * Subprocess entrypoint: print an App Management API token for the CLI + * session in this process's XDG dirs, as JSON on stdout. + * + * Runs under tsx (see app-management-api.ts) because importing cli-kit's dist + * inside Playwright's transpiled harness crashes Node's require(esm) path on + * the Node versions CI uses ("Unexpected module status 3"). tsx's loader + * handles the ESM/CJS interop, which is also why the cleanup scripts import + * cli-kit this way without issues. + */ +import {ensureAuthenticatedAppManagementAndBusinessPlatform} from '../../cli-kit/dist/public/node/session.js' + +const forceRefresh = process.argv.includes('--force-refresh') + +const {appManagementToken} = await ensureAuthenticatedAppManagementAndBusinessPlatform({ + noPrompt: true, + forceRefresh, +}) +process.stdout.write(JSON.stringify({token: appManagementToken})) From b8e696b2f484b9be320299593d2eb239b6c3886b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 12 Aug 2026 16:05:07 +0200 Subject: [PATCH 3/8] Name the appByKey variable apiKey so the API can resolve the org The App Management API resolves the request's organization from specifically-named GraphQL variables (organizationId, apiKey, appId). The lookup variable was named "key", so every teardown lookup failed with 404 "Cannot find a valid organization" and app/store deletion was skipped. Co-Authored-By: Claude Fable 5 --- packages/e2e/setup/app-management-api.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/packages/e2e/setup/app-management-api.ts b/packages/e2e/setup/app-management-api.ts index ccd67cca592..672da75c74a 100644 --- a/packages/e2e/setup/app-management-api.ts +++ b/packages/e2e/setup/app-management-api.ts @@ -88,6 +88,11 @@ async function appManagementQuery( /** * Look up an app by its API key (client_id). Returns undefined when the app * does not exist — for teardown that means it was already deleted. + * + * The variable must be named `apiKey`: the API resolves the request's + * organization from specifically-named variables (`organizationId`, `apiKey`, + * `appId`) and rejects the request with "Cannot find a valid organization" + * otherwise. */ export async function findAppByClientId( sessionEnv: NodeJS.ProcessEnv, @@ -95,9 +100,9 @@ export async function findAppByClientId( ): Promise { const data = (await appManagementQuery( sessionEnv, - 'query appByKey($key: String!) { appByKey(key: $key) { id key } }', + 'query appByKey($apiKey: String!) { appByKey(key: $apiKey) { id key } }', { - key: clientId, + apiKey: clientId, }, )) as {appByKey?: AppManagementApp | null} return data.appByKey ?? undefined From 69bb1f69c05b41fdd72d9e485c22979117fb0eff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 12 Aug 2026 16:21:03 +0200 Subject: [PATCH 4/8] Make teardown app resolution and browser delete resilient MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - appByKey now sends organizationId: without it a deleted app cannot resolve an organization and 404s instead of returning null. - Client IDs are only taken from /apps/{segment} URL parts that are non-numeric; deploy output yields /apps/{numericAppId} URLs, which now fall back to a name search. - The settings-page navigation clicks through the accounts.shopify.com account picker, which cold browser contexts bounce to — the main reason direct-URL app deletion has been failing. Co-Authored-By: Claude Fable 5 --- packages/e2e/setup/app-management-api.ts | 12 ++++++--- packages/e2e/setup/app.ts | 31 ++++++++++++++++++++++-- packages/e2e/setup/teardown.ts | 15 ++++++++---- 3 files changed, 47 insertions(+), 11 deletions(-) diff --git a/packages/e2e/setup/app-management-api.ts b/packages/e2e/setup/app-management-api.ts index 672da75c74a..70edb29e20a 100644 --- a/packages/e2e/setup/app-management-api.ts +++ b/packages/e2e/setup/app-management-api.ts @@ -89,20 +89,24 @@ async function appManagementQuery( * Look up an app by its API key (client_id). Returns undefined when the app * does not exist — for teardown that means it was already deleted. * - * The variable must be named `apiKey`: the API resolves the request's - * organization from specifically-named variables (`organizationId`, `apiKey`, - * `appId`) and rejects the request with "Cannot find a valid organization" - * otherwise. + * Variable names matter: the API resolves the request's organization from + * specifically-named variables (`organizationId`, `apiKey`, `appId`) and + * rejects the request with "Cannot find a valid organization" otherwise. + * `organizationId` must be sent even though `apiKey` alone can resolve it: + * a deleted app resolves no organization, which would surface as that same + * rejection instead of a null `appByKey`. */ export async function findAppByClientId( sessionEnv: NodeJS.ProcessEnv, clientId: string, + orgId: string, ): Promise { const data = (await appManagementQuery( sessionEnv, 'query appByKey($apiKey: String!) { appByKey(key: $apiKey) { id key } }', { apiKey: clientId, + organizationId: orgId, }, )) as {appByKey?: AppManagementApp | null} return data.appByKey ?? undefined diff --git a/packages/e2e/setup/app.ts b/packages/e2e/setup/app.ts index 7df1077a2e5..e4f6ef2d496 100644 --- a/packages/e2e/setup/app.ts +++ b/packages/e2e/setup/app.ts @@ -323,6 +323,29 @@ export async function configLink( // Dev dashboard browser actions — delete apps // --------------------------------------------------------------------------- +/** + * Load the app's settings page, clicking through the accounts.shopify.com + * account picker if the session bounces there — which is common when the + * browser context has not visited the Dev Dashboard yet. + */ +async function gotoAppSettings(page: Page, appUrl: string): Promise { + await page.goto(`${appUrl}/settings`, {waitUntil: 'domcontentloaded'}) + await page.waitForTimeout(BROWSER_TIMEOUT.medium) + + if (!page.url().startsWith('https://accounts.shopify.com')) return + + const email = process.env.E2E_ACCOUNT_EMAIL + if (email) { + const accountButton = page.locator(`text=${email}`).first() + if (await isVisibleWithin(accountButton, BROWSER_TIMEOUT.long)) { + await accountButton.click() + await page.waitForTimeout(BROWSER_TIMEOUT.medium) + } + } + await page.goto(`${appUrl}/settings`, {waitUntil: 'domcontentloaded'}) + await page.waitForTimeout(BROWSER_TIMEOUT.medium) +} + /** * Delete an app from its dev dashboard settings page. Returns true if deleted. * @@ -333,8 +356,7 @@ export async function configLink( */ export async function deleteAppFromDevDashboard(page: Page, appUrl: string): Promise { // Step 1: Navigate to the app's settings page. 404 → already deleted. 5xx → throw for retry. - await page.goto(`${appUrl}/settings`, {waitUntil: 'domcontentloaded'}) - await page.waitForTimeout(BROWSER_TIMEOUT.medium) + await gotoAppSettings(page, appUrl) const gotoStatus = getLastPageStatus(page) if (gotoStatus === 404) return true if (gotoStatus !== undefined && gotoStatus >= 500) { @@ -345,6 +367,11 @@ export async function deleteAppFromDevDashboard(page: Page, appUrl: string): Pro // Button can be below the fold, and takes ~1-2s to enable after uninstall (one reload covers propagation lag). // If it stays disabled after reload, installs remain — fail fast for caller. const deleteBtn = page.locator('button:has-text("Delete app")').first() + if (!(await isVisibleWithin(deleteBtn, BROWSER_TIMEOUT.long))) { + // Include the landed URL: the usual cause is a session bounce that the + // account-picker handling above did not cover. + throw new Error(`Delete app button not found (page: ${page.url()})`) + } await deleteBtn.scrollIntoViewIfNeeded({timeout: BROWSER_TIMEOUT.long}) if (!(await deleteBtn.isEnabled())) { await page.reload({waitUntil: 'domcontentloaded'}) diff --git a/packages/e2e/setup/teardown.ts b/packages/e2e/setup/teardown.ts index 08806952080..35d7b588afc 100644 --- a/packages/e2e/setup/teardown.ts +++ b/packages/e2e/setup/teardown.ts @@ -52,9 +52,10 @@ export async function teardownAll(ctx: TeardownCtx): Promise { const clientId = resolveClientId(ctx) for (let attempt = 1; attempt <= 3; attempt++) { try { - if (clientId) { - app = await findAppByClientId(sessionEnv, clientId) - } else if (ctx.env.orgId) { + if (clientId && ctx.env.orgId) { + app = await findAppByClientId(sessionEnv, clientId, ctx.env.orgId) + } + if (!app && ctx.env.orgId) { app = await findAppByName(sessionEnv, ctx.appName, ctx.env.orgId) } appResolved = true @@ -170,7 +171,10 @@ export async function teardownAll(ctx: TeardownCtx): Promise { /** * The app's client_id: read from the local TOML when the app dir is known, - * otherwise take the last segment of the Dev Dashboard app URL. + * otherwise from the Dev Dashboard app URL. Dashboard URLs come in two + * shapes — apps/[clientId] (built by devDashboardAppUrl) and + * apps/[numericAppId] (parsed from deploy output) — and only the former is + * usable as an API key, so numeric segments resolve via name search instead. */ function resolveClientId(ctx: TeardownCtx): string | undefined { if (ctx.appDir) { @@ -181,5 +185,6 @@ function resolveClientId(ctx: TeardownCtx): string | undefined { // TOML may be missing when the test failed before app creation. } } - return ctx.appUrl?.split('/').filter(Boolean).at(-1) + const urlSegment = ctx.appUrl?.match(/\/apps\/([^/?#]+)/)?.[1] + return urlSegment && !/^\d+$/.test(urlSegment) ? urlSegment : undefined } From 3d750aabcb91db97fe957f3ee7d86aa66f23c218 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Wed, 12 Aug 2026 16:35:37 +0200 Subject: [PATCH 5/8] Delete apps via their numeric dashboard URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Settings pages under client-key URLs usually render without the Delete button; the numeric-id form (what the dashboard links to and the CLI's appDeepLink builds) works reliably — the run-level cleanup deletes 11/11 apps with it while key-form teardown deletes went 2/13. The app GID from the API lookup provides the numeric id. Co-Authored-By: Claude Fable 5 --- packages/e2e/setup/teardown.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/e2e/setup/teardown.ts b/packages/e2e/setup/teardown.ts index 35d7b588afc..b6f45e0a12a 100644 --- a/packages/e2e/setup/teardown.ts +++ b/packages/e2e/setup/teardown.ts @@ -140,7 +140,16 @@ export async function teardownAll(ctx: TeardownCtx): Promise { return } - const appUrl = ctx.appUrl ?? `https://dev.shopify.com/dashboard/${ctx.env.orgId}/apps/${app.key}` + // Prefer the numeric-id URL derived from the app's GID — the same form the + // dashboard links to and the CLI's appDeepLink builds. Settings pages under + // client-key URLs (devDashboardAppUrl) usually render without the Delete + // button: the run-level cleanup deletes 11/11 apps with numeric URLs while + // key-form teardown deletes went 2/13. + const numericAppId = app.id.match(/(\d+)$/)?.[1] + const appUrl = + numericAppId && ctx.env.orgId + ? `https://dev.shopify.com/dashboard/${ctx.env.orgId}/apps/${numericAppId}` + : (ctx.appUrl ?? `https://dev.shopify.com/dashboard/${ctx.env.orgId}/apps/${app.key}`) log.log(wCtx, 'deleting app') let appDeleted = false for (let attempt = 1; attempt <= 3; attempt++) { From c52ba456e954b452441406abad70e968baadaecb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 14 Aug 2026 11:56:17 +0200 Subject: [PATCH 6/8] Correct E2E teardown ordering Assisted-By: devx/215513a9-13fa-4e74-bb8f-79faee6e4f09 --- packages/e2e/playwright.config.ts | 4 + packages/e2e/setup/app-management-api.ts | 177 ++---------- packages/e2e/setup/app-management-state.ts | 32 +++ packages/e2e/setup/app.ts | 15 +- .../e2e/setup/get-app-management-token.ts | 20 -- .../e2e/setup/inspect-app-management-state.ts | 111 ++++++++ packages/e2e/setup/teardown-orchestrator.ts | 124 +++++++++ packages/e2e/setup/teardown.ts | 251 +++++++----------- .../e2e/tests/app-management-state.spec.ts | 50 ++++ packages/e2e/tests/teardown.spec.ts | 186 +++++++++++++ 10 files changed, 641 insertions(+), 329 deletions(-) create mode 100644 packages/e2e/setup/app-management-state.ts delete mode 100644 packages/e2e/setup/get-app-management-token.ts create mode 100644 packages/e2e/setup/inspect-app-management-state.ts create mode 100644 packages/e2e/setup/teardown-orchestrator.ts create mode 100644 packages/e2e/tests/app-management-state.spec.ts create mode 100644 packages/e2e/tests/teardown.spec.ts diff --git a/packages/e2e/playwright.config.ts b/packages/e2e/playwright.config.ts index c4e6e148c8f..3ebff191d00 100644 --- a/packages/e2e/playwright.config.ts +++ b/packages/e2e/playwright.config.ts @@ -31,6 +31,8 @@ export default defineConfig({ 'tests/smoke-pty.spec.ts', 'tests/fixture-toml.spec.ts', 'tests/auth-diagnostics.spec.ts', + 'tests/app-management-state.spec.ts', + 'tests/teardown.spec.ts', ], }, { @@ -45,6 +47,8 @@ export default defineConfig({ 'tests/smoke-pty.spec.ts', 'tests/fixture-toml.spec.ts', 'tests/auth-diagnostics.spec.ts', + 'tests/app-management-state.spec.ts', + 'tests/teardown.spec.ts', ], dependencies: ['remote-auth'], }, diff --git a/packages/e2e/setup/app-management-api.ts b/packages/e2e/setup/app-management-api.ts index 70edb29e20a..417176d8d99 100644 --- a/packages/e2e/setup/app-management-api.ts +++ b/packages/e2e/setup/app-management-api.ts @@ -1,167 +1,46 @@ -/* eslint-disable no-restricted-globals, no-await-in-loop, no-restricted-imports -- the - harness calls the App Management API directly with minimal queries, like admin-api.ts - does for the Admin API */ -import {loadtestHeaderRecord} from '../helpers/loadtest-header.js' +/* eslint-disable no-restricted-imports -- this wrapper runs the cli-kit API client in a tsx subprocess */ import {execa} from 'execa' import * as path from 'path' import {fileURLToPath} from 'url' +import type {AppDeletionReadiness} from './teardown-orchestrator.js' const __dirname = path.dirname(fileURLToPath(import.meta.url)) +const RESULT_PREFIX = 'E2E_APP_MANAGEMENT_RESULT=' -const APP_MANAGEMENT_URL = 'https://app.shopify.com/app_management/unstable/graphql.json' - -/** Minimal app identity as returned by the App Management API. */ -export interface AppManagementApp { - /** GID, e.g. gid://organization/App/123 — input for install-count lookups. */ - id: string - /** API key (the app's client_id) — last segment of the Dev Dashboard app URL. */ - key: string +interface AppDeletionReadinessOptions { + appName: string + clientId?: string + orgId: string + timeoutMs?: number + pollIntervalMs?: number } -let cachedToken: string | undefined - /** - * Get an App Management API token for the CLI session in `sessionEnv`'s XDG - * dirs. Delegates to get-app-management-token.ts in a tsx subprocess: cli-kit - * reuses the session cleanly there, while importing its dist into Playwright's - * transpiled harness crashes Node's require(esm) path on CI's Node version. + * Inspect the app and wait for its install count to reach zero. + * + * The subprocess is required because Playwright's transformed module graph + * cannot load cli-kit's dist ESM on every supported CI Node version. Running + * the API work under tsx also lets teardown use cli-kit's normal GraphQL + * throttling, network retry, and token-refresh behavior. */ -async function appManagementToken(sessionEnv: NodeJS.ProcessEnv, forceRefresh = false): Promise { - if (cachedToken && !forceRefresh) return cachedToken - const script = path.join(__dirname, 'get-app-management-token.ts') - const result = await execa('tsx', [script, ...(forceRefresh ? ['--force-refresh'] : [])], { - env: sessionEnv, +export async function waitForAppDeletionReadiness( + sessionEnv: NodeJS.ProcessEnv, + options: AppDeletionReadinessOptions, +): Promise { + const script = path.join(__dirname, 'inspect-app-management-state.ts') + const result = await execa('tsx', [script], { + env: {...sessionEnv, SHOPIFY_FLAG_VERBOSE: undefined}, extendEnv: false, preferLocal: true, localDir: path.resolve(__dirname, '..'), - timeout: 60_000, + input: JSON.stringify(options), + timeout: (options.timeoutMs ?? 30_000) + 90_000, }) - const {token} = JSON.parse(result.stdout) as {token: string} - // eslint-disable-next-line require-atomic-updates - cachedToken = token - return token -} - -interface GraphQLPayload { - errors?: {message?: string}[] - data?: unknown -} - -/** - * Run a GraphQL query against the App Management API using the CLI session - * from `sessionEnv`. Retries once with a refreshed token on 401. - */ -async function appManagementQuery( - sessionEnv: NodeJS.ProcessEnv, - query: string, - variables: {[key: string]: string}, -): Promise { - let token = await appManagementToken(sessionEnv) - - for (let attempt = 1; ; attempt++) { - const response = await fetch(APP_MANAGEMENT_URL, { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - Authorization: `Bearer ${token}`, - ...loadtestHeaderRecord(), - }, - body: JSON.stringify({query, variables}), - }) - if (response.status === 401 && attempt === 1) { - token = await appManagementToken(sessionEnv, true) - continue - } - if (!response.ok) { - throw new Error(`App Management API request failed (status ${response.status}): ${await response.text()}`) - } - - const payload = (await response.json()) as GraphQLPayload - if (payload.errors?.length) { - throw new Error(`App Management API returned errors: ${payload.errors.map((error) => error.message).join(', ')}`) - } - return payload.data + const resultLine = result.stdout.split('\n').findLast((line) => line.startsWith(RESULT_PREFIX)) + if (!resultLine) { + throw new Error('App Management inspection did not return a result') } -} -/** - * Look up an app by its API key (client_id). Returns undefined when the app - * does not exist — for teardown that means it was already deleted. - * - * Variable names matter: the API resolves the request's organization from - * specifically-named variables (`organizationId`, `apiKey`, `appId`) and - * rejects the request with "Cannot find a valid organization" otherwise. - * `organizationId` must be sent even though `apiKey` alone can resolve it: - * a deleted app resolves no organization, which would surface as that same - * rejection instead of a null `appByKey`. - */ -export async function findAppByClientId( - sessionEnv: NodeJS.ProcessEnv, - clientId: string, - orgId: string, -): Promise { - const data = (await appManagementQuery( - sessionEnv, - 'query appByKey($apiKey: String!) { appByKey(key: $apiKey) { id key } }', - { - apiKey: clientId, - organizationId: orgId, - }, - )) as {appByKey?: AppManagementApp | null} - return data.appByKey ?? undefined -} - -/** - * Search the org's apps by exact name. Returns undefined when no app matches. - * - * `organizationId` is not declared in the operation on purpose: the App - * Management API reads it from the variables map for request routing, exactly - * as the CLI's own `appsForOrg` does. - */ -export async function findAppByName( - sessionEnv: NodeJS.ProcessEnv, - appName: string, - orgId: string, -): Promise { - const data = (await appManagementQuery( - sessionEnv, - 'query listApps($query: String) { appsConnection(query: $query, first: 50) { edges { node { id key activeRelease { version { name } } } } } }', - {query: `title:${appName}`, organizationId: orgId}, - )) as { - appsConnection?: {edges: {node: AppManagementApp & {activeRelease: {version: {name: string}}}}[]} | null - } - return data.appsConnection?.edges.map((edge) => edge.node).find((node) => node.activeRelease.version.name === appName) -} - -/** Total install count for an app across all stores. */ -export async function appInstallCount(sessionEnv: NodeJS.ProcessEnv, appId: string): Promise { - const data = (await appManagementQuery( - sessionEnv, - 'query AppInstallCount($appId: ID!) { app(id: $appId) { installCount } }', - {appId}, - )) as {app?: {installCount?: number | null} | null} - return data.app?.installCount ?? 0 -} - -/** - * Poll until the app reports zero installs. Uninstall records clear - * asynchronously after an Admin API uninstall, usually within seconds. - * Returns false when installs remain after the timeout. - */ -export async function waitForZeroInstalls( - sessionEnv: NodeJS.ProcessEnv, - appId: string, - options: {timeoutMs?: number; pollIntervalMs?: number} = {}, -): Promise { - const timeoutMs = options.timeoutMs ?? 30_000 - const pollIntervalMs = options.pollIntervalMs ?? 2_000 - const deadline = Date.now() + timeoutMs - - while ((await appInstallCount(sessionEnv, appId)) > 0) { - if (Date.now() >= deadline) return false - - await new Promise((resolve) => setTimeout(resolve, pollIntervalMs)) - } - return true + return JSON.parse(resultLine.slice(RESULT_PREFIX.length)) as AppDeletionReadiness } diff --git a/packages/e2e/setup/app-management-state.ts b/packages/e2e/setup/app-management-state.ts new file mode 100644 index 00000000000..24b2d4f028e --- /dev/null +++ b/packages/e2e/setup/app-management-state.ts @@ -0,0 +1,32 @@ +import type {AppDeletionReadiness} from './teardown-orchestrator.js' + +export interface AppManagementAppState { + id: string + key: string + installCount?: number | null + activeRelease: {version: {name: string}} +} + +export function appDeletionReadinessFromApps( + apps: AppManagementAppState[], + appName: string, + clientId?: string, +): AppDeletionReadiness { + const exactNameMatches = apps.filter((app) => app.activeRelease.version.name === appName) + const clientIdMatches = clientId ? exactNameMatches.filter((app) => app.key === clientId) : [] + const matchingApps = clientIdMatches.length > 0 ? clientIdMatches : exactNameMatches + + if (matchingApps.length === 0) return {status: 'already-deleted'} + if (matchingApps.length > 1) { + throw new Error(`App Management API returned multiple apps named ${appName}`) + } + + const app = matchingApps[0]! + if (typeof app.installCount !== 'number') { + throw new Error(`App Management API did not return installCount for ${appName}`) + } + if (app.installCount === 0) { + return {status: 'ready', app: {id: app.id, key: app.key}} + } + return {status: 'still-installed', installCount: app.installCount} +} diff --git a/packages/e2e/setup/app.ts b/packages/e2e/setup/app.ts index e4f6ef2d496..92cb3a90d9f 100644 --- a/packages/e2e/setup/app.ts +++ b/packages/e2e/setup/app.ts @@ -332,7 +332,7 @@ async function gotoAppSettings(page: Page, appUrl: string): Promise { await page.goto(`${appUrl}/settings`, {waitUntil: 'domcontentloaded'}) await page.waitForTimeout(BROWSER_TIMEOUT.medium) - if (!page.url().startsWith('https://accounts.shopify.com')) return + if (!isAccountsShopifyUrl(page.url())) return const email = process.env.E2E_ACCOUNT_EMAIL if (email) { @@ -346,13 +346,22 @@ async function gotoAppSettings(page: Page, appUrl: string): Promise { await page.waitForTimeout(BROWSER_TIMEOUT.medium) } +function isAccountsShopifyUrl(rawUrl: string): boolean { + try { + return new URL(rawUrl).hostname === 'accounts.shopify.com' + // eslint-disable-next-line no-catch-all/no-catch-all + } catch { + return false + } +} + /** * Delete an app from its dev dashboard settings page. Returns true if deleted. * * Single attempt — caller owns the retry loop. * - * Fail-fast on STILL_HAS_INSTALLS: the Delete button stays disabled while - * installs exist, so we throw to let the caller skip instead of spinning. + * Throws STILL_HAS_INSTALLS when the Delete button remains disabled after a + * reload so the caller can apply its bounded retry policy. */ export async function deleteAppFromDevDashboard(page: Page, appUrl: string): Promise { // Step 1: Navigate to the app's settings page. 404 → already deleted. 5xx → throw for retry. diff --git a/packages/e2e/setup/get-app-management-token.ts b/packages/e2e/setup/get-app-management-token.ts deleted file mode 100644 index 84e7f32bb65..00000000000 --- a/packages/e2e/setup/get-app-management-token.ts +++ /dev/null @@ -1,20 +0,0 @@ -/* eslint-disable @nx/enforce-module-boundaries -- see app-management-api.ts */ -/** - * Subprocess entrypoint: print an App Management API token for the CLI - * session in this process's XDG dirs, as JSON on stdout. - * - * Runs under tsx (see app-management-api.ts) because importing cli-kit's dist - * inside Playwright's transpiled harness crashes Node's require(esm) path on - * the Node versions CI uses ("Unexpected module status 3"). tsx's loader - * handles the ESM/CJS interop, which is also why the cleanup scripts import - * cli-kit this way without issues. - */ -import {ensureAuthenticatedAppManagementAndBusinessPlatform} from '../../cli-kit/dist/public/node/session.js' - -const forceRefresh = process.argv.includes('--force-refresh') - -const {appManagementToken} = await ensureAuthenticatedAppManagementAndBusinessPlatform({ - noPrompt: true, - forceRefresh, -}) -process.stdout.write(JSON.stringify({token: appManagementToken})) diff --git a/packages/e2e/setup/inspect-app-management-state.ts b/packages/e2e/setup/inspect-app-management-state.ts new file mode 100644 index 00000000000..e1008006f31 --- /dev/null +++ b/packages/e2e/setup/inspect-app-management-state.ts @@ -0,0 +1,111 @@ +/* eslint-disable @nx/enforce-module-boundaries, no-await-in-loop -- this + subprocess uses cli-kit's built API client with the isolated E2E session */ +import {appDeletionReadinessFromApps} from './app-management-state.js' +import {appManagementFqdn} from '../../cli-kit/dist/public/node/context/fqdn.js' +import {graphqlRequest} from '../../cli-kit/dist/public/node/api/graphql.js' +import {ensureAuthenticatedAppManagementAndBusinessPlatform} from '../../cli-kit/dist/public/node/session.js' +import {loadtestHeaderRecord} from '../helpers/loadtest-header.js' +import type {AppDeletionReadiness} from './teardown-orchestrator.js' +import type {AppManagementAppState} from './app-management-state.js' + +const RESULT_PREFIX = 'E2E_APP_MANAGEMENT_RESULT=' + +interface InspectionOptions { + appName: string + clientId?: string + orgId: string + timeoutMs?: number + pollIntervalMs?: number +} + +interface AppsQueryResult { + appsConnection?: {edges: {node: AppManagementAppState}[]} | null +} + +const query = ` + query E2ETeardownApps($query: String) { + appsConnection(query: $query, first: 50) { + edges { + node { + id + key + installCount + activeRelease { + version { + name + } + } + } + } + } + } +` + +const options = JSON.parse(await readStandardInput()) as InspectionOptions +const timeoutMs = options.timeoutMs ?? 30_000 +const pollIntervalMs = options.pollIntervalMs ?? 2_000 +const deadline = Date.now() + timeoutMs +const missingAppConfirmationsRequired = 2 +const apiUrl = `https://${await appManagementFqdn()}/app_management/unstable/graphql.json` + +let {appManagementToken} = await ensureAuthenticatedAppManagementAndBusinessPlatform({noPrompt: true}) + +async function inspectApp(): Promise { + const result = await graphqlRequest({ + api: 'App Management', + url: apiUrl, + token: appManagementToken, + addedHeaders: loadtestHeaderRecord(), + query, + variables: { + query: `title:${options.appName}`, + // App Management reads this undeclared variable to route the request to the organization. + organizationId: options.orgId, + }, + unauthorizedHandler: { + type: 'token_refresh', + handler: async () => { + const refreshed = await ensureAuthenticatedAppManagementAndBusinessPlatform({ + noPrompt: true, + forceRefresh: true, + }) + appManagementToken = refreshed.appManagementToken + return {token: appManagementToken} + }, + }, + }) + + if (!result.appsConnection) { + throw new Error('App Management API did not return appsConnection') + } + + return appDeletionReadinessFromApps( + result.appsConnection.edges.map((edge) => edge.node), + options.appName, + options.clientId, + ) +} + +let readiness: AppDeletionReadiness +let missingAppConfirmations = 0 + +while (true) { + readiness = await inspectApp() + missingAppConfirmations = readiness.status === 'already-deleted' ? missingAppConfirmations + 1 : 0 + + if (readiness.status === 'ready') break + if (readiness.status === 'already-deleted' && missingAppConfirmations >= missingAppConfirmationsRequired) break + if (Date.now() >= deadline) break + + await new Promise((resolve) => setTimeout(resolve, pollIntervalMs)) +} + +process.stdout.write(`\n${RESULT_PREFIX}${JSON.stringify(readiness)}\n`) + +async function readStandardInput(): Promise { + let input = '' + for await (const chunk of process.stdin) { + input += chunk.toString() + } + return input +} diff --git a/packages/e2e/setup/teardown-orchestrator.ts b/packages/e2e/setup/teardown-orchestrator.ts new file mode 100644 index 00000000000..5c531835103 --- /dev/null +++ b/packages/e2e/setup/teardown-orchestrator.ts @@ -0,0 +1,124 @@ +export type AppDeletionReadiness = + | {status: 'ready'; app: {id: string; key: string}} + | {status: 'already-deleted'} + | {status: 'still-installed'; installCount: number} + +export type CleanupPhase = 'uninstall-app' | 'wait-for-zero-installs' | 'delete-app' | 'delete-store' +export type CleanupPhaseStatus = 'completed' | 'failed' | 'skipped' + +export interface CleanupPhaseRecord { + phase: CleanupPhase + status: CleanupPhaseStatus + detail: string +} + +interface TeardownOperations { + hasStore: boolean + uninstallApp?: () => Promise + waitForAppDeletionReadiness: () => Promise + deleteApp: (app: {id: string; key: string}) => Promise + deleteStore?: () => Promise + record: (record: CleanupPhaseRecord) => void +} + +export async function runTeardown(operations: TeardownOperations): Promise { + if (operations.hasStore) { + if (operations.uninstallApp) { + try { + await operations.uninstallApp() + operations.record({phase: 'uninstall-app', status: 'completed', detail: 'app uninstalled'}) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + operations.record({phase: 'uninstall-app', status: 'failed', detail: errorMessage(error)}) + } + } else { + operations.record({ + phase: 'uninstall-app', + status: 'skipped', + detail: 'app directory unavailable', + }) + } + } + + let readiness: AppDeletionReadiness + try { + readiness = await operations.waitForAppDeletionReadiness() + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + operations.record({phase: 'wait-for-zero-installs', status: 'failed', detail: errorMessage(error)}) + skipRemainingPhases(operations, 'installation state is unknown') + return + } + + if (readiness.status === 'still-installed') { + operations.record({ + phase: 'wait-for-zero-installs', + status: 'failed', + detail: `app still has ${readiness.installCount} install(s)`, + }) + skipRemainingPhases(operations, 'app still has installs') + return + } + + operations.record({ + phase: 'wait-for-zero-installs', + status: 'completed', + detail: readiness.status === 'already-deleted' ? 'app already deleted' : 'app has zero installs', + }) + + if (readiness.status === 'already-deleted') { + operations.record({phase: 'delete-app', status: 'completed', detail: 'app already deleted'}) + } else { + try { + const deleted = await operations.deleteApp(readiness.app) + operations.record({ + phase: 'delete-app', + status: deleted ? 'completed' : 'failed', + detail: deleted ? 'app deleted' : 'app deletion was not confirmed', + }) + if (!deleted) { + skipStoreDeletion(operations, 'app deletion was not confirmed') + return + } + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + operations.record({phase: 'delete-app', status: 'failed', detail: errorMessage(error)}) + skipStoreDeletion(operations, 'app deletion failed') + return + } + } + + if (!operations.hasStore) return + + if (!operations.deleteStore) { + operations.record({phase: 'delete-store', status: 'failed', detail: 'store deletion operation unavailable'}) + return + } + + try { + const deleted = await operations.deleteStore() + operations.record({ + phase: 'delete-store', + status: deleted ? 'completed' : 'failed', + detail: deleted ? 'store deletion requested' : 'store deletion was not confirmed', + }) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + operations.record({phase: 'delete-store', status: 'failed', detail: errorMessage(error)}) + } +} + +function skipRemainingPhases(operations: TeardownOperations, detail: string): void { + operations.record({phase: 'delete-app', status: 'skipped', detail}) + skipStoreDeletion(operations, detail) +} + +function skipStoreDeletion(operations: TeardownOperations, detail: string): void { + if (operations.hasStore) { + operations.record({phase: 'delete-store', status: 'skipped', detail}) + } +} + +function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error) +} diff --git a/packages/e2e/setup/teardown.ts b/packages/e2e/setup/teardown.ts index b6f45e0a12a..6840ff92dd7 100644 --- a/packages/e2e/setup/teardown.ts +++ b/packages/e2e/setup/teardown.ts @@ -1,199 +1,136 @@ /* eslint-disable no-await-in-loop */ import {uninstallAppWithAdminApi} from './admin-api.js' -import {findAppByClientId, findAppByName, appInstallCount, waitForZeroInstalls} from './app-management-api.js' +import {waitForAppDeletionReadiness} from './app-management-api.js' import {deleteAppFromDevDashboard, extractClientId} from './app.js' -import {createLogger, e2eSection} from './env.js' +import {runTeardown} from './teardown-orchestrator.js' +import {e2eSection} from './env.js' import {BROWSER_TIMEOUT} from './constants.js' import {deleteDevStoreWithCli} from './store.js' -import type {AppManagementApp} from './app-management-api.js' +import type {CleanupPhaseRecord} from './teardown-orchestrator.js' import type {CLIProcess} from './cli.js' import type {E2EEnv} from './env.js' import type {Page} from '@playwright/test' -const log = createLogger('teardown') - -interface BaseTeardownCtx { +interface BaseTeardownContext { browserPage: Page appName: string env: E2EEnv - /** Direct Dev Dashboard app URL. Prefer this when available to avoid an app search by name. */ appUrl?: string } -type TeardownCtx = BaseTeardownCtx & - ( - | {storeFqdn: string; cli: CLIProcess; appDir: string | undefined} - | {storeFqdn?: undefined; cli?: CLIProcess; appDir?: string} - ) +type StoreTeardownContext = BaseTeardownContext & { + storeFqdn: string + cli: CLIProcess + appDir: string | undefined +} + +type TeardownContext = StoreTeardownContext | (BaseTeardownContext & {storeFqdn?: undefined; appDir?: string}) /** - * Best-effort per-test teardown. Each phase retries up to 3 times. - * - * App + store flow: - * Phase 1: uninstall app from store over the Admin API - * Phase 2: delete store (skipped until the app reports zero installs) - * Phase 3: delete app from dev dashboard (browser — the App Management API - * has no delete mutation) + * Best-effort per-test teardown. * - * App-only flow: - * Phase 3 only - * - * The app's identity and install state come from the App Management API; the - * browser is only used for the final delete click. + * Store-backed tests use this order: + * uninstall app, wait for zero installs, delete app, then delete store. + * Every phase records its own result and teardown never replaces the test result. */ -export async function teardownAll(ctx: TeardownCtx): Promise { - const wCtx = {workerIndex: ctx.env.workerIndex} - const sessionEnv = ctx.env.processEnv - - // Resolve the app via the App Management API. `undefined` app after a - // successful lookup means it does not exist (already deleted). - let app: AppManagementApp | undefined - let appResolved = false - const clientId = resolveClientId(ctx) +export async function teardownAll(context: TeardownContext): Promise { + const workerContext = {workerIndex: context.env.workerIndex} + const clientId = resolveClientId(context) + const storeContext = hasStore(context) ? context : undefined + const appDir = storeContext?.appDir + + e2eSection(workerContext, `Teardown: app ${context.appName}`) + + await runTeardown({ + hasStore: Boolean(storeContext), + uninstallApp: + storeContext && appDir + ? () => + uninstallAppWithAdminApi({ + cli: storeContext.cli, + appDir, + storeFqdn: storeContext.storeFqdn, + }) + : undefined, + waitForAppDeletionReadiness: () => + waitForAppDeletionReadiness(context.env.processEnv, { + appName: context.appName, + clientId, + orgId: context.env.orgId, + }), + deleteApp: (app) => deleteAppWithRetry(context, app), + deleteStore: storeContext ? () => deleteStoreWithRetry(storeContext) : undefined, + record: (record) => recordCleanupPhase(context.env.workerIndex, record), + }) +} + +function hasStore(context: TeardownContext): context is StoreTeardownContext { + return context.storeFqdn !== undefined +} + +async function deleteAppWithRetry(context: TeardownContext, app: {id: string; key: string}): Promise { + const numericAppId = app.id.match(/(\d+)$/)?.[1] + // The dashboard exposes the Delete button on its numeric app route. Keep the + // client-key URL only as a fallback when the API does not return a numeric GID. + const appUrl = + numericAppId && context.env.orgId + ? `https://dev.shopify.com/dashboard/${context.env.orgId}/apps/${numericAppId}` + : (context.appUrl ?? `https://dev.shopify.com/dashboard/${context.env.orgId}/apps/${app.key}`) + + let lastError: unknown for (let attempt = 1; attempt <= 3; attempt++) { try { - if (clientId && ctx.env.orgId) { - app = await findAppByClientId(sessionEnv, clientId, ctx.env.orgId) - } - if (!app && ctx.env.orgId) { - app = await findAppByName(sessionEnv, ctx.appName, ctx.env.orgId) - } - appResolved = true - break + if (await deleteAppFromDevDashboard(context.browserPage, appUrl)) return true + lastError = new Error('app deletion was not confirmed') // eslint-disable-next-line no-catch-all/no-catch-all - } catch (err) { - log.log(wCtx, `(${attempt}/3) app lookup failed: ${err instanceof Error ? err.message : err}`) - } - } - - // Phase 1: Uninstall app from store over the Admin API (app+store tests - // only). No browser fallback: an API failure must surface loudly so it gets - // fixed instead of hiding behind a flaky store-admin click-through. - if (ctx.storeFqdn) { - e2eSection(wCtx, `Teardown: store ${ctx.storeFqdn}`) - if (ctx.appDir) { - log.log(wCtx, 'uninstalling app via admin API') - await uninstallAppWithAdminApi({cli: ctx.cli, appDir: ctx.appDir, storeFqdn: ctx.storeFqdn}) - log.log(wCtx, 'app uninstalled via admin API') - } else { - // The test failed before createApp finished — nothing was installed. - log.log(wCtx, 'no app dir, skipping uninstall') + } catch (error) { + lastError = error } - } - // Install state gates both store deletion (an install record would strand - // the app in the Dev Dashboard) and app deletion (the Delete button stays - // disabled while installs exist, so the browser flow would just spin). - // Uninstall records clear asynchronously, hence the poll after uninstall. - let installsCleared = false - if (appResolved) { - if (!app) { - installsCleared = true - } else if (ctx.storeFqdn) { - installsCleared = await waitForZeroInstalls(sessionEnv, app.id) - } else { - installsCleared = (await appInstallCount(sessionEnv, app.id)) === 0 + if (attempt < 3) { + await context.browserPage.waitForTimeout(BROWSER_TIMEOUT.medium) } } - // Phase 2: Delete store - if (ctx.storeFqdn) { - if (appResolved && installsCleared) { - log.log(wCtx, 'deleting store') - let storeDeletionRequested = false - for (let attempt = 1; attempt <= 3; attempt++) { - try { - const deletionConfirmed = await deleteDevStoreWithCli({ - cli: ctx.cli, - storeFqdn: ctx.storeFqdn, - orgId: ctx.env.orgId, - }) - log.log(wCtx, deletionConfirmed ? 'store deletion confirmed by CLI' : 'store deletion requested with CLI') - storeDeletionRequested = true - break - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (err) { - log.log(wCtx, `(${attempt}/3) store deletion failed: ${err instanceof Error ? err.message : err}`) - } - } - if (!storeDeletionRequested) { - log.error(wCtx, 'store deletion request failed after 3 attempts') - } - } else { - const reason = appResolved ? 'app still reports installs' : 'install state unknown (app lookup failed)' - log.error(wCtx, `${reason}, skipping store delete`) - } - } - - // Phase 3: Delete app from dev dashboard - e2eSection(wCtx, `Teardown: app ${ctx.appName}`) - if (!appResolved) { - log.error(wCtx, 'skipping app delete — app lookup failed, run `pnpm test:e2e-cleanup-apps` after') - return - } - if (!app) { - log.log(wCtx, 'app already deleted') - return - } - if (!installsCleared) { - log.log(wCtx, 'app delete skipped — still has installs, run `pnpm test:e2e-cleanup-apps` after') - return - } + throw lastError instanceof Error ? lastError : new Error(String(lastError)) +} - // Prefer the numeric-id URL derived from the app's GID — the same form the - // dashboard links to and the CLI's appDeepLink builds. Settings pages under - // client-key URLs (devDashboardAppUrl) usually render without the Delete - // button: the run-level cleanup deletes 11/11 apps with numeric URLs while - // key-form teardown deletes went 2/13. - const numericAppId = app.id.match(/(\d+)$/)?.[1] - const appUrl = - numericAppId && ctx.env.orgId - ? `https://dev.shopify.com/dashboard/${ctx.env.orgId}/apps/${numericAppId}` - : (ctx.appUrl ?? `https://dev.shopify.com/dashboard/${ctx.env.orgId}/apps/${app.key}`) - log.log(wCtx, 'deleting app') - let appDeleted = false +async function deleteStoreWithRetry(context: StoreTeardownContext): Promise { + let lastError: unknown for (let attempt = 1; attempt <= 3; attempt++) { try { - const deleted = await deleteAppFromDevDashboard(ctx.browserPage, appUrl) - if (deleted) { - log.log(wCtx, 'app deleted') - appDeleted = true - break - } - log.log(wCtx, `(${attempt}/3) app deletion failed`) + await deleteDevStoreWithCli({ + cli: context.cli, + storeFqdn: context.storeFqdn, + orgId: context.env.orgId, + }) + return true // eslint-disable-next-line no-catch-all/no-catch-all - } catch (err) { - // Defense in depth: the API said zero installs, but the dashboard can - // still show the Delete button disabled if a record lags behind. - if (err instanceof Error && err.message === 'STILL_HAS_INSTALLS') { - log.log(wCtx, 'app delete skipped — still has installs, run `pnpm test:e2e-cleanup-apps` after') - return - } - log.log(wCtx, `(${attempt}/3) app deletion failed: ${err instanceof Error ? err.message : err}`) + } catch (error) { + lastError = error } - await ctx.browserPage.waitForTimeout(BROWSER_TIMEOUT.medium) - } - if (!appDeleted) { - log.error(wCtx, 'app deletion failed after 3 attempts') } + + throw lastError instanceof Error ? lastError : new Error(String(lastError)) } -/** - * The app's client_id: read from the local TOML when the app dir is known, - * otherwise from the Dev Dashboard app URL. Dashboard URLs come in two - * shapes — apps/[clientId] (built by devDashboardAppUrl) and - * apps/[numericAppId] (parsed from deploy output) — and only the former is - * usable as an API key, so numeric segments resolve via name search instead. - */ -function resolveClientId(ctx: TeardownCtx): string | undefined { - if (ctx.appDir) { +function recordCleanupPhase(workerIndex: number, record: CleanupPhaseRecord): void { + const output = record.status === 'failed' ? process.stderr : process.stdout + output.write( + `[e2e][w${workerIndex}][teardown] phase=${record.phase} status=${record.status} detail=${record.detail}\n`, + ) +} + +function resolveClientId(context: TeardownContext): string | undefined { + if (context.appDir) { try { - return extractClientId(ctx.appDir) + return extractClientId(context.appDir) // eslint-disable-next-line no-catch-all/no-catch-all } catch { - // TOML may be missing when the test failed before app creation. + // The app TOML can be unavailable when setup failed partway through. } } - const urlSegment = ctx.appUrl?.match(/\/apps\/([^/?#]+)/)?.[1] + + const urlSegment = context.appUrl?.match(/\/apps\/([^/?#]+)/)?.[1] return urlSegment && !/^\d+$/.test(urlSegment) ? urlSegment : undefined } diff --git a/packages/e2e/tests/app-management-state.spec.ts b/packages/e2e/tests/app-management-state.spec.ts new file mode 100644 index 00000000000..a8b0f2d5d6f --- /dev/null +++ b/packages/e2e/tests/app-management-state.spec.ts @@ -0,0 +1,50 @@ +import {appDeletionReadinessFromApps} from '../setup/app-management-state.js' +import {expect, test} from '@playwright/test' +import type {AppManagementAppState} from '../setup/app-management-state.js' + +test.describe('App Management teardown state', () => { + test('returns the exact client ID match when app names collide', () => { + const matchingApp = appState({id: 'gid://organization/App/2', key: 'expected-client-id'}) + + expect( + appDeletionReadinessFromApps( + [appState({id: 'gid://organization/App/1', key: 'other-client-id'}), matchingApp], + 'E2E app', + 'expected-client-id', + ), + ).toEqual({status: 'ready', app: {id: matchingApp.id, key: matchingApp.key}}) + }) + + test('falls back to the unique name match when the local client ID is stale', () => { + const matchingApp = appState({id: 'gid://organization/App/1', key: 'current-client-id'}) + + expect(appDeletionReadinessFromApps([matchingApp], 'E2E app', 'stale-client-id')).toEqual({ + status: 'ready', + app: {id: matchingApp.id, key: matchingApp.key}, + }) + }) + + test('does not treat a missing install count as zero', () => { + expect(() => appDeletionReadinessFromApps([appState({installCount: null})], 'E2E app')).toThrow( + 'App Management API did not return installCount for E2E app', + ) + }) + + test('reports installed and deleted apps without ambiguity', () => { + expect(appDeletionReadinessFromApps([appState({installCount: 2})], 'E2E app')).toEqual({ + status: 'still-installed', + installCount: 2, + }) + expect(appDeletionReadinessFromApps([], 'E2E app')).toEqual({status: 'already-deleted'}) + }) +}) + +function appState(overrides: Partial = {}): AppManagementAppState { + return { + id: 'gid://organization/App/1', + key: 'client-id', + installCount: 0, + activeRelease: {version: {name: 'E2E app'}}, + ...overrides, + } +} diff --git a/packages/e2e/tests/teardown.spec.ts b/packages/e2e/tests/teardown.spec.ts new file mode 100644 index 00000000000..9b8426a242f --- /dev/null +++ b/packages/e2e/tests/teardown.spec.ts @@ -0,0 +1,186 @@ +import {runTeardown} from '../setup/teardown-orchestrator.js' +import {expect, test} from '@playwright/test' +import type {AppDeletionReadiness, CleanupPhaseRecord} from '../setup/teardown-orchestrator.js' + +test.describe('teardown orchestration', () => { + test('deletes a store-backed app in the required order', async () => { + const calls: string[] = [] + const records: CleanupPhaseRecord[] = [] + + await runTeardown({ + hasStore: true, + uninstallApp: async () => { + calls.push('uninstall-app') + }, + waitForAppDeletionReadiness: async () => { + calls.push('wait-for-zero-installs') + return readyApp() + }, + deleteApp: async () => { + calls.push('delete-app') + return true + }, + deleteStore: async () => { + calls.push('delete-store') + return true + }, + record: (record) => records.push(record), + }) + + expect(calls).toEqual(['uninstall-app', 'wait-for-zero-installs', 'delete-app', 'delete-store']) + expect(records.map(({phase, status}) => ({phase, status}))).toEqual([ + {phase: 'uninstall-app', status: 'completed'}, + {phase: 'wait-for-zero-installs', status: 'completed'}, + {phase: 'delete-app', status: 'completed'}, + {phase: 'delete-store', status: 'completed'}, + ]) + }) + + test('does not delete the store when app deletion fails', async () => { + const calls: string[] = [] + const records: CleanupPhaseRecord[] = [] + + await runTeardown({ + hasStore: true, + uninstallApp: async () => {}, + waitForAppDeletionReadiness: async () => readyApp(), + deleteApp: async () => { + calls.push('delete-app') + return false + }, + deleteStore: async () => { + calls.push('delete-store') + return true + }, + record: (record) => records.push(record), + }) + + expect(calls).toEqual(['delete-app']) + expect(records).toContainEqual({ + phase: 'delete-store', + status: 'skipped', + detail: 'app deletion was not confirmed', + }) + }) + + test('does not replace the test result when a cleanup phase throws', async () => { + const calls: string[] = [] + const records: CleanupPhaseRecord[] = [] + + await expect( + runTeardown({ + hasStore: true, + uninstallApp: async () => { + calls.push('uninstall-app') + throw new Error('uninstall failed') + }, + waitForAppDeletionReadiness: async () => { + calls.push('wait-for-zero-installs') + return readyApp() + }, + deleteApp: async () => { + calls.push('delete-app') + throw new Error('delete failed') + }, + deleteStore: async () => { + calls.push('delete-store') + return true + }, + record: (record) => records.push(record), + }), + ).resolves.toBeUndefined() + + expect(calls).toEqual(['uninstall-app', 'wait-for-zero-installs', 'delete-app']) + expect(records).toContainEqual({phase: 'uninstall-app', status: 'failed', detail: 'uninstall failed'}) + expect(records).toContainEqual({phase: 'delete-app', status: 'failed', detail: 'delete failed'}) + expect(records).toContainEqual({phase: 'delete-store', status: 'skipped', detail: 'app deletion failed'}) + }) + + test('does not treat unknown install state as zero installs', async () => { + const calls: string[] = [] + const records: CleanupPhaseRecord[] = [] + + await runTeardown({ + hasStore: true, + uninstallApp: async () => {}, + waitForAppDeletionReadiness: async () => { + throw new Error('query failed') + }, + deleteApp: async () => { + calls.push('delete-app') + return true + }, + deleteStore: async () => { + calls.push('delete-store') + return true + }, + record: (record) => records.push(record), + }) + + expect(calls).toEqual([]) + expect(records).toContainEqual({ + phase: 'wait-for-zero-installs', + status: 'failed', + detail: 'query failed', + }) + expect(records).toContainEqual({phase: 'delete-app', status: 'skipped', detail: 'installation state is unknown'}) + expect(records).toContainEqual({ + phase: 'delete-store', + status: 'skipped', + detail: 'installation state is unknown', + }) + }) + + test('does not delete resources while the app still has installs', async () => { + const calls: string[] = [] + const records: CleanupPhaseRecord[] = [] + + await runTeardown({ + hasStore: true, + uninstallApp: async () => {}, + waitForAppDeletionReadiness: async () => ({status: 'still-installed', installCount: 1}), + deleteApp: async () => { + calls.push('delete-app') + return true + }, + deleteStore: async () => { + calls.push('delete-store') + return true + }, + record: (record) => records.push(record), + }) + + expect(calls).toEqual([]) + expect(records).toContainEqual({ + phase: 'wait-for-zero-installs', + status: 'failed', + detail: 'app still has 1 install(s)', + }) + expect(records).toContainEqual({phase: 'delete-app', status: 'skipped', detail: 'app still has installs'}) + expect(records).toContainEqual({phase: 'delete-store', status: 'skipped', detail: 'app still has installs'}) + }) + + test('deletes the store when the app is already deleted', async () => { + const calls: string[] = [] + + await runTeardown({ + hasStore: true, + waitForAppDeletionReadiness: async () => ({status: 'already-deleted'}), + deleteApp: async () => { + calls.push('delete-app') + return true + }, + deleteStore: async () => { + calls.push('delete-store') + return true + }, + record: () => {}, + }) + + expect(calls).toEqual(['delete-store']) + }) +}) + +function readyApp(): AppDeletionReadiness { + return {status: 'ready', app: {id: 'gid://organization/App/1', key: 'client-id'}} +} From 723d154aa79ba5b5cfe99d5b665e846ba7946981 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 14 Aug 2026 11:59:09 +0200 Subject: [PATCH 7/8] Fix E2E button evaluation typing Assisted-By: devx/215513a9-13fa-4e74-bb8f-79faee6e4f09 --- packages/e2e/setup/store.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/e2e/setup/store.ts b/packages/e2e/setup/store.ts index 135aa7e228b..73378a69a8a 100644 --- a/packages/e2e/setup/store.ts +++ b/packages/e2e/setup/store.ts @@ -162,7 +162,7 @@ export async function uninstallAppFromStore(page: Page, storeSlug: string, appNa // Force a DOM click to bypass Playwright actionability (the button can read as // disabled mid-transition). - await confirmBtn.evaluate((button) => button.click()) + await confirmBtn.evaluate((button) => (button as HTMLButtonElement).click()) await page.waitForTimeout(BROWSER_TIMEOUT.medium) } From d5698aa98144634ccdd31ae7fe4db0b97feb7596 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Isaac=20Rold=C3=A1n?= Date: Fri, 14 Aug 2026 12:28:13 +0200 Subject: [PATCH 8/8] Simplify E2E teardown orchestration Assisted-By: devx/215513a9-13fa-4e74-bb8f-79faee6e4f09 --- packages/e2e/playwright.config.ts | 6 +- packages/e2e/setup/app-management-api.ts | 41 +++- packages/e2e/setup/app-management-state.ts | 32 --- .../e2e/setup/inspect-app-management-state.ts | 13 +- packages/e2e/setup/teardown-orchestrator.ts | 124 ------------ packages/e2e/setup/teardown.ts | 183 ++++++++++++----- ...ate.spec.ts => app-management-api.spec.ts} | 15 +- packages/e2e/tests/teardown.spec.ts | 186 ------------------ 8 files changed, 179 insertions(+), 421 deletions(-) delete mode 100644 packages/e2e/setup/app-management-state.ts delete mode 100644 packages/e2e/setup/teardown-orchestrator.ts rename packages/e2e/tests/{app-management-state.spec.ts => app-management-api.spec.ts} (75%) delete mode 100644 packages/e2e/tests/teardown.spec.ts diff --git a/packages/e2e/playwright.config.ts b/packages/e2e/playwright.config.ts index 3ebff191d00..2e8b62e894b 100644 --- a/packages/e2e/playwright.config.ts +++ b/packages/e2e/playwright.config.ts @@ -31,8 +31,7 @@ export default defineConfig({ 'tests/smoke-pty.spec.ts', 'tests/fixture-toml.spec.ts', 'tests/auth-diagnostics.spec.ts', - 'tests/app-management-state.spec.ts', - 'tests/teardown.spec.ts', + 'tests/app-management-api.spec.ts', ], }, { @@ -47,8 +46,7 @@ export default defineConfig({ 'tests/smoke-pty.spec.ts', 'tests/fixture-toml.spec.ts', 'tests/auth-diagnostics.spec.ts', - 'tests/app-management-state.spec.ts', - 'tests/teardown.spec.ts', + 'tests/app-management-api.spec.ts', ], dependencies: ['remote-auth'], }, diff --git a/packages/e2e/setup/app-management-api.ts b/packages/e2e/setup/app-management-api.ts index 417176d8d99..139117fb460 100644 --- a/packages/e2e/setup/app-management-api.ts +++ b/packages/e2e/setup/app-management-api.ts @@ -2,17 +2,26 @@ import {execa} from 'execa' import * as path from 'path' import {fileURLToPath} from 'url' -import type {AppDeletionReadiness} from './teardown-orchestrator.js' const __dirname = path.dirname(fileURLToPath(import.meta.url)) const RESULT_PREFIX = 'E2E_APP_MANAGEMENT_RESULT=' +export type AppDeletionReadiness = + | {status: 'ready'; app: {id: string; key: string}} + | {status: 'already-deleted'} + | {status: 'still-installed'; installCount: number} + +export interface AppManagementAppState { + id: string + key: string + installCount?: number | null + activeRelease: {version: {name: string}} +} + interface AppDeletionReadinessOptions { appName: string clientId?: string orgId: string - timeoutMs?: number - pollIntervalMs?: number } /** @@ -34,7 +43,7 @@ export async function waitForAppDeletionReadiness( preferLocal: true, localDir: path.resolve(__dirname, '..'), input: JSON.stringify(options), - timeout: (options.timeoutMs ?? 30_000) + 90_000, + timeout: 120_000, }) const resultLine = result.stdout.split('\n').findLast((line) => line.startsWith(RESULT_PREFIX)) @@ -44,3 +53,27 @@ export async function waitForAppDeletionReadiness( return JSON.parse(resultLine.slice(RESULT_PREFIX.length)) as AppDeletionReadiness } + +export function appDeletionReadinessFromApps( + apps: AppManagementAppState[], + appName: string, + clientId?: string, +): AppDeletionReadiness { + const exactNameMatches = apps.filter((app) => app.activeRelease.version.name === appName) + const clientIdMatches = clientId ? exactNameMatches.filter((app) => app.key === clientId) : [] + const matchingApps = clientIdMatches.length > 0 ? clientIdMatches : exactNameMatches + + if (matchingApps.length === 0) return {status: 'already-deleted'} + if (matchingApps.length > 1) { + throw new Error(`App Management API returned multiple apps named ${appName}`) + } + + const app = matchingApps[0]! + if (typeof app.installCount !== 'number') { + throw new Error(`App Management API did not return installCount for ${appName}`) + } + + return app.installCount === 0 + ? {status: 'ready', app: {id: app.id, key: app.key}} + : {status: 'still-installed', installCount: app.installCount} +} diff --git a/packages/e2e/setup/app-management-state.ts b/packages/e2e/setup/app-management-state.ts deleted file mode 100644 index 24b2d4f028e..00000000000 --- a/packages/e2e/setup/app-management-state.ts +++ /dev/null @@ -1,32 +0,0 @@ -import type {AppDeletionReadiness} from './teardown-orchestrator.js' - -export interface AppManagementAppState { - id: string - key: string - installCount?: number | null - activeRelease: {version: {name: string}} -} - -export function appDeletionReadinessFromApps( - apps: AppManagementAppState[], - appName: string, - clientId?: string, -): AppDeletionReadiness { - const exactNameMatches = apps.filter((app) => app.activeRelease.version.name === appName) - const clientIdMatches = clientId ? exactNameMatches.filter((app) => app.key === clientId) : [] - const matchingApps = clientIdMatches.length > 0 ? clientIdMatches : exactNameMatches - - if (matchingApps.length === 0) return {status: 'already-deleted'} - if (matchingApps.length > 1) { - throw new Error(`App Management API returned multiple apps named ${appName}`) - } - - const app = matchingApps[0]! - if (typeof app.installCount !== 'number') { - throw new Error(`App Management API did not return installCount for ${appName}`) - } - if (app.installCount === 0) { - return {status: 'ready', app: {id: app.id, key: app.key}} - } - return {status: 'still-installed', installCount: app.installCount} -} diff --git a/packages/e2e/setup/inspect-app-management-state.ts b/packages/e2e/setup/inspect-app-management-state.ts index e1008006f31..bfb8d71ec17 100644 --- a/packages/e2e/setup/inspect-app-management-state.ts +++ b/packages/e2e/setup/inspect-app-management-state.ts @@ -1,12 +1,11 @@ /* eslint-disable @nx/enforce-module-boundaries, no-await-in-loop -- this subprocess uses cli-kit's built API client with the isolated E2E session */ -import {appDeletionReadinessFromApps} from './app-management-state.js' +import {appDeletionReadinessFromApps} from './app-management-api.js' import {appManagementFqdn} from '../../cli-kit/dist/public/node/context/fqdn.js' import {graphqlRequest} from '../../cli-kit/dist/public/node/api/graphql.js' import {ensureAuthenticatedAppManagementAndBusinessPlatform} from '../../cli-kit/dist/public/node/session.js' import {loadtestHeaderRecord} from '../helpers/loadtest-header.js' -import type {AppDeletionReadiness} from './teardown-orchestrator.js' -import type {AppManagementAppState} from './app-management-state.js' +import type {AppDeletionReadiness, AppManagementAppState} from './app-management-api.js' const RESULT_PREFIX = 'E2E_APP_MANAGEMENT_RESULT=' @@ -14,8 +13,6 @@ interface InspectionOptions { appName: string clientId?: string orgId: string - timeoutMs?: number - pollIntervalMs?: number } interface AppsQueryResult { @@ -42,9 +39,7 @@ const query = ` ` const options = JSON.parse(await readStandardInput()) as InspectionOptions -const timeoutMs = options.timeoutMs ?? 30_000 -const pollIntervalMs = options.pollIntervalMs ?? 2_000 -const deadline = Date.now() + timeoutMs +const deadline = Date.now() + 30_000 const missingAppConfirmationsRequired = 2 const apiUrl = `https://${await appManagementFqdn()}/app_management/unstable/graphql.json` @@ -97,7 +92,7 @@ while (true) { if (readiness.status === 'already-deleted' && missingAppConfirmations >= missingAppConfirmationsRequired) break if (Date.now() >= deadline) break - await new Promise((resolve) => setTimeout(resolve, pollIntervalMs)) + await new Promise((resolve) => setTimeout(resolve, 2_000)) } process.stdout.write(`\n${RESULT_PREFIX}${JSON.stringify(readiness)}\n`) diff --git a/packages/e2e/setup/teardown-orchestrator.ts b/packages/e2e/setup/teardown-orchestrator.ts deleted file mode 100644 index 5c531835103..00000000000 --- a/packages/e2e/setup/teardown-orchestrator.ts +++ /dev/null @@ -1,124 +0,0 @@ -export type AppDeletionReadiness = - | {status: 'ready'; app: {id: string; key: string}} - | {status: 'already-deleted'} - | {status: 'still-installed'; installCount: number} - -export type CleanupPhase = 'uninstall-app' | 'wait-for-zero-installs' | 'delete-app' | 'delete-store' -export type CleanupPhaseStatus = 'completed' | 'failed' | 'skipped' - -export interface CleanupPhaseRecord { - phase: CleanupPhase - status: CleanupPhaseStatus - detail: string -} - -interface TeardownOperations { - hasStore: boolean - uninstallApp?: () => Promise - waitForAppDeletionReadiness: () => Promise - deleteApp: (app: {id: string; key: string}) => Promise - deleteStore?: () => Promise - record: (record: CleanupPhaseRecord) => void -} - -export async function runTeardown(operations: TeardownOperations): Promise { - if (operations.hasStore) { - if (operations.uninstallApp) { - try { - await operations.uninstallApp() - operations.record({phase: 'uninstall-app', status: 'completed', detail: 'app uninstalled'}) - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (error) { - operations.record({phase: 'uninstall-app', status: 'failed', detail: errorMessage(error)}) - } - } else { - operations.record({ - phase: 'uninstall-app', - status: 'skipped', - detail: 'app directory unavailable', - }) - } - } - - let readiness: AppDeletionReadiness - try { - readiness = await operations.waitForAppDeletionReadiness() - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (error) { - operations.record({phase: 'wait-for-zero-installs', status: 'failed', detail: errorMessage(error)}) - skipRemainingPhases(operations, 'installation state is unknown') - return - } - - if (readiness.status === 'still-installed') { - operations.record({ - phase: 'wait-for-zero-installs', - status: 'failed', - detail: `app still has ${readiness.installCount} install(s)`, - }) - skipRemainingPhases(operations, 'app still has installs') - return - } - - operations.record({ - phase: 'wait-for-zero-installs', - status: 'completed', - detail: readiness.status === 'already-deleted' ? 'app already deleted' : 'app has zero installs', - }) - - if (readiness.status === 'already-deleted') { - operations.record({phase: 'delete-app', status: 'completed', detail: 'app already deleted'}) - } else { - try { - const deleted = await operations.deleteApp(readiness.app) - operations.record({ - phase: 'delete-app', - status: deleted ? 'completed' : 'failed', - detail: deleted ? 'app deleted' : 'app deletion was not confirmed', - }) - if (!deleted) { - skipStoreDeletion(operations, 'app deletion was not confirmed') - return - } - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (error) { - operations.record({phase: 'delete-app', status: 'failed', detail: errorMessage(error)}) - skipStoreDeletion(operations, 'app deletion failed') - return - } - } - - if (!operations.hasStore) return - - if (!operations.deleteStore) { - operations.record({phase: 'delete-store', status: 'failed', detail: 'store deletion operation unavailable'}) - return - } - - try { - const deleted = await operations.deleteStore() - operations.record({ - phase: 'delete-store', - status: deleted ? 'completed' : 'failed', - detail: deleted ? 'store deletion requested' : 'store deletion was not confirmed', - }) - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (error) { - operations.record({phase: 'delete-store', status: 'failed', detail: errorMessage(error)}) - } -} - -function skipRemainingPhases(operations: TeardownOperations, detail: string): void { - operations.record({phase: 'delete-app', status: 'skipped', detail}) - skipStoreDeletion(operations, detail) -} - -function skipStoreDeletion(operations: TeardownOperations, detail: string): void { - if (operations.hasStore) { - operations.record({phase: 'delete-store', status: 'skipped', detail}) - } -} - -function errorMessage(error: unknown): string { - return error instanceof Error ? error.message : String(error) -} diff --git a/packages/e2e/setup/teardown.ts b/packages/e2e/setup/teardown.ts index 6840ff92dd7..b11c2d14ee1 100644 --- a/packages/e2e/setup/teardown.ts +++ b/packages/e2e/setup/teardown.ts @@ -2,11 +2,9 @@ import {uninstallAppWithAdminApi} from './admin-api.js' import {waitForAppDeletionReadiness} from './app-management-api.js' import {deleteAppFromDevDashboard, extractClientId} from './app.js' -import {runTeardown} from './teardown-orchestrator.js' import {e2eSection} from './env.js' import {BROWSER_TIMEOUT} from './constants.js' import {deleteDevStoreWithCli} from './store.js' -import type {CleanupPhaseRecord} from './teardown-orchestrator.js' import type {CLIProcess} from './cli.js' import type {E2EEnv} from './env.js' import type {Page} from '@playwright/test' @@ -26,6 +24,14 @@ type StoreTeardownContext = BaseTeardownContext & { type TeardownContext = StoreTeardownContext | (BaseTeardownContext & {storeFqdn?: undefined; appDir?: string}) +interface CleanupPhaseRecord { + phase: CleanupPhase + status: 'completed' | 'failed' | 'skipped' + detail: string +} + +type CleanupPhase = 'uninstall-app' | 'wait-for-zero-installs' | 'delete-app' | 'delete-store' + /** * Best-effort per-test teardown. * @@ -34,41 +40,118 @@ type TeardownContext = StoreTeardownContext | (BaseTeardownContext & {storeFqdn? * Every phase records its own result and teardown never replaces the test result. */ export async function teardownAll(context: TeardownContext): Promise { - const workerContext = {workerIndex: context.env.workerIndex} + const {workerIndex} = context.env const clientId = resolveClientId(context) const storeContext = hasStore(context) ? context : undefined - const appDir = storeContext?.appDir - - e2eSection(workerContext, `Teardown: app ${context.appName}`) - - await runTeardown({ - hasStore: Boolean(storeContext), - uninstallApp: - storeContext && appDir - ? () => - uninstallAppWithAdminApi({ - cli: storeContext.cli, - appDir, - storeFqdn: storeContext.storeFqdn, - }) - : undefined, - waitForAppDeletionReadiness: () => - waitForAppDeletionReadiness(context.env.processEnv, { - appName: context.appName, - clientId, - orgId: context.env.orgId, - }), - deleteApp: (app) => deleteAppWithRetry(context, app), - deleteStore: storeContext ? () => deleteStoreWithRetry(storeContext) : undefined, - record: (record) => recordCleanupPhase(context.env.workerIndex, record), + e2eSection({workerIndex}, `Teardown: app ${context.appName}`) + + if (storeContext) { + const {appDir} = storeContext + if (appDir) { + await runCleanupPhase(workerIndex, 'uninstall-app', 'app uninstalled', () => + uninstallAppWithAdminApi({ + cli: storeContext.cli, + appDir, + storeFqdn: storeContext.storeFqdn, + }), + ) + } else { + recordCleanupPhase(workerIndex, { + phase: 'uninstall-app', + status: 'skipped', + detail: 'app directory unavailable', + }) + } + } + + let readiness + try { + readiness = await waitForAppDeletionReadiness(context.env.processEnv, { + appName: context.appName, + clientId, + orgId: context.env.orgId, + }) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + recordCleanupPhase(workerIndex, { + phase: 'wait-for-zero-installs', + status: 'failed', + detail: errorMessage(error), + }) + recordSkippedDeletion(workerIndex, Boolean(storeContext), 'installation state is unknown') + return + } + + if (readiness.status === 'still-installed') { + recordCleanupPhase(workerIndex, { + phase: 'wait-for-zero-installs', + status: 'failed', + detail: `app still has ${readiness.installCount} install(s)`, + }) + recordSkippedDeletion(workerIndex, Boolean(storeContext), 'app still has installs') + return + } + + recordCleanupPhase(workerIndex, { + phase: 'wait-for-zero-installs', + status: 'completed', + detail: readiness.status === 'already-deleted' ? 'app already deleted' : 'app has zero installs', }) + + let appDeleted = true + if (readiness.status === 'already-deleted') { + recordCleanupPhase(workerIndex, {phase: 'delete-app', status: 'completed', detail: 'app already deleted'}) + } else { + appDeleted = await runCleanupPhase(workerIndex, 'delete-app', 'app deleted', () => + deleteAppWithRetry(context, readiness.app), + ) + } + + if (!appDeleted) { + if (storeContext) { + recordCleanupPhase(workerIndex, {phase: 'delete-store', status: 'skipped', detail: 'app deletion failed'}) + } + return + } + + if (storeContext) { + await runCleanupPhase(workerIndex, 'delete-store', 'store deletion requested', () => + deleteStoreWithRetry(storeContext), + ) + } } function hasStore(context: TeardownContext): context is StoreTeardownContext { return context.storeFqdn !== undefined } -async function deleteAppWithRetry(context: TeardownContext, app: {id: string; key: string}): Promise { +async function runCleanupPhase( + workerIndex: number, + phase: CleanupPhase, + detail: string, + operation: () => Promise, +): Promise { + try { + await operation() + recordCleanupPhase(workerIndex, {phase, status: 'completed', detail}) + return true + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (error) { + recordCleanupPhase(workerIndex, {phase, status: 'failed', detail: errorMessage(error)}) + return false + } +} + +function recordSkippedDeletion(workerIndex: number, hasStore: boolean, detail: string): void { + recordCleanupPhase(workerIndex, {phase: 'delete-app', status: 'skipped', detail}) + if (hasStore) recordCleanupPhase(workerIndex, {phase: 'delete-store', status: 'skipped', detail}) +} + +function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error) +} + +async function deleteAppWithRetry(context: TeardownContext, app: {id: string; key: string}): Promise { const numericAppId = app.id.match(/(\d+)$/)?.[1] // The dashboard exposes the Delete button on its numeric app route. Keep the // client-key URL only as a fallback when the API does not return a numeric GID. @@ -77,38 +160,38 @@ async function deleteAppWithRetry(context: TeardownContext, app: {id: string; ke ? `https://dev.shopify.com/dashboard/${context.env.orgId}/apps/${numericAppId}` : (context.appUrl ?? `https://dev.shopify.com/dashboard/${context.env.orgId}/apps/${app.key}`) - let lastError: unknown - for (let attempt = 1; attempt <= 3; attempt++) { - try { - if (await deleteAppFromDevDashboard(context.browserPage, appUrl)) return true - lastError = new Error('app deletion was not confirmed') - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (error) { - lastError = error - } - - if (attempt < 3) { - await context.browserPage.waitForTimeout(BROWSER_TIMEOUT.medium) - } - } + await retryCleanup( + async () => { + if (!(await deleteAppFromDevDashboard(context.browserPage, appUrl))) { + throw new Error('app deletion was not confirmed') + } + }, + () => context.browserPage.waitForTimeout(BROWSER_TIMEOUT.medium), + ) +} - throw lastError instanceof Error ? lastError : new Error(String(lastError)) +async function deleteStoreWithRetry(context: StoreTeardownContext): Promise { + await retryCleanup(async () => { + await deleteDevStoreWithCli({ + cli: context.cli, + storeFqdn: context.storeFqdn, + orgId: context.env.orgId, + }) + }) } -async function deleteStoreWithRetry(context: StoreTeardownContext): Promise { +async function retryCleanup(operation: () => Promise, waitBeforeRetry?: () => Promise): Promise { let lastError: unknown for (let attempt = 1; attempt <= 3; attempt++) { try { - await deleteDevStoreWithCli({ - cli: context.cli, - storeFqdn: context.storeFqdn, - orgId: context.env.orgId, - }) - return true + await operation() + return // eslint-disable-next-line no-catch-all/no-catch-all } catch (error) { lastError = error } + + if (attempt < 3) await waitBeforeRetry?.() } throw lastError instanceof Error ? lastError : new Error(String(lastError)) diff --git a/packages/e2e/tests/app-management-state.spec.ts b/packages/e2e/tests/app-management-api.spec.ts similarity index 75% rename from packages/e2e/tests/app-management-state.spec.ts rename to packages/e2e/tests/app-management-api.spec.ts index a8b0f2d5d6f..e104ada4d99 100644 --- a/packages/e2e/tests/app-management-state.spec.ts +++ b/packages/e2e/tests/app-management-api.spec.ts @@ -1,9 +1,9 @@ -import {appDeletionReadinessFromApps} from '../setup/app-management-state.js' +import {appDeletionReadinessFromApps} from '../setup/app-management-api.js' import {expect, test} from '@playwright/test' -import type {AppManagementAppState} from '../setup/app-management-state.js' +import type {AppManagementAppState} from '../setup/app-management-api.js' test.describe('App Management teardown state', () => { - test('returns the exact client ID match when app names collide', () => { + test('uses the client ID to disambiguate apps with the same name', () => { const matchingApp = appState({id: 'gid://organization/App/2', key: 'expected-client-id'}) expect( @@ -15,15 +15,6 @@ test.describe('App Management teardown state', () => { ).toEqual({status: 'ready', app: {id: matchingApp.id, key: matchingApp.key}}) }) - test('falls back to the unique name match when the local client ID is stale', () => { - const matchingApp = appState({id: 'gid://organization/App/1', key: 'current-client-id'}) - - expect(appDeletionReadinessFromApps([matchingApp], 'E2E app', 'stale-client-id')).toEqual({ - status: 'ready', - app: {id: matchingApp.id, key: matchingApp.key}, - }) - }) - test('does not treat a missing install count as zero', () => { expect(() => appDeletionReadinessFromApps([appState({installCount: null})], 'E2E app')).toThrow( 'App Management API did not return installCount for E2E app', diff --git a/packages/e2e/tests/teardown.spec.ts b/packages/e2e/tests/teardown.spec.ts deleted file mode 100644 index 9b8426a242f..00000000000 --- a/packages/e2e/tests/teardown.spec.ts +++ /dev/null @@ -1,186 +0,0 @@ -import {runTeardown} from '../setup/teardown-orchestrator.js' -import {expect, test} from '@playwright/test' -import type {AppDeletionReadiness, CleanupPhaseRecord} from '../setup/teardown-orchestrator.js' - -test.describe('teardown orchestration', () => { - test('deletes a store-backed app in the required order', async () => { - const calls: string[] = [] - const records: CleanupPhaseRecord[] = [] - - await runTeardown({ - hasStore: true, - uninstallApp: async () => { - calls.push('uninstall-app') - }, - waitForAppDeletionReadiness: async () => { - calls.push('wait-for-zero-installs') - return readyApp() - }, - deleteApp: async () => { - calls.push('delete-app') - return true - }, - deleteStore: async () => { - calls.push('delete-store') - return true - }, - record: (record) => records.push(record), - }) - - expect(calls).toEqual(['uninstall-app', 'wait-for-zero-installs', 'delete-app', 'delete-store']) - expect(records.map(({phase, status}) => ({phase, status}))).toEqual([ - {phase: 'uninstall-app', status: 'completed'}, - {phase: 'wait-for-zero-installs', status: 'completed'}, - {phase: 'delete-app', status: 'completed'}, - {phase: 'delete-store', status: 'completed'}, - ]) - }) - - test('does not delete the store when app deletion fails', async () => { - const calls: string[] = [] - const records: CleanupPhaseRecord[] = [] - - await runTeardown({ - hasStore: true, - uninstallApp: async () => {}, - waitForAppDeletionReadiness: async () => readyApp(), - deleteApp: async () => { - calls.push('delete-app') - return false - }, - deleteStore: async () => { - calls.push('delete-store') - return true - }, - record: (record) => records.push(record), - }) - - expect(calls).toEqual(['delete-app']) - expect(records).toContainEqual({ - phase: 'delete-store', - status: 'skipped', - detail: 'app deletion was not confirmed', - }) - }) - - test('does not replace the test result when a cleanup phase throws', async () => { - const calls: string[] = [] - const records: CleanupPhaseRecord[] = [] - - await expect( - runTeardown({ - hasStore: true, - uninstallApp: async () => { - calls.push('uninstall-app') - throw new Error('uninstall failed') - }, - waitForAppDeletionReadiness: async () => { - calls.push('wait-for-zero-installs') - return readyApp() - }, - deleteApp: async () => { - calls.push('delete-app') - throw new Error('delete failed') - }, - deleteStore: async () => { - calls.push('delete-store') - return true - }, - record: (record) => records.push(record), - }), - ).resolves.toBeUndefined() - - expect(calls).toEqual(['uninstall-app', 'wait-for-zero-installs', 'delete-app']) - expect(records).toContainEqual({phase: 'uninstall-app', status: 'failed', detail: 'uninstall failed'}) - expect(records).toContainEqual({phase: 'delete-app', status: 'failed', detail: 'delete failed'}) - expect(records).toContainEqual({phase: 'delete-store', status: 'skipped', detail: 'app deletion failed'}) - }) - - test('does not treat unknown install state as zero installs', async () => { - const calls: string[] = [] - const records: CleanupPhaseRecord[] = [] - - await runTeardown({ - hasStore: true, - uninstallApp: async () => {}, - waitForAppDeletionReadiness: async () => { - throw new Error('query failed') - }, - deleteApp: async () => { - calls.push('delete-app') - return true - }, - deleteStore: async () => { - calls.push('delete-store') - return true - }, - record: (record) => records.push(record), - }) - - expect(calls).toEqual([]) - expect(records).toContainEqual({ - phase: 'wait-for-zero-installs', - status: 'failed', - detail: 'query failed', - }) - expect(records).toContainEqual({phase: 'delete-app', status: 'skipped', detail: 'installation state is unknown'}) - expect(records).toContainEqual({ - phase: 'delete-store', - status: 'skipped', - detail: 'installation state is unknown', - }) - }) - - test('does not delete resources while the app still has installs', async () => { - const calls: string[] = [] - const records: CleanupPhaseRecord[] = [] - - await runTeardown({ - hasStore: true, - uninstallApp: async () => {}, - waitForAppDeletionReadiness: async () => ({status: 'still-installed', installCount: 1}), - deleteApp: async () => { - calls.push('delete-app') - return true - }, - deleteStore: async () => { - calls.push('delete-store') - return true - }, - record: (record) => records.push(record), - }) - - expect(calls).toEqual([]) - expect(records).toContainEqual({ - phase: 'wait-for-zero-installs', - status: 'failed', - detail: 'app still has 1 install(s)', - }) - expect(records).toContainEqual({phase: 'delete-app', status: 'skipped', detail: 'app still has installs'}) - expect(records).toContainEqual({phase: 'delete-store', status: 'skipped', detail: 'app still has installs'}) - }) - - test('deletes the store when the app is already deleted', async () => { - const calls: string[] = [] - - await runTeardown({ - hasStore: true, - waitForAppDeletionReadiness: async () => ({status: 'already-deleted'}), - deleteApp: async () => { - calls.push('delete-app') - return true - }, - deleteStore: async () => { - calls.push('delete-store') - return true - }, - record: () => {}, - }) - - expect(calls).toEqual(['delete-store']) - }) -}) - -function readyApp(): AppDeletionReadiness { - return {status: 'ready', app: {id: 'gid://organization/App/1', key: 'client-id'}} -}