From 31a63b5c2a750d8fb3d7d2184ea098f0604b1cc7 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Thu, 5 Jan 2023 14:50:47 -0800 Subject: [PATCH] fix(reuse): make reuse work with tracing (#19733) Fixes #19059. --- .../playwright-core/src/client/browser.ts | 4 +- packages/playwright-test/src/index.ts | 54 ++++++++++--------- .../playwright-test/src/timeoutManager.ts | 4 ++ .../playwright-test/playwright.reuse.spec.ts | 33 ++++++++++-- 4 files changed, 64 insertions(+), 31 deletions(-) diff --git a/packages/playwright-core/src/client/browser.ts b/packages/playwright-core/src/client/browser.ts index 8ded087173..d63303d604 100644 --- a/packages/playwright-core/src/client/browser.ts +++ b/packages/playwright-core/src/client/browser.ts @@ -64,7 +64,9 @@ export class Browser extends ChannelOwner implements ap async _newContextForReuse(options: BrowserContextOptions = {}): Promise { for (const context of this._contexts) { - await this._browserType._onWillCloseContext?.(context); + await this._wrapApiCall(async () => { + await this._browserType._onWillCloseContext?.(context); + }, true); for (const page of context.pages()) page._onClose(); context._onClose(); diff --git a/packages/playwright-test/src/index.ts b/packages/playwright-test/src/index.ts index 0da9779d03..abaadcbc2f 100644 --- a/packages/playwright-test/src/index.ts +++ b/packages/playwright-test/src/index.ts @@ -251,10 +251,11 @@ const playwrightFixtures: Fixtures = ({ const traceMode = normalizeTraceMode(trace); const defaultTraceOptions = { screenshots: true, snapshots: true, sources: true }; const traceOptions = typeof trace === 'string' ? defaultTraceOptions : { ...defaultTraceOptions, ...trace, mode: undefined }; - const captureTrace = shouldCaptureTrace(traceMode, testInfo) && !_reuseContext; + const captureTrace = shouldCaptureTrace(traceMode, testInfo); const temporaryTraceFiles: string[] = []; const temporaryScreenshots: string[] = []; const testInfoImpl = testInfo as TestInfoImpl; + const reusedContexts = new Set(); const createInstrumentationListener = (context?: BrowserContext) => { return { @@ -282,7 +283,7 @@ const playwrightFixtures: Fixtures = ({ }; }; - const startTracing = async (tracing: Tracing) => { + const startTraceChunkOnContextCreation = async (tracing: Tracing) => { if (captureTrace) { const title = [path.relative(testInfo.project.testDir, testInfo.file) + ':' + testInfo.line, ...testInfo.titlePath.slice(1)].join(' › '); if (!(tracing as any)[kTracingStarted]) { @@ -302,20 +303,19 @@ const playwrightFixtures: Fixtures = ({ const onDidCreateBrowserContext = async (context: BrowserContext) => { context.setDefaultTimeout(actionTimeout || 0); context.setDefaultNavigationTimeout(navigationTimeout || actionTimeout || 0); - await startTracing(context.tracing); + await startTraceChunkOnContextCreation(context.tracing); const listener = createInstrumentationListener(context); (context as any)._instrumentation.addListener(listener); (context.request as any)._instrumentation.addListener(listener); }; const onDidCreateRequestContext = async (context: APIRequestContext) => { const tracing = (context as any)._tracing as Tracing; - await startTracing(tracing); + await startTraceChunkOnContextCreation(tracing); (context as any)._instrumentation.addListener(createInstrumentationListener()); }; const startedCollectingArtifacts = Symbol('startedCollectingArtifacts'); - - const stopTracing = async (tracing: Tracing) => { + const stopTraceChunkOnContextClosure = async (tracing: Tracing) => { (tracing as any)[startedCollectingArtifacts] = true; if (captureTrace) { // Export trace for now. We'll know whether we have to preserve it @@ -346,7 +346,11 @@ const playwrightFixtures: Fixtures = ({ }; const onWillCloseContext = async (context: BrowserContext) => { - await stopTracing(context.tracing); + // When reusing context, we get all previous contexts closed at the start of next test. + // Do not record empty traces and useless screenshots for them. + if (reusedContexts.has(context)) + return; + await stopTraceChunkOnContextClosure(context.tracing); if (screenshotMode === 'on' || screenshotMode === 'only-on-failure') { // Capture screenshot for now. We'll know whether we have to preserve them // after the test finishes. @@ -356,7 +360,7 @@ const playwrightFixtures: Fixtures = ({ const onWillCloseRequestContext = async (context: APIRequestContext) => { const tracing = (context as any)._tracing as Tracing; - await stopTracing(tracing); + await stopTraceChunkOnContextClosure(tracing); }; // 1. Setup instrumentation and process existing contexts. @@ -365,7 +369,10 @@ const playwrightFixtures: Fixtures = ({ (browserType as any)._onWillCloseContext = onWillCloseContext; (browserType as any)._defaultContextOptions = _combinedContextOptions; const existingContexts = Array.from((browserType as any)._contexts) as BrowserContext[]; - await Promise.all(existingContexts.map(onDidCreateBrowserContext)); + if (_reuseContext) + existingContexts.forEach(c => reusedContexts.add(c)); + else + await Promise.all(existingContexts.map(onDidCreateBrowserContext)); } { (playwright.request as any)._onDidCreateContext = onDidCreateRequestContext; @@ -416,23 +423,21 @@ const playwrightFixtures: Fixtures = ({ (playwright.request as any)._onWillCloseContext = undefined; testInfoImpl._onTestFailureImmediateCallbacks.delete(screenshotOnTestFailure); - const stopTraceChunk = async (tracing: Tracing): Promise => { + const stopTraceChunkOnTestFinish = async (tracing: Tracing) => { // When we timeout during context.close(), we might end up with context still alive // but artifacts being already collected. In this case, do not collect artifacts // for the second time. if ((tracing as any)[startedCollectingArtifacts]) - return false; + return; if (preserveTrace) await tracing.stopChunk({ path: addTraceAttachment() }); else if (captureTrace) await tracing.stopChunk(); - return true; }; // 5. Collect artifacts from any non-closed contexts. await Promise.all(leftoverContexts.map(async context => { - if (!await stopTraceChunk(context.tracing)) - return; + await stopTraceChunkOnTestFinish(context.tracing); if (captureScreenshots) { await Promise.all(context.pages().map(async page => { if ((page as any)[screenshottedSymbol]) @@ -444,7 +449,7 @@ const playwrightFixtures: Fixtures = ({ } }).concat(leftoverApiRequests.map(async context => { const tracing = (context as any)._tracing as Tracing; - await stopTraceChunk(tracing); + await stopTraceChunkOnTestFinish(tracing); }))); // 6. Either remove or attach temporary traces and screenshots for contexts closed @@ -464,12 +469,13 @@ const playwrightFixtures: Fixtures = ({ }, { auto: 'all-hooks-included', _title: 'playwright configuration' } as any], _contextFactory: [async ({ browser, video, _artifactsDir, _reuseContext }, use, testInfo) => { + const testInfoImpl = testInfo as TestInfoImpl; const videoMode = normalizeVideoMode(video); const captureVideo = shouldCaptureVideo(videoMode, testInfo) && !_reuseContext; const contexts = new Map(); await use(async options => { - const hook = hookType(testInfo); + const hook = hookType(testInfoImpl); if (hook) { throw new Error([ `"context" and "page" fixtures are not supported in "${hook}" since they are created on a per-test basis.`, @@ -490,7 +496,7 @@ const playwrightFixtures: Fixtures = ({ return context; }); - const prependToError = (testInfo as any)._didTimeout ? + const prependToError = testInfoImpl._didTimeout ? formatPendingCalls((browser as any)._connection.pendingProtocolCalls()) : ''; let counter = 0; @@ -521,9 +527,8 @@ const playwrightFixtures: Fixtures = ({ _contextReuseMode: process.env.PW_TEST_REUSE_CONTEXT === 'when-possible' ? 'when-possible' : (process.env.PW_TEST_REUSE_CONTEXT ? 'force' : 'none'), - _reuseContext: async ({ video, trace, _contextReuseMode }, use, testInfo) => { - const reuse = _contextReuseMode === 'force' || - (_contextReuseMode === 'when-possible' && !shouldCaptureVideo(normalizeVideoMode(video), testInfo) && !shouldCaptureTrace(normalizeTraceMode(trace), testInfo)); + _reuseContext: async ({ video, _contextReuseMode }, use, testInfo) => { + const reuse = _contextReuseMode === 'force' || (_contextReuseMode === 'when-possible' && !shouldCaptureVideo(normalizeVideoMode(video), testInfo)); await use(reuse); }, @@ -575,11 +580,10 @@ function formatStackFrame(frame: StackFrame) { return `${file}:${frame.line || 1}:${frame.column || 1}`; } -function hookType(testInfo: TestInfo): 'beforeAll' | 'afterAll' | undefined { - if ((testInfo as any)._timeoutManager._runnable?.type === 'beforeAll') - return 'beforeAll'; - if ((testInfo as any)._timeoutManager._runnable?.type === 'afterAll') - return 'afterAll'; +function hookType(testInfo: TestInfoImpl): 'beforeAll' | 'afterAll' | undefined { + const type = testInfo._timeoutManager.currentRunnableType(); + if (type === 'beforeAll' || type === 'afterAll') + return type; } type StackFrame = { diff --git a/packages/playwright-test/src/timeoutManager.ts b/packages/playwright-test/src/timeoutManager.ts index 94f0ee7fb4..4ca41b61e1 100644 --- a/packages/playwright-test/src/timeoutManager.ts +++ b/packages/playwright-test/src/timeoutManager.ts @@ -89,6 +89,10 @@ export class TimeoutManager { this._timeoutRunner.updateTimeout(timeout); } + currentRunnableType() { + return this._runnable.type; + } + private _currentSlot() { return this._fixture?.slot || this._runnable.slot || this._defaultSlot; } diff --git a/tests/playwright-test/playwright.reuse.spec.ts b/tests/playwright-test/playwright.reuse.spec.ts index 3b061f4544..ee30383c1d 100644 --- a/tests/playwright-test/playwright.reuse.spec.ts +++ b/tests/playwright-test/playwright.reuse.spec.ts @@ -114,29 +114,52 @@ test('should reuse context and disable video if mode=force', async ({ runInlineT expect(fs.existsSync(testInfo.outputPath('test-results', 'reuse-two', 'video.webm'))).toBeFalsy(); }); -test('should not reuse context with trace if mode=when-possible', async ({ runInlineTest }) => { +test('should reuse context with trace if mode=when-possible', async ({ runInlineTest }, testInfo) => { const result = await runInlineTest({ 'playwright.config.ts': ` export default { use: { trace: 'on' }, }; `, - 'src/reuse.test.ts': ` + 'reuse.spec.ts': ` const { test } = pwt; let lastContextGuid; - test('one', async ({ context }) => { + test('one', async ({ context, page }) => { lastContextGuid = context._guid; + await page.setContent(''); + await page.click('button'); }); - test('two', async ({ context }) => { - expect(context._guid).not.toBe(lastContextGuid); + test('two', async ({ context, page }) => { + expect(context._guid).toBe(lastContextGuid); + await page.setContent(''); + await page.fill('input', 'value'); + await page.locator('input').click(); }); `, }, { workers: 1 }, { PW_TEST_REUSE_CONTEXT: 'when-possible' }); expect(result.exitCode).toBe(0); expect(result.passed).toBe(2); + + const trace1 = await parseTrace(testInfo.outputPath('test-results', 'reuse-one', 'trace.zip')); + expect(trace1.actions).toEqual([ + 'browserContext.newPage', + 'page.setContent', + 'page.click', + ]); + expect(trace1.events.some(e => e.type === 'frame-snapshot')).toBe(true); + expect(fs.existsSync(testInfo.outputPath('test-results', 'reuse-one', 'trace-1.zip'))).toBe(false); + + const trace2 = await parseTrace(testInfo.outputPath('test-results', 'reuse-two', 'trace.zip')); + expect(trace2.actions).toEqual([ + 'page.setContent', + 'page.fill', + 'locator.click', + ]); + expect(trace2.events.some(e => e.type === 'frame-snapshot')).toBe(true); + expect(fs.existsSync(testInfo.outputPath('test-results', 'reuse-two', 'trace-1.zip'))).toBe(false); }); test('should work with manually closed pages', async ({ runInlineTest }) => {