From 46ad6ef6f695da76f96c85cee6d7ab962b81c5ff Mon Sep 17 00:00:00 2001 From: yyh <92089059+lyzno1@users.noreply.github.com> Date: Tue, 14 Jul 2026 14:15:57 +0800 Subject: [PATCH] fix(e2e): harden failure lifecycle diagnostics (#38896) --- .github/workflows/web-e2e.yml | 10 +- e2e/AGENTS.md | 2 + .../agent-v2/configure.steps.ts | 14 -- .../step-definitions/apps/delete-app.steps.ts | 6 +- e2e/features/support/hooks.ts | 132 +++++++++++------- e2e/features/support/world.ts | 49 +++---- e2e/scripts/setup.ts | 1 + e2e/support/api.ts | 3 +- e2e/support/cleanup.ts | 18 +++ e2e/tests/cleanup.test.ts | 49 +++++++ 10 files changed, 186 insertions(+), 98 deletions(-) create mode 100644 e2e/support/cleanup.ts create mode 100644 e2e/tests/cleanup.test.ts diff --git a/.github/workflows/web-e2e.yml b/.github/workflows/web-e2e.yml index 80798668189..43108462a09 100644 --- a/.github/workflows/web-e2e.yml +++ b/.github/workflows/web-e2e.yml @@ -97,6 +97,11 @@ jobs: mv cucumber-report cucumber-report-non-external fi + if [[ -d .logs ]]; then + rm -rf .logs-non-external + mv .logs .logs-non-external + fi + trap 'vp run e2e:middleware:down' EXIT vp run e2e:middleware:up vp run e2e:external:prepare @@ -117,5 +122,8 @@ jobs: uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: e2e-logs - path: e2e/.logs + path: | + e2e/.logs/*.log + e2e/.logs-non-external/*.log + include-hidden-files: true retention-days: 7 diff --git a/e2e/AGENTS.md b/e2e/AGENTS.md index d5bcb0bc868..10eacdbe44e 100644 --- a/e2e/AGENTS.md +++ b/e2e/AGENTS.md @@ -127,6 +127,7 @@ This removes: - `docker/volumes/plugin_daemon` - `e2e/.auth` - `e2e/.logs` +- `e2e/.logs-non-external` - `e2e/cucumber-report` Start the full middleware stack: @@ -171,6 +172,7 @@ Artifacts and diagnostics: - `cucumber-report/artifacts/`: failure screenshots and HTML captures - `.logs/cucumber-api.log`: backend startup log - `.logs/cucumber-web.log`: frontend startup log +- `.logs-non-external/`: non-external logs preserved before an external CI run Open the HTML report locally with: diff --git a/e2e/features/step-definitions/agent-v2/configure.steps.ts b/e2e/features/step-definitions/agent-v2/configure.steps.ts index 080bc6662df..9a7e83eba24 100644 --- a/e2e/features/step-definitions/agent-v2/configure.steps.ts +++ b/e2e/features/step-definitions/agent-v2/configure.steps.ts @@ -32,19 +32,6 @@ import { const concurrentAgentPrompts = [concurrentFirstAgentPrompt, concurrentSecondAgentPrompt] -function attachPageDiagnostics(world: DifyWorld, page: Page) { - page.setDefaultTimeout(30_000) - page.on('console', (message) => { - if (message.type() === 'error') world.consoleErrors.push(message.text()) - }) - page.on('pageerror', (error) => { - world.pageErrors.push(error.message) - }) - page.on('download', (dl) => { - world.capturedDownloads.push(dl) - }) -} - const getPromptEditor = (page: Page) => page.getByRole('region', { name: 'Prompt' }).getByRole('textbox', { name: 'Prompt' }) @@ -242,7 +229,6 @@ When('I open the same Agent v2 configure page in another tab', async function (t const agentId = getCurrentAgentId(this) const concurrentPage = await this.context.newPage() - attachPageDiagnostics(this, concurrentPage) this.agentBuilder.configure.concurrentPage = concurrentPage await concurrentPage.goto(getAgentConfigurePath(agentId)) diff --git a/e2e/features/step-definitions/apps/delete-app.steps.ts b/e2e/features/step-definitions/apps/delete-app.steps.ts index e5a5da424aa..6179857e543 100644 --- a/e2e/features/step-definitions/apps/delete-app.steps.ts +++ b/e2e/features/step-definitions/apps/delete-app.steps.ts @@ -23,13 +23,15 @@ When('I confirm the deletion', async function (this: DifyWorld) { Then('the app should no longer appear in the apps console', async function (this: DifyWorld) { const appName = this.lastCreatedAppName - if (!appName) { + const appId = this.createdAppIds.at(-1) + if (!appName || !appId) { throw new Error( - 'No app name stored. Run "there is an existing E2E app available for testing" first.', + 'No app stored. Run "there is an existing E2E app available for testing" first.', ) } await expect(this.getPage().getByRole('link', { name: appName, exact: true })).not.toBeVisible({ timeout: 10_000, }) + this.createdAppIds = this.createdAppIds.filter((createdAppId) => createdAppId !== appId) }) diff --git a/e2e/features/support/hooks.ts b/e2e/features/support/hooks.ts index b35668fca86..ebafbb9892a 100644 --- a/e2e/features/support/hooks.ts +++ b/e2e/features/support/hooks.ts @@ -1,5 +1,6 @@ import type { Browser, Page } from '@playwright/test' import type { Buffer } from 'node:buffer' +import type { CleanupTask } from '../../support/cleanup' import type { DifyWorld } from './world' import { mkdir, writeFile } from 'node:fs/promises' import path from 'node:path' @@ -8,6 +9,7 @@ import { After, AfterAll, Before, BeforeAll, setDefaultTimeout, Status } from '@ import { chromium } from '@playwright/test' import { AUTH_BOOTSTRAP_TIMEOUT_MS, ensureAuthenticatedState } from '../../fixtures/auth' import { deleteTestApp } from '../../support/api' +import { runCleanupTasks } from '../../support/cleanup' import { deleteTestDataset } from '../../support/datasets' import { getVoiceInputTestMaterialPath } from '../../support/test-materials' import { deleteBuiltinToolCredential } from '../../support/tools' @@ -21,6 +23,9 @@ import { const e2eRoot = fileURLToPath(new URL('../..', import.meta.url)) const artifactsDir = path.join(e2eRoot, 'cucumber-report', 'artifacts') +const closeSessionHookTimeoutMs = 30_000 +const cleanupHookTimeoutMs = 120_000 +const diagnosticHookTimeoutMs = 60_000 let browser: Browser | undefined let microphoneBrowserPromise: Promise | undefined @@ -83,14 +88,6 @@ const captureDiagnosticPage = async ( return [screenshotPath, htmlPath] } -const recordCleanup = async (errors: string[], label: string, cleanup: () => Promise) => { - try { - await cleanup() - } catch (error) { - errors.push(`${label}: ${error instanceof Error ? error.message : String(error)}`) - } -} - BeforeAll({ timeout: AUTH_BOOTSTRAP_TIMEOUT_MS }, async () => { await mkdir(artifactsDir, { recursive: true }) @@ -143,11 +140,78 @@ Before(async function (this: DifyWorld, { pickle }) { console.warn(`[e2e] start ${pickle.name}${tags ? ` ${tags}` : ''}`) }) -After(async function (this: DifyWorld, { pickle, result }) { - const elapsedMs = this.scenarioStartedAt ? Date.now() - this.scenarioStartedAt : undefined - const status = result?.status || Status.UNKNOWN +// Cucumber runs After hooks in reverse registration order: diagnostics, cleanup, then close. +After( + { name: 'Close browser session', timeout: closeSessionHookTimeoutMs }, + async function (this: DifyWorld, { result }) { + const closeErrors = await runCleanupTasks([ + { label: 'Close browser session', run: () => this.closeSession() }, + ]) + if (closeErrors.length === 0) return + + const message = `Cleanup errors:\n${closeErrors.join('\n')}` + this.attach(message, 'text/plain') + if (result?.status === Status.PASSED) throw new Error(message) + }, +) + +After( + { name: 'Clean up scenario resources', timeout: cleanupHookTimeoutMs }, + async function (this: DifyWorld, { result }) { + const cleanupTasks: CleanupTask[] = [ + ...this.createdAgentConfigSkills.toReversed().map((skill) => ({ + label: `Delete Agent config skill ${skill.name}`, + run: () => deleteAgentConfigSkill(skill.agentId, skill.name), + })), + ...this.createdAgentConfigFiles.toReversed().map((file) => ({ + label: `Delete Agent config file ${file.name}`, + run: () => deleteAgentConfigFile(file.agentId, file.name), + })), + ...this.createdAgentDriveFiles.toReversed().map((file) => ({ + label: `Delete Agent drive file ${file.key}`, + run: () => deleteAgentDriveFile(file.agentId, file.key), + })), + ...this.createdAppIds.toReversed().map((id) => ({ + label: `Delete app ${id}`, + run: () => deleteTestApp(id), + })), + ...this.createdAgentIds.toReversed().map((id) => ({ + label: `Delete Agent ${id}`, + run: () => deleteTestAgent(id), + })), + ...this.createdDatasetIds.toReversed().map((id) => ({ + label: `Delete dataset ${id}`, + run: () => deleteTestDataset(id), + })), + ...this.createdBuiltinToolCredentials.toReversed().map((credential) => ({ + label: `Delete builtin tool credential ${credential.provider}/${credential.credentialId}`, + run: () => deleteBuiltinToolCredential(credential.provider, credential.credentialId), + })), + ] + + const cleanupErrors = await runCleanupTasks(cleanupTasks) + cleanupErrors.push(...(await this.runRegisteredCleanups())) + + if (cleanupErrors.length === 0) return + + const message = `Cleanup errors:\n${cleanupErrors.join('\n')}` + this.attach(message, 'text/plain') + if (result?.status === Status.PASSED) throw new Error(message) + }, +) + +After( + { name: 'Capture scenario diagnostics', timeout: diagnosticHookTimeoutMs }, + async function (this: DifyWorld, { pickle, result }) { + const elapsedMs = this.scenarioStartedAt ? Date.now() - this.scenarioStartedAt : undefined + const status = result?.status || Status.UNKNOWN + + console.warn( + `[e2e] end ${pickle.name} status=${status}${elapsedMs ? ` durationMs=${elapsedMs}` : ''}`, + ) + + if (!diagnosticArtifactStatuses.has(status)) return - if (diagnosticArtifactStatuses.has(status)) { const artifactPaths: string[] = [] const artifactErrors: string[] = [] const diagnosticPages = uniqueDiagnosticPages([ @@ -181,48 +245,8 @@ After(async function (this: DifyWorld, { pickle, result }) { if (artifactPaths.length > 0) this.attach(`Artifacts:\n${artifactPaths.join('\n')}`, 'text/plain') - } - - console.warn( - `[e2e] end ${pickle.name} status=${status}${elapsedMs ? ` durationMs=${elapsedMs}` : ''}`, - ) - - const cleanupErrors: string[] = [] - - for (const skill of this.createdAgentConfigSkills.toReversed()) { - await recordCleanup(cleanupErrors, `Delete Agent config skill ${skill.name}`, () => - deleteAgentConfigSkill(skill.agentId, skill.name), - ) - } - for (const file of this.createdAgentConfigFiles.toReversed()) { - await recordCleanup(cleanupErrors, `Delete Agent config file ${file.name}`, () => - deleteAgentConfigFile(file.agentId, file.name), - ) - } - for (const file of this.createdAgentDriveFiles.toReversed()) { - await recordCleanup(cleanupErrors, `Delete Agent drive file ${file.key}`, () => - deleteAgentDriveFile(file.agentId, file.key), - ) - } - for (const id of this.createdAppIds) - await recordCleanup(cleanupErrors, `Delete app ${id}`, () => deleteTestApp(id)) - for (const id of this.createdAgentIds) - await recordCleanup(cleanupErrors, `Delete Agent ${id}`, () => deleteTestAgent(id)) - for (const id of this.createdDatasetIds) - await recordCleanup(cleanupErrors, `Delete dataset ${id}`, () => deleteTestDataset(id)) - for (const credential of this.createdBuiltinToolCredentials.toReversed()) { - await recordCleanup( - cleanupErrors, - `Delete builtin tool credential ${credential.provider}/${credential.credentialId}`, - () => deleteBuiltinToolCredential(credential.provider, credential.credentialId), - ) - } - if (cleanupErrors.length > 0) - this.attach(`Typed cleanup errors:\n${cleanupErrors.join('\n')}`, 'text/plain') - await this.runRegisteredCleanups() - - await this.closeSession() -}) + }, +) AfterAll(async () => { const microphoneBrowser = await microphoneBrowserPromise?.catch(() => undefined) diff --git a/e2e/features/support/world.ts b/e2e/features/support/world.ts index 21e4fa8e18e..bbdf01b35d9 100644 --- a/e2e/features/support/world.ts +++ b/e2e/features/support/world.ts @@ -1,8 +1,9 @@ import type { IWorldOptions } from '@cucumber/cucumber' -import type { Browser, BrowserContext, ConsoleMessage, Download, Page } from '@playwright/test' +import type { Browser, BrowserContext, Download, Page } from '@playwright/test' import type { AuthSessionMetadata } from '../../fixtures/auth' import { setWorldConstructor, World } from '@cucumber/cucumber' import { authStatePath, readAuthSessionMetadata } from '../../fixtures/auth' +import { runCleanupTasks } from '../../support/cleanup' import { baseURL, defaultLocale } from '../../test-env' export type ScenarioCleanup = () => Promise | void @@ -128,18 +129,16 @@ export class DifyWorld extends World { ...(authenticated ? { storageState: authStatePath } : {}), }) this.context.setDefaultTimeout(30_000) - this.page = await this.context.newPage() - this.page.setDefaultTimeout(30_000) - - this.page.on('console', (message: ConsoleMessage) => { + this.context.on('console', (message) => { if (message.type() === 'error') this.consoleErrors.push(message.text()) }) - this.page.on('pageerror', (error) => { - this.pageErrors.push(error.message) + this.context.on('weberror', (webError) => { + this.pageErrors.push(webError.error().message) }) - this.page.on('download', (dl) => { + this.context.on('download', (dl) => { this.capturedDownloads.push(dl) }) + this.page = await this.context.newPage() } async startAuthenticatedSession(browser: Browser) { @@ -165,27 +164,25 @@ export class DifyWorld extends World { this.scenarioCleanups.push(cleanup) } - async runRegisteredCleanups() { - const errors: string[] = [] - - for (const cleanup of this.scenarioCleanups.toReversed()) { - try { - await cleanup() - } catch (error) { - errors.push(error instanceof Error ? error.message : String(error)) - } - } - - if (errors.length > 0) this.attach(`Cleanup errors:\n${errors.join('\n')}`, 'text/plain') + runRegisteredCleanups() { + return runCleanupTasks( + this.scenarioCleanups.toReversed().map((run, index) => ({ + label: `Registered cleanup ${index + 1}`, + run, + })), + ) } async closeSession() { - await this.context?.close() - this.context = undefined - this.page = undefined - this.session = undefined - this.scenarioStartedAt = undefined - this.resetScenarioState() + try { + await this.context?.close() + } finally { + this.context = undefined + this.page = undefined + this.session = undefined + this.scenarioStartedAt = undefined + this.resetScenarioState() + } } } diff --git a/e2e/scripts/setup.ts b/e2e/scripts/setup.ts index 075994e16cd..5302a75db7d 100644 --- a/e2e/scripts/setup.ts +++ b/e2e/scripts/setup.ts @@ -55,6 +55,7 @@ const e2eStatePaths = [ path.join(e2eDir, '.auth'), path.join(e2eDir, 'cucumber-report'), path.join(e2eDir, '.logs'), + path.join(e2eDir, '.logs-non-external'), path.join(e2eDir, 'playwright-report'), path.join(e2eDir, 'test-results'), ] diff --git a/e2e/support/api.ts b/e2e/support/api.ts index 444fa2ff35a..a6dfde8ef17 100644 --- a/e2e/support/api.ts +++ b/e2e/support/api.ts @@ -154,7 +154,8 @@ export async function syncAgentV2WorkflowDraft(appId: string, agentId: string): export async function deleteTestApp(id: string): Promise { const ctx = await createApiContext() try { - await ctx.delete(`/console/api/apps/${id}`) + const response = await ctx.delete(`/console/api/apps/${id}`) + await expectApiResponseOK(response, `Delete app ${id}`) } finally { await ctx.dispose() } diff --git a/e2e/support/cleanup.ts b/e2e/support/cleanup.ts new file mode 100644 index 00000000000..147c59a1f79 --- /dev/null +++ b/e2e/support/cleanup.ts @@ -0,0 +1,18 @@ +export type CleanupTask = { + label: string + run: () => Promise | void +} + +export async function runCleanupTasks(tasks: CleanupTask[]): Promise { + const errors: string[] = [] + + for (const task of tasks) { + try { + await task.run() + } catch (error) { + errors.push(`${task.label}: ${error instanceof Error ? error.message : String(error)}`) + } + } + + return errors +} diff --git a/e2e/tests/cleanup.test.ts b/e2e/tests/cleanup.test.ts new file mode 100644 index 00000000000..30eb0a37b73 --- /dev/null +++ b/e2e/tests/cleanup.test.ts @@ -0,0 +1,49 @@ +import { describe, expect, it, vi } from 'vitest' +import { runCleanupTasks } from '../support/cleanup' + +describe('runCleanupTasks', () => { + it('runs every task in order', async () => { + const order: string[] = [] + + const errors = await runCleanupTasks([ + { + label: 'first', + run: () => { + order.push('first') + }, + }, + { + label: 'second', + run: async () => { + order.push('second') + }, + }, + ]) + + expect(order).toEqual(['first', 'second']) + expect(errors).toEqual([]) + }) + + it('continues after failures and labels every error', async () => { + const finalTask = vi.fn() + + const errors = await runCleanupTasks([ + { + label: 'sync cleanup', + run: () => { + throw new Error('sync failure') + }, + }, + { + label: 'async cleanup', + run: async () => { + throw new Error('async failure') + }, + }, + { label: 'final cleanup', run: finalTask }, + ]) + + expect(finalTask).toHaveBeenCalledOnce() + expect(errors).toEqual(['sync cleanup: sync failure', 'async cleanup: async failure']) + }) +})