From 9ef5dbaab0015d2330ca0e7595073a56d1ebf9f4 Mon Sep 17 00:00:00 2001 From: Phyllis Wu Date: Tue, 21 Apr 2026 09:22:47 -0400 Subject: [PATCH] E2E: automatic teardown polish --- packages/e2e/setup/app.ts | 262 ++++++++++++------ packages/e2e/setup/store.ts | 134 ++++----- packages/e2e/setup/teardown.ts | 70 +++-- packages/e2e/tests/app-deploy.spec.ts | 4 +- packages/e2e/tests/app-dev-server.spec.ts | 4 +- packages/e2e/tests/app-scaffold.spec.ts | 12 +- packages/e2e/tests/dev-hot-reload.spec.ts | 12 +- packages/e2e/tests/multi-config-dev.spec.ts | 8 +- .../e2e/tests/toml-config-invalid.spec.ts | 4 +- packages/e2e/tests/toml-config.spec.ts | 8 +- 10 files changed, 307 insertions(+), 211 deletions(-) diff --git a/packages/e2e/setup/app.ts b/packages/e2e/setup/app.ts index b958491185..aa43070c03 100644 --- a/packages/e2e/setup/app.ts +++ b/packages/e2e/setup/app.ts @@ -27,20 +27,16 @@ export async function createApp(ctx: { const template = ctx.template ?? 'reactRouter' const packageManager = ctx.packageManager ?? (process.env.E2E_PACKAGE_MANAGER as 'npm' | 'yarn' | 'pnpm' | 'bun') ?? 'pnpm' - - const args = [ - '--name', - name, - '--path', - parentDir, - '--package-manager', - packageManager, - '--local', - '--template', - template, - ] + // reactRouter/remix both require a --flavor or they'll hang on the language + // prompt in non-interactive runs. Default to javascript when template needs + // it. For `--template none` (extension-only) flavor is ignored. + const flavor = ctx.flavor ?? (template === 'none' ? undefined : 'javascript') + + const args = ['--template', template] + if (flavor) args.push('--flavor', flavor) + args.push('--name', name, '--package-manager', packageManager, '--local') if (ctx.orgId) args.push('--organization-id', ctx.orgId) - if (ctx.flavor) args.push('--flavor', ctx.flavor) + args.push('--path', parentDir) const result = await cli.execCreateApp(args, { env: {FORCE_COLOR: '0'}, @@ -116,8 +112,9 @@ export async function generateExtension( flavor?: string }, ): Promise { - const args = ['app', 'generate', 'extension', '--name', ctx.name, '--path', ctx.appDir, '--template', ctx.template] + const args = ['app', 'generate', 'extension', '--template', ctx.template] if (ctx.flavor) args.push('--flavor', ctx.flavor) + args.push('--name', ctx.name, '--path', ctx.appDir) return ctx.cli.exec(args, {timeout: CLI_TIMEOUT.long}) } @@ -134,12 +131,13 @@ export async function deployApp( noBuild?: boolean }, ): Promise { - const args = ['app', 'deploy', '--path', ctx.appDir] - if (ctx.force ?? true) args.push('--force') - if (ctx.noBuild) args.push('--no-build') + const args = ['app', 'deploy'] if (ctx.version) args.push('--version', ctx.version) if (ctx.message) args.push('--message', ctx.message) if (ctx.config) args.push('--config', ctx.config) + if (ctx.force ?? true) args.push('--force') + if (ctx.noBuild) args.push('--no-build') + args.push('--path', ctx.appDir) return ctx.cli.exec(args, {timeout: CLI_TIMEOUT.long}) } @@ -152,7 +150,7 @@ export async function appInfo(ctx: CLIContext): Promise<{ entrySourceFilePath: string }[] }> { - const result = await ctx.cli.exec(['app', 'info', '--path', ctx.appDir, '--json']) + const result = await ctx.cli.exec(['app', 'info', '--json', '--path', ctx.appDir]) if (result.exitCode !== 0) { throw new Error(`app info failed (exit ${result.exitCode}):\nstdout: ${result.stdout}\nstderr: ${result.stderr}`) } @@ -168,25 +166,124 @@ export async function functionRun( inputPath: string }, ): Promise { - return ctx.cli.exec(['app', 'function', 'run', '--path', ctx.appDir, '--input', ctx.inputPath], { + return ctx.cli.exec(['app', 'function', 'run', '--input', ctx.inputPath, '--path', ctx.appDir], { timeout: CLI_TIMEOUT.short, }) } -export async function versionsList(ctx: CLIContext): Promise { - return ctx.cli.exec(['app', 'versions', 'list', '--path', ctx.appDir, '--json'], { - timeout: CLI_TIMEOUT.short, - }) +export async function versionsList( + ctx: CLIContext & { + config?: string + }, +): Promise { + const args = ['app', 'versions', 'list', '--json'] + if (ctx.config) args.push('--config', ctx.config) + args.push('--path', ctx.appDir) + return ctx.cli.exec(args, {timeout: CLI_TIMEOUT.short}) } +/** + * Run `app config link` to create a brand-new app on Shopify interactively. + * Answers the prompts: + * "Which organization is this work for?" → filter by orgId → Enter + * "Create this project as a new app on Shopify?" → Yes (default) + * "App name" → appName + * "Configuration file name" → skipped via `--config` flag + * + * Env overrides (via PTY spawn): + * CI=undefined — drop the key so prompts render. + * Fixture default is CI=1; Ink's `is-in-ci` + * treats `'CI' in env` as CI even when ''. + * In CI mode Ink suppresses prompt frames + * (only emitted on unmount), so waitForOutput + * hangs until the process is killed. + * SHOPIFY_CLI_NEVER_USE_PARTNERS_API=1 — skip Partners client in fetchOrganizations. + * Without this, fetchOrganizations iterates + * AppManagement AND Partners sequentially. + * Partners requires SHOPIFY_CLI_PARTNERS_TOKEN + * (not set in OAuth-auth'd tests) and hangs + * for minutes trying to authenticate. The e2e + * test org (161686155) lives in AppManagement. + */ export async function configLink( ctx: CLIContext & { - clientId: string + appName: string + orgId: string + configName?: string }, ): Promise { - return ctx.cli.exec(['app', 'config', 'link', '--path', ctx.appDir, '--client-id', ctx.clientId], { - timeout: CLI_TIMEOUT.medium, + const args = ['app', 'config', 'link'] + // Pass configName as --config flag. link.ts → loadConfigurationFileName skips + // the "Configuration file name" prompt when options.configName is set, which + // also side-steps a painful interactive quirk: that prompt uses + // `initialAnswer = remoteApp.title`, so any text we write would be appended + // to the app name rather than replacing it. + if (ctx.configName) args.push('--config', ctx.configName) + args.push('--path', ctx.appDir) + + const proc = await ctx.cli.spawn(args, { + env: { + CI: undefined, + SHOPIFY_CLI_NEVER_USE_PARTNERS_API: '1', + }, }) + + // Short sleep so Ink's useInput hooks attach before we start writing. + // Without this, an Enter press arrives mid-mount and a subsequent render can + // flip the prompt state unexpectedly (e.g. turning a select into search mode). + const settle = (ms = 150) => new Promise((resolve) => setTimeout(resolve, ms)) + + try { + // The first prompt is either the multi-org selector or — when the account + // has only one org, or none of the orgs have existing apps — we jump + // straight to `createAsNewAppPrompt`. Race both. + const firstPrompt = await Promise.race([ + proc.waitForOutput('Which organization', CLI_TIMEOUT.medium).then(() => 'org' as const), + proc.waitForOutput('Create this project as a new app', CLI_TIMEOUT.medium).then(() => 'create' as const), + proc.waitForOutput('App name', CLI_TIMEOUT.medium).then(() => 'appName' as const), + ]) + + if (firstPrompt === 'org') { + // Type the orgId to filter the autocomplete prompt to exactly one match. + // selectOrganizationPrompt's label includes `(${org.id})` when duplicate + // org names exist (which is true for the e2e test account), so substring + // matching on the numeric ID is unique. Avoids relying on MRU ordering. + await settle() + proc.ptyProcess.write(ctx.orgId) + await settle() + proc.sendKey('\r') + // After org selection the CLI fetches apps for the chosen org. If + // the org has existing apps → "Create this project" prompt. If it has + // zero apps → selectOrCreateApp skips straight to appNamePrompt. + const next = await Promise.race([ + proc.waitForOutput('Create this project as a new app', CLI_TIMEOUT.medium).then(() => 'create' as const), + proc.waitForOutput('App name', CLI_TIMEOUT.medium).then(() => 'appName' as const), + ]) + if (next === 'create') { + await settle() + proc.sendKey('\r') + } + } else if (firstPrompt === 'create') { + await settle() + proc.sendKey('\r') + } + + // Wait for "App name" text prompt and submit the desired name. + // Important: Ink parses each PTY data event as ONE keypress. If we write + // "name\r" in one call, parseKeypress sees the whole string and treats + // it as text (not Enter), so the prompt never submits. We must write the + // text, wait for it to be consumed, then write \r separately. + await proc.waitForOutput('App name', CLI_TIMEOUT.medium) + await settle() + proc.ptyProcess.write(ctx.appName) + await settle() + proc.sendKey('\r') + + const exitCode = await proc.waitForExit(CLI_TIMEOUT.long) + return {exitCode, stdout: proc.getOutput(), stderr: ''} + } finally { + proc.kill() + } } // --------------------------------------------------------------------------- @@ -225,70 +322,69 @@ export async function findAppOnDevDashboard(page: Page, appName: string, orgId?: return null } -/** Delete an app from its dev dashboard settings page. Returns true if deleted, false if not. */ +/** + * Delete an app from its dev dashboard settings page. Returns true if deleted. + * Mirrors the resilience patterns from scripts/cleanup-apps.ts:deleteApp: + * - Outer 3-attempt retry loop around the whole flow (500/502 recovery) + * - scrollIntoViewIfNeeded on the "Delete app" button + * - Conditional "type DELETE" confirmation (some orgs/modals require it) + * - Verify via settings-page reload → expect 404 + */ export async function deleteAppFromDevDashboard(page: Page, appUrl: string): Promise { - // Step 1: Navigate to settings page - await page.goto(`${appUrl}/settings`, {waitUntil: 'domcontentloaded'}) - await page.waitForTimeout(BROWSER_TIMEOUT.medium) - await refreshIfPageError(page) - - // Step 2: Wait for "Delete app" button to be enabled, then click (retry with error check) - const deleteAppBtn = page.locator('button:has-text("Delete app")').first() - for (let attempt = 1; attempt <= 3; attempt++) { - if (await refreshIfPageError(page)) continue - const isDisabled = await deleteAppBtn.getAttribute('disabled').catch(() => 'true') - if (!isDisabled) break - await page.reload({waitUntil: 'domcontentloaded'}) - await page.waitForTimeout(BROWSER_TIMEOUT.medium) - } - - // Click the delete button — if it's not found, the page didn't load properly - const deleteClicked = await deleteAppBtn - .click({timeout: BROWSER_TIMEOUT.long}) - .then(() => true) - .catch(() => false) - if (!deleteClicked) return false - await page.waitForTimeout(BROWSER_TIMEOUT.medium) - - // Step 3: Click confirm "Delete" in the modal (retry step 2+3 if not visible) - // The dev dashboard modal has a submit button with class "critical" inside a form - const confirmAppBtn = page.locator('button.critical[type="submit"]') for (let attempt = 1; attempt <= 3; attempt++) { - if (await confirmAppBtn.isVisible({timeout: BROWSER_TIMEOUT.medium}).catch(() => false)) break - if (attempt === 3) return false - // Retry: re-click the delete button to reopen modal - await page.keyboard.press('Escape') - await page.waitForTimeout(BROWSER_TIMEOUT.short) - const retryClicked = await deleteAppBtn - .click({timeout: BROWSER_TIMEOUT.long}) - .then(() => true) - .catch(() => false) - if (!retryClicked) return false - await page.waitForTimeout(BROWSER_TIMEOUT.medium) - } - - const urlBefore = page.url() - const confirmClicked = await confirmAppBtn - .click({timeout: BROWSER_TIMEOUT.long}) - .then(() => true) - .catch(() => false) - if (!confirmClicked) return false + try { + await page.goto(`${appUrl}/settings`, {waitUntil: 'domcontentloaded'}) + await page.waitForTimeout(BROWSER_TIMEOUT.medium) - // Wait for page to navigate away after deletion - try { - await page.waitForURL((url) => url.toString() !== urlBefore, {timeout: BROWSER_TIMEOUT.max}) - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (_err) { - // URL didn't change — check if page error occurred during redirect - if (await refreshIfPageError(page)) { - // After refresh, 404 means the app was deleted (settings page no longer exists) + // 404 before we even click = app already gone. 500/502 = throw to retry. const bodyText = (await page.textContent('body')) ?? '' if (bodyText.includes('404: Not Found')) return true - return false + if (bodyText.includes('500: Internal Server Error') || bodyText.includes('502 Bad Gateway')) { + throw new Error('Server error loading app settings page') + } + await refreshIfPageError(page) + + // If uninstall succeeded, the button enables within ~1-2s. Reload once + // as a fallback for propagation lag; click()'s auto-wait handles the + // rest at ~100ms granularity. Button can be below the fold. + const deleteBtn = page.locator('button:has-text("Delete app")').first() + await deleteBtn.scrollIntoViewIfNeeded() + if (!(await deleteBtn.isEnabled())) { + await page.reload({waitUntil: 'domcontentloaded'}) + await page.waitForTimeout(BROWSER_TIMEOUT.medium) + await deleteBtn.scrollIntoViewIfNeeded() + } + // 10s: generous budget for click auto-wait + await deleteBtn.click({timeout: 2 * BROWSER_TIMEOUT.long}) + await page.waitForTimeout(BROWSER_TIMEOUT.medium) + + // Some confirmation modals require typing "DELETE". Fill if present. + const confirmInput = page.locator('input[type="text"]').last() + if (await confirmInput.isVisible({timeout: BROWSER_TIMEOUT.medium}).catch(() => false)) { + await confirmInput.fill('DELETE') + await page.waitForTimeout(BROWSER_TIMEOUT.short) + } + + // Confirm button is the second "Delete app" button on the page (inside modal). + const confirmBtn = page.locator('button:has-text("Delete app")').last() + await confirmBtn.click({timeout: BROWSER_TIMEOUT.long}) + await page.waitForTimeout(BROWSER_TIMEOUT.medium) + + // Verify by reloading settings — 404 means deleted. + await page.goto(`${appUrl}/settings`, {waitUntil: 'domcontentloaded'}) + await page.waitForTimeout(BROWSER_TIMEOUT.short) + const afterText = (await page.textContent('body')) ?? '' + if (afterText.includes('404: Not Found')) return true + if (afterText.includes('500: Internal Server Error') || afterText.includes('502 Bad Gateway')) { + throw new Error('Server error verifying app deletion') + } + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (_err) { + if (attempt === 3) return false + await page.waitForTimeout(BROWSER_TIMEOUT.medium) } - await page.waitForTimeout(BROWSER_TIMEOUT.medium) } - return page.url() !== urlBefore + return false } // --------------------------------------------------------------------------- diff --git a/packages/e2e/setup/store.ts b/packages/e2e/setup/store.ts index 51507c98e0..6b71dfdd5b 100644 --- a/packages/e2e/setup/store.ts +++ b/packages/e2e/setup/store.ts @@ -167,20 +167,16 @@ export async function uninstallAppFromStore(page: Page, storeSlug: string, appNa await page.waitForTimeout(BROWSER_TIMEOUT.medium) } - // Verify: check the specific app is gone - const check = async () => - page - .locator(`span:has-text("${appName}"):not([class*="Polaris"])`) - .first() - .isVisible({timeout: BROWSER_TIMEOUT.medium}) - .catch(() => false) - - if (!(await check())) return true - - // If still visible — reload and check again + // Verify: reload and confirm the app is no longer listed await page.reload({waitUntil: 'domcontentloaded'}) await page.waitForTimeout(BROWSER_TIMEOUT.long) - return !(await check()) + await dismissDevConsole(page) + const stillVisible = await page + .locator(`span:has-text("${appName}"):not([class*="Polaris"])`) + .first() + .isVisible({timeout: BROWSER_TIMEOUT.medium}) + .catch(() => false) + return !stillVisible } /** Check if the current page shows the empty state (zero apps installed). Caller must navigate first. */ @@ -197,74 +193,82 @@ export async function isStoreAppsEmpty(page: Page): Promise { /** * Delete a store via the admin settings plan page. * Caller must verify no apps are installed before calling. + * Retries the full flow if the page redirects to store home instead of access_account. * Returns true if deleted, false if not. */ export async function deleteStore(page: Page, storeSlug: string): Promise { - // Step 1: Navigate to plan page and click delete button to open modal (retry navigation on failure) const planUrl = `https://admin.shopify.com/store/${storeSlug}/settings/plan` - const deleteButton = page.locator('s-internal-button[tone="critical"]').locator('button') for (let attempt = 1; attempt <= 3; attempt++) { try { - await page.goto(planUrl, {waitUntil: 'domcontentloaded'}) - await page.waitForTimeout(BROWSER_TIMEOUT.long) + // Step 1: Navigate to plan page (wait for full hydration before clicking) + await page.goto(planUrl, {waitUntil: 'load'}) + await page.waitForTimeout(2 * BROWSER_TIMEOUT.long) + // If redirected to access_account, store is already deleted if (page.url().includes('access_account')) return true - await deleteButton.click({timeout: BROWSER_TIMEOUT.long}) - break - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (_err) { - if (attempt === 3) return false - } - } - await page.waitForTimeout(BROWSER_TIMEOUT.medium) - - const modal = page.locator('.Polaris-Modal-Dialog__Modal') - // Step 2: Check the confirmation checkbox (retry step 1+2 if fails) - for (let attempt = 1; attempt <= 3; attempt++) { - const checkbox = modal.locator('input[type="checkbox"]') - if (await checkbox.isVisible({timeout: BROWSER_TIMEOUT.medium}).catch(() => false)) { + // Step 2: Click delete button → wait for modal (retry click if modal doesn't appear) + const deleteButton = page.locator('s-internal-button[tone="critical"]').locator('button') + // Scope to the delete modal by title to avoid matching stale/hidden modals + const modal = page.locator('.Polaris-Modal-Dialog__Modal:has-text("Review before deleting store")') + const checkbox = modal.locator('input[type="checkbox"]') + + let modalOpened = false + for (let clickAttempt = 1; clickAttempt <= 3; clickAttempt++) { + await deleteButton.click({timeout: BROWSER_TIMEOUT.long}) + await page.waitForTimeout(BROWSER_TIMEOUT.medium) + + if (await checkbox.isVisible({timeout: BROWSER_TIMEOUT.long}).catch(() => false)) { + modalOpened = true + break + } + // Modal didn't open — first click often refreshes the page. + // Don't re-navigate; the page already reloaded. Just wait for hydration and retry. + await page.waitForTimeout(BROWSER_TIMEOUT.long) + } + if (!modalOpened) throw new Error('Delete modal did not appear after 3 clicks') await checkbox.check({force: true}) await page.waitForTimeout(BROWSER_TIMEOUT.short) - break - } - if (attempt === 3) return false - // Retry: close modal and re-click delete - await page.keyboard.press('Escape') - await page.waitForTimeout(BROWSER_TIMEOUT.short) - await deleteButton.click({timeout: BROWSER_TIMEOUT.max}) - await page.waitForTimeout(BROWSER_TIMEOUT.medium) - } - // Step 3: Click confirm (retry step 2+3 if button is still disabled) - const confirmButton = modal.locator('button:has-text("Delete store")') - for (let attempt = 1; attempt <= 3; attempt++) { - const isDisabled = await confirmButton - .evaluate((el) => el.getAttribute('aria-disabled') === 'true' || el.hasAttribute('disabled')) - .catch(() => true) - if (!isDisabled) break - if (attempt === 3) return false - // Retry: re-check the checkbox - const checkbox = modal.locator('input[type="checkbox"]') - await checkbox.check({force: true}) - await page.waitForTimeout(BROWSER_TIMEOUT.short) - } + // Step 3: Click confirm + const confirmButton = modal.locator('button:has-text("Delete store")') + for (let i = 1; i <= 3; i++) { + const isDisabled = await confirmButton + .evaluate((el) => el.getAttribute('aria-disabled') === 'true' || el.hasAttribute('disabled')) + .catch(() => true) + if (!isDisabled) break + if (i === 3) throw new Error('Confirm button still disabled') + await checkbox.check({force: true}) + await page.waitForTimeout(BROWSER_TIMEOUT.short) + } - const confirmClicked = await confirmButton - .click({force: true}) - .then(() => true) - .catch(() => false) - if (!confirmClicked) return false - - // Verify: URL reaching access_account confirms store is deleted - try { - await page.waitForURL(/access_account/, {timeout: BROWSER_TIMEOUT.max}) - return true - // eslint-disable-next-line no-catch-all/no-catch-all - } catch (_err) { - return false + await confirmButton.click({force: true}) + + // Step 4: Verify — wait for either access_account (success) or store home (failure). + // Waiting only for access_account would burn the full 60s budget on silent + // failures. Matching either outcome returns fast in both cases. + try { + await page.waitForURL( + (url) => url.toString().includes('access_account') || url.pathname.match(/^\/store\/[^/]+\/?$/) !== null, + {timeout: BROWSER_TIMEOUT.max}, + ) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (_err) { + // Timeout — neither redirect happened + } + + if (page.url().includes('access_account')) return true + + // Landed on store home — deletion failed silently, will retry + throw new Error(`Store deletion not confirmed (url: ${page.url()})`) + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (_err) { + if (attempt === 3) return false + // Retry — next iteration re-navigates to planUrl + } } + return false } // --------------------------------------------------------------------------- diff --git a/packages/e2e/setup/teardown.ts b/packages/e2e/setup/teardown.ts index ea28db8737..c93b931884 100644 --- a/packages/e2e/setup/teardown.ts +++ b/packages/e2e/setup/teardown.ts @@ -18,15 +18,14 @@ interface TeardownCtx { } /** - * Best-effort per-test teardown with escalating retry. - * - * Each phase is independent — failure never prevents later phases. - * Escalation: retry same step → go back one step and retry both. + * Best-effort per-test teardown. Each phase retries internally up to 3 times. * * App + store flow: * Phase 1: uninstall app from store admin - * Phase 2: delete store (escalates to phase 1 on failure) - * Phase 3: delete app from dev dashboard (always runs) + * Phase 2: delete store (skipped if apps remain on the store) + * Phase 3: delete app from dev dashboard (skipped if phase 1 failed — + * button stays disabled while installs exist; cleanup-apps.ts + * reaps the orphan instead) * * App-only flow: * Phase 3 only @@ -60,24 +59,20 @@ export async function teardownAll(ctx: TeardownCtx): Promise { log.error(wCtx, 'app uninstall failed after 3 attempts') } - // Phase 2: Delete store + // Phase 2: Delete store (deleteStore retries the full flow internally up to 3x) log.log(wCtx, 'deleting store') let storeDeleted = false - for (let attempt = 1; attempt <= 3; attempt++) { - try { - // Navigate to apps page to check state - await page.goto(`https://admin.shopify.com/store/${storeSlug}/settings/apps`, { - waitUntil: 'domcontentloaded', - }) - await page.waitForTimeout(BROWSER_TIMEOUT.long) - - // Store already deleted? - if (page.url().includes('access_account')) { - log.log(wCtx, 'store already deleted') - storeDeleted = true - break - } + try { + // Check state from apps page first + 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') + storeDeleted = true + } else { await dismissDevConsole(page) // Apps still installed? Reload once in case page is stale @@ -85,31 +80,32 @@ export async function teardownAll(ctx: TeardownCtx): Promise { await page.reload({waitUntil: 'domcontentloaded'}) await page.waitForTimeout(BROWSER_TIMEOUT.long) await dismissDevConsole(page) - if (!(await isStoreAppsEmpty(page))) { - log.error(wCtx, 'store still has apps installed, skipping delete') - break - } } - // Safe to delete - const deleted = await deleteStore(page, storeSlug) - if (deleted) { - log.log(wCtx, 'store deleted') - storeDeleted = true - break + if (await isStoreAppsEmpty(page)) { + storeDeleted = await deleteStore(page, storeSlug) + log.log(wCtx, storeDeleted ? 'store deleted' : 'store deletion failed') + } else { + log.error(wCtx, 'store still has apps installed, skipping delete') } - log.log(wCtx, `(${attempt}/3) store deletion failed`) - // 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}`) } + // eslint-disable-next-line no-catch-all/no-catch-all + } catch (err) { + log.log(wCtx, `store deletion failed: ${err instanceof Error ? err.message : err}`) } if (!storeDeleted) { - log.error(wCtx, 'store deletion failed after 3 attempts') + log.error(wCtx, 'store deletion failed') + } + + // If uninstall failed, the dev dashboard's Delete button will never enable (app still has installs). + // Skip phase 3 to avoid wasting time polling a perma-disabled button — cleanup-apps.ts can handle the orphan. + if (!uninstalled) { + log.log(wCtx, 'skipping app delete — uninstall failed, run `pnpm test:e2e-cleanup-apps` after') + return } } - // Phase 3: Delete app from dev dashboard — ALWAYS runs + // Phase 3: Delete app from dev dashboard e2eSection(wCtx, `Teardown: app ${ctx.appName}`) log.log(wCtx, 'deleting app') let appDeleted = false diff --git a/packages/e2e/tests/app-deploy.spec.ts b/packages/e2e/tests/app-deploy.spec.ts index a02bb93a92..e3505b805c 100644 --- a/packages/e2e/tests/app-deploy.spec.ts +++ b/packages/e2e/tests/app-deploy.spec.ts @@ -41,8 +41,8 @@ test.describe('App deploy', () => { expect(listResult.exitCode, `versions list failed:\n${listOutput}`).toBe(0) expect(listOutput).toContain(versionTag) } finally { - // E2E_SKIP_CLEANUP=1 skips cleanup for debugging. Run `pnpm test:e2e-cleanup` afterward. - if (!process.env.E2E_SKIP_CLEANUP) { + // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. + if (!process.env.E2E_SKIP_TEARDOWN) { fs.rmSync(parentDir, {recursive: true, force: true}) await teardownAll({ browserPage, diff --git a/packages/e2e/tests/app-dev-server.spec.ts b/packages/e2e/tests/app-dev-server.spec.ts index 846b987d25..ae8305debb 100644 --- a/packages/e2e/tests/app-dev-server.spec.ts +++ b/packages/e2e/tests/app-dev-server.spec.ts @@ -49,8 +49,8 @@ test.describe('App dev server', () => { const exitCode = await dev.waitForExit(CLI_TIMEOUT.short) expect(exitCode, `dev exited with non-zero code. Output:\n${dev.getOutput()}`).toBe(0) } finally { - // E2E_SKIP_CLEANUP=1 skips cleanup for debugging. Run `pnpm test:e2e-cleanup` afterward. - if (!process.env.E2E_SKIP_CLEANUP) { + // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. + if (!process.env.E2E_SKIP_TEARDOWN) { fs.rmSync(parentDir, {recursive: true, force: true}) await teardownAll({ browserPage, diff --git a/packages/e2e/tests/app-scaffold.spec.ts b/packages/e2e/tests/app-scaffold.spec.ts index 790d370c16..a53d9d6622 100644 --- a/packages/e2e/tests/app-scaffold.spec.ts +++ b/packages/e2e/tests/app-scaffold.spec.ts @@ -45,8 +45,8 @@ test.describe('App scaffold', () => { `buildApp failed:\nstdout: ${buildResult.stdout}\nstderr: ${buildResult.stderr}`, ).toBe(0) } finally { - // E2E_SKIP_CLEANUP=1 skips cleanup for debugging. Run `pnpm test:e2e-cleanup` afterward. - if (!process.env.E2E_SKIP_CLEANUP) { + // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. + if (!process.env.E2E_SKIP_TEARDOWN) { fs.rmSync(parentDir, {recursive: true, force: true}) await teardownAll({ browserPage, @@ -80,8 +80,8 @@ test.describe('App scaffold', () => { expect(fs.existsSync(initResult.appDir)).toBe(true) expect(fs.existsSync(path.join(initResult.appDir, 'shopify.app.toml'))).toBe(true) } finally { - // E2E_SKIP_CLEANUP=1 skips cleanup for debugging. Run `pnpm test:e2e-cleanup` afterward. - if (!process.env.E2E_SKIP_CLEANUP) { + // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. + if (!process.env.E2E_SKIP_TEARDOWN) { fs.rmSync(parentDir, {recursive: true, force: true}) await teardownAll({ browserPage, @@ -138,8 +138,8 @@ test.describe('App scaffold', () => { `buildApp failed:\nstdout: ${buildResult.stdout}\nstderr: ${buildResult.stderr}`, ).toBe(0) } finally { - // E2E_SKIP_CLEANUP=1 skips cleanup for debugging. Run `pnpm test:e2e-cleanup` afterward. - if (!process.env.E2E_SKIP_CLEANUP) { + // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. + if (!process.env.E2E_SKIP_TEARDOWN) { fs.rmSync(parentDir, {recursive: true, force: true}) await teardownAll({ browserPage, diff --git a/packages/e2e/tests/dev-hot-reload.spec.ts b/packages/e2e/tests/dev-hot-reload.spec.ts index c3f53de191..b16c9c6d5b 100644 --- a/packages/e2e/tests/dev-hot-reload.spec.ts +++ b/packages/e2e/tests/dev-hot-reload.spec.ts @@ -83,8 +83,8 @@ test.describe('Dev hot reload', () => { proc.kill() } } finally { - // E2E_SKIP_CLEANUP=1 skips cleanup for debugging. Run `pnpm test:e2e-cleanup` afterward. - if (!process.env.E2E_SKIP_CLEANUP) { + // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. + if (!process.env.E2E_SKIP_TEARDOWN) { fs.rmSync(parentDir, {recursive: true, force: true}) await teardownAll({ browserPage, @@ -135,8 +135,8 @@ test.describe('Dev hot reload', () => { proc.kill() } } finally { - // E2E_SKIP_CLEANUP=1 skips cleanup for debugging. Run `pnpm test:e2e-cleanup` afterward. - if (!process.env.E2E_SKIP_CLEANUP) { + // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. + if (!process.env.E2E_SKIP_TEARDOWN) { fs.rmSync(parentDir, {recursive: true, force: true}) await teardownAll({ browserPage, @@ -193,8 +193,8 @@ test.describe('Dev hot reload', () => { proc.kill() } } finally { - // E2E_SKIP_CLEANUP=1 skips cleanup for debugging. Run `pnpm test:e2e-cleanup` afterward. - if (!process.env.E2E_SKIP_CLEANUP) { + // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. + if (!process.env.E2E_SKIP_TEARDOWN) { fs.rmSync(parentDir, {recursive: true, force: true}) await teardownAll({ browserPage, diff --git a/packages/e2e/tests/multi-config-dev.spec.ts b/packages/e2e/tests/multi-config-dev.spec.ts index cbb8efd6c2..7084cdb471 100644 --- a/packages/e2e/tests/multi-config-dev.spec.ts +++ b/packages/e2e/tests/multi-config-dev.spec.ts @@ -84,8 +84,8 @@ include_config_on_deploy = true proc.kill() } } finally { - // E2E_SKIP_CLEANUP=1 skips cleanup for debugging. Run `pnpm test:e2e-cleanup` afterward. - if (!process.env.E2E_SKIP_CLEANUP) { + // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. + if (!process.env.E2E_SKIP_TEARDOWN) { fs.rmSync(parentDir, {recursive: true, force: true}) await teardownAll({ browserPage, @@ -156,8 +156,8 @@ api_version = "2025-01" proc.kill() } } finally { - // E2E_SKIP_CLEANUP=1 skips cleanup for debugging. Run `pnpm test:e2e-cleanup` afterward. - if (!process.env.E2E_SKIP_CLEANUP) { + // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. + if (!process.env.E2E_SKIP_TEARDOWN) { fs.rmSync(parentDir, {recursive: true, force: true}) await teardownAll({ browserPage, diff --git a/packages/e2e/tests/toml-config-invalid.spec.ts b/packages/e2e/tests/toml-config-invalid.spec.ts index 7d8cbbf05e..f0bee68d64 100644 --- a/packages/e2e/tests/toml-config-invalid.spec.ts +++ b/packages/e2e/tests/toml-config-invalid.spec.ts @@ -36,8 +36,8 @@ test.describe('TOML config invalid', () => { expect(result.exitCode, `expected deploy to fail for ${label}, but it succeeded:\n${output}`).not.toBe(0) expect(output.toLowerCase(), `expected error output for ${label}:\n${output}`).toMatch(/error|invalid|failed/) } finally { - // E2E_SKIP_CLEANUP=1 skips cleanup for debugging. Run `pnpm test:e2e-cleanup` afterward. - if (!process.env.E2E_SKIP_CLEANUP) { + // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. + if (!process.env.E2E_SKIP_TEARDOWN) { fs.rmSync(appDir, {recursive: true, force: true}) } } diff --git a/packages/e2e/tests/toml-config.spec.ts b/packages/e2e/tests/toml-config.spec.ts index f3ff4f854c..8d1fa0c65c 100644 --- a/packages/e2e/tests/toml-config.spec.ts +++ b/packages/e2e/tests/toml-config.spec.ts @@ -35,8 +35,8 @@ test.describe('TOML config regression', () => { const output = result.stdout + result.stderr expect(result.exitCode, `deploy failed:\n${output}`).toBe(0) } finally { - // E2E_SKIP_CLEANUP=1 skips cleanup for debugging. Run `pnpm test:e2e-cleanup` afterward. - if (!process.env.E2E_SKIP_CLEANUP) { + // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. + if (!process.env.E2E_SKIP_TEARDOWN) { fs.rmSync(parentDir, {recursive: true, force: true}) await teardownAll({ browserPage, @@ -77,8 +77,8 @@ test.describe('TOML config regression', () => { proc.kill() } } finally { - // E2E_SKIP_CLEANUP=1 skips cleanup for debugging. Run `pnpm test:e2e-cleanup` afterward. - if (!process.env.E2E_SKIP_CLEANUP) { + // E2E_SKIP_TEARDOWN=1 skips teardown for debugging. Run cleanup scripts afterward. + if (!process.env.E2E_SKIP_TEARDOWN) { fs.rmSync(parentDir, {recursive: true, force: true}) await teardownAll({ browserPage,