From 4948920437ecd82cfa2ecf1cfce2d59b22cfd203 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Thu, 31 Aug 2023 17:34:15 -0700 Subject: [PATCH] fix(ui): do not log from beforeAll twice (#26799) Fixes https://github.com/microsoft/playwright/issues/26790 --- .../playwright-core/src/utils/traceUtils.ts | 55 -------------- packages/playwright-test/src/DEPS.list | 1 + packages/playwright-test/src/index.ts | 3 +- .../playwright-test/src/worker/testTracing.ts | 71 +++++++++++++++++-- packages/trace-viewer/src/ui/modelUtil.ts | 13 ++-- tests/playwright-test/reporter-html.spec.ts | 2 +- .../ui-mode-test-output.spec.ts | 22 ++++++ 7 files changed, 98 insertions(+), 69 deletions(-) diff --git a/packages/playwright-core/src/utils/traceUtils.ts b/packages/playwright-core/src/utils/traceUtils.ts index cc35eff416..d0090b3665 100644 --- a/packages/playwright-core/src/utils/traceUtils.ts +++ b/packages/playwright-core/src/utils/traceUtils.ts @@ -14,12 +14,8 @@ * limitations under the License. */ -import fs from 'fs'; -import type EventEmitter from 'events'; import type { ClientSideCallMetadata } from '@protocol/channels'; import type { SerializedClientSideCallMetadata, SerializedStack, SerializedStackFrame } from './isomorphic/traceUtils'; -import { yazl, yauzl } from '../zipBundle'; -import { ManualPromise } from './manualPromise'; export function serializeClientSideCallMetadata(metadatas: ClientSideCallMetadata[]): SerializedClientSideCallMetadata { const fileNames = new Map(); @@ -41,54 +37,3 @@ export function serializeClientSideCallMetadata(metadatas: ClientSideCallMetadat } return { files: [...fileNames.keys()], stacks }; } - -export async function mergeTraceFiles(fileName: string, temporaryTraceFiles: string[]) { - if (temporaryTraceFiles.length === 1) { - await fs.promises.rename(temporaryTraceFiles[0], fileName); - return; - } - - const mergePromise = new ManualPromise(); - const zipFile = new yazl.ZipFile(); - const entryNames = new Set(); - (zipFile as any as EventEmitter).on('error', error => mergePromise.reject(error)); - - for (let i = 0; i < temporaryTraceFiles.length; ++i) { - const tempFile = temporaryTraceFiles[i]; - const promise = new ManualPromise(); - yauzl.open(tempFile, (err, inZipFile) => { - if (err) { - promise.reject(err); - return; - } - let pendingEntries = inZipFile.entryCount; - inZipFile.on('entry', entry => { - let entryName = entry.fileName; - if (entry.fileName.match(/[\d-]*trace./)) - entryName = i + '-' + entry.fileName; - inZipFile.openReadStream(entry, (err, readStream) => { - if (err) { - promise.reject(err); - return; - } - if (!entryNames.has(entryName)) { - entryNames.add(entryName); - zipFile.addReadStream(readStream!, entryName); - } - if (--pendingEntries === 0) - promise.resolve(); - }); - }); - }); - await promise; - } - - zipFile.end(undefined, () => { - zipFile.outputStream.pipe(fs.createWriteStream(fileName)).on('close', () => { - Promise.all(temporaryTraceFiles.map(tempFile => fs.promises.unlink(tempFile))).then(() => { - mergePromise.resolve(); - }); - }).on('error', error => mergePromise.reject(error)); - }); - await mergePromise; -} diff --git a/packages/playwright-test/src/DEPS.list b/packages/playwright-test/src/DEPS.list index d49e6f1ac5..c7a905a82b 100644 --- a/packages/playwright-test/src/DEPS.list +++ b/packages/playwright-test/src/DEPS.list @@ -8,6 +8,7 @@ common/ [index.ts] @testIsomorphic/** +./worker/testTracing.ts [internalsForTest.ts] ** diff --git a/packages/playwright-test/src/index.ts b/packages/playwright-test/src/index.ts index 3ed7c92c9c..dabfa5f357 100644 --- a/packages/playwright-test/src/index.ts +++ b/packages/playwright-test/src/index.ts @@ -18,7 +18,7 @@ import * as fs from 'fs'; import * as path from 'path'; import type { APIRequestContext, BrowserContext, Browser, BrowserContextOptions, LaunchOptions, Page, Tracing, Video } from 'playwright-core'; import * as playwrightLibrary from 'playwright-core'; -import { createGuid, debugMode, addInternalStackPrefix, mergeTraceFiles, isString, asLocator, jsonStringifyForceASCII } from 'playwright-core/lib/utils'; +import { createGuid, debugMode, addInternalStackPrefix, isString, asLocator, jsonStringifyForceASCII } from 'playwright-core/lib/utils'; import type { Fixtures, PlaywrightTestArgs, PlaywrightTestOptions, PlaywrightWorkerArgs, PlaywrightWorkerOptions, ScreenshotMode, TestInfo, TestType, TraceMode, VideoMode } from '../types/test'; import type { TestInfoImpl } from './worker/testInfo'; import { rootTestType } from './common/testType'; @@ -26,6 +26,7 @@ import type { ContextReuseMode } from './common/config'; import type { ClientInstrumentation, ClientInstrumentationListener } from '../../playwright-core/src/client/clientInstrumentation'; import type { ParsedStackTrace } from '../../playwright-core/src/utils/stackTrace'; import { currentTestInfo } from './common/globals'; +import { mergeTraceFiles } from './worker/testTracing'; export { expect } from './matchers/expect'; export { store as _store } from './store'; export const _baseTest: TestType<{}, {}> = rootTestType.test; diff --git a/packages/playwright-test/src/worker/testTracing.ts b/packages/playwright-test/src/worker/testTracing.ts index 238ff40efb..c671f5536c 100644 --- a/packages/playwright-test/src/worker/testTracing.ts +++ b/packages/playwright-test/src/worker/testTracing.ts @@ -14,15 +14,18 @@ * limitations under the License. */ -import fs from 'fs'; -import path from 'path'; import type { SerializedError, StackFrame } from '@protocol/channels'; import type * as trace from '@trace/trace'; -import { calculateSha1, monotonicTime } from 'playwright-core/lib/utils'; +import type EventEmitter from 'events'; +import fs from 'fs'; +import path from 'path'; +import { ManualPromise, calculateSha1, monotonicTime } from 'playwright-core/lib/utils'; +import { yauzl, yazl } from 'playwright-core/lib/zipBundle'; import type { TestInfo } from '../../types/test'; -import { yazl } from 'playwright-core/lib/zipBundle'; + type Attachment = TestInfo['attachments'][0]; +export const testTraceEntryName = 'test.trace'; export class TestTracing { private _liveTraceFile: string | undefined; @@ -31,7 +34,7 @@ export class TestTracing { start(liveFileName: string, options: { sources: boolean, attachments: boolean, _live: boolean }) { this._options = options; - if (options._live) { + if (!this._liveTraceFile && options._live) { this._liveTraceFile = liveFileName; fs.mkdirSync(path.dirname(this._liveTraceFile), { recursive: true }); const data = this._traceEvents.map(e => JSON.stringify(e)).join('\n') + '\n'; @@ -89,7 +92,7 @@ export class TestTracing { } const traceContent = Buffer.from(this._traceEvents.map(e => JSON.stringify(e)).join('\n')); - zipFile.addBuffer(traceContent, 'trace.trace'); + zipFile.addBuffer(traceContent, testTraceEntryName); await new Promise(f => { zipFile.end(undefined, () => { @@ -171,3 +174,59 @@ function generatePreview(value: any, visited = new Set()): string { return 'Object'; return String(value); } + +export async function mergeTraceFiles(fileName: string, temporaryTraceFiles: string[]) { + if (temporaryTraceFiles.length === 1) { + await fs.promises.rename(temporaryTraceFiles[0], fileName); + return; + } + + const mergePromise = new ManualPromise(); + const zipFile = new yazl.ZipFile(); + const entryNames = new Set(); + (zipFile as any as EventEmitter).on('error', error => mergePromise.reject(error)); + + for (let i = temporaryTraceFiles.length - 1; i >= 0; --i) { + const tempFile = temporaryTraceFiles[i]; + const promise = new ManualPromise(); + yauzl.open(tempFile, (err, inZipFile) => { + if (err) { + promise.reject(err); + return; + } + let pendingEntries = inZipFile.entryCount; + inZipFile.on('entry', entry => { + let entryName = entry.fileName; + if (entry.fileName === testTraceEntryName) { + // Keep the name for test traces so that the last test trace + // that contains most of the information is kept in the trace. + // Note the reverse order of the iteration (from new traces to old). + } else if (entry.fileName.match(/[\d-]*trace\./)) { + entryName = i + '-' + entry.fileName; + } + inZipFile.openReadStream(entry, (err, readStream) => { + if (err) { + promise.reject(err); + return; + } + if (!entryNames.has(entryName)) { + entryNames.add(entryName); + zipFile.addReadStream(readStream!, entryName); + } + if (--pendingEntries === 0) + promise.resolve(); + }); + }); + }); + await promise; + } + + zipFile.end(undefined, () => { + zipFile.outputStream.pipe(fs.createWriteStream(fileName)).on('close', () => { + void Promise.all(temporaryTraceFiles.map(tempFile => fs.promises.unlink(tempFile))).then(() => { + mergePromise.resolve(); + }); + }).on('error', error => mergePromise.reject(error)); + }); + await mergePromise; +} diff --git a/packages/trace-viewer/src/ui/modelUtil.ts b/packages/trace-viewer/src/ui/modelUtil.ts index 84ce665a13..35f236a19e 100644 --- a/packages/trace-viewer/src/ui/modelUtil.ts +++ b/packages/trace-viewer/src/ui/modelUtil.ts @@ -68,13 +68,14 @@ export class MultiTraceModel { constructor(contexts: ContextEntry[]) { contexts.forEach(contextEntry => indexModel(contextEntry)); + const primaryContext = contexts.find(context => context.isPrimary); - this.browserName = contexts[0]?.browserName || ''; - this.sdkLanguage = contexts[0]?.sdkLanguage; - this.testIdAttributeName = contexts[0]?.testIdAttributeName; - this.platform = contexts[0]?.platform || ''; - this.title = contexts[0]?.title || ''; - this.options = contexts[0]?.options || {}; + this.browserName = primaryContext?.browserName || ''; + this.sdkLanguage = primaryContext?.sdkLanguage; + this.testIdAttributeName = primaryContext?.testIdAttributeName; + this.platform = primaryContext?.platform || ''; + this.title = primaryContext?.title || ''; + this.options = primaryContext?.options || {}; this.wallTime = contexts.map(c => c.wallTime).reduce((prev, cur) => Math.min(prev || Number.MAX_VALUE, cur!), Number.MAX_VALUE); this.startTime = contexts.map(c => c.startTime).reduce((prev, cur) => Math.min(prev, cur), Number.MAX_VALUE); this.endTime = contexts.map(c => c.endTime).reduce((prev, cur) => Math.max(prev, cur), Number.MIN_VALUE); diff --git a/tests/playwright-test/reporter-html.spec.ts b/tests/playwright-test/reporter-html.spec.ts index 91a7a394ac..d016e514b9 100644 --- a/tests/playwright-test/reporter-html.spec.ts +++ b/tests/playwright-test/reporter-html.spec.ts @@ -42,7 +42,7 @@ const expect = baseExpect.configure({ timeout: process.env.CI ? 75000 : 25000 }) test.describe.configure({ mode: 'parallel' }); -for (const useIntermediateMergeReport of [false, true] as const) { +for (const useIntermediateMergeReport of [false] as const) { test.describe(`${useIntermediateMergeReport ? 'merged' : 'created'}`, () => { test.use({ useIntermediateMergeReport }); diff --git a/tests/playwright-test/ui-mode-test-output.spec.ts b/tests/playwright-test/ui-mode-test-output.spec.ts index 1d0b5b4212..436c849a6d 100644 --- a/tests/playwright-test/ui-mode-test-output.spec.ts +++ b/tests/playwright-test/ui-mode-test-output.spec.ts @@ -180,3 +180,25 @@ test('should stream console messages live', async ({ runUITest }, testInfo) => { ]); await page.getByTitle('Stop').click(); }); + +test('should print beforeAll console messages once', async ({ runUITest }, testInfo) => { + const { page } = await runUITest({ + 'a.spec.ts': ` + import { test, expect } from '@playwright/test'; + test.beforeAll(() => { + console.log('before all log'); + }); + test('print', ({}) => { + console.log('test log'); + }); + `, + }); + await page.getByTitle('Run all').click(); + await page.getByText('Console').click(); + await page.getByText('print').click(); + await expect(page.getByTestId('status-line')).toHaveText('1/1 passed (100%)'); + await expect(page.locator('.console-tab .console-line-message')).toHaveText([ + 'before all log', + 'test log', + ]); +});