From de69b766d9f8d8bdbc20c1a1b4ffcfc9869d0625 Mon Sep 17 00:00:00 2001 From: Dmitry Gozman Date: Fri, 10 Feb 2023 20:26:19 -0800 Subject: [PATCH] test: code health in ttest (#20837) - Remove duplicate tests. - Remove unused test helpers. - Print full watch output on failures. - Unflake some tests. --- tests/config/commonFixtures.ts | 5 +- tests/playwright-test/deps.spec.ts | 43 ++++ tests/playwright-test/global-scripts.spec.ts | 243 ------------------ .../playwright-test-fixtures.ts | 68 +---- tests/playwright-test/reporter-html.spec.ts | 43 ++-- tests/playwright-test/reporter-list.spec.ts | 2 +- 6 files changed, 83 insertions(+), 321 deletions(-) delete mode 100644 tests/playwright-test/global-scripts.spec.ts diff --git a/tests/config/commonFixtures.ts b/tests/config/commonFixtures.ts index 4a12d8c9ce..650afad612 100644 --- a/tests/config/commonFixtures.ts +++ b/tests/config/commonFixtures.ts @@ -32,6 +32,7 @@ export class TestChildProcess { params: TestChildParams; process: ChildProcess; output = ''; + fullOutput = ''; onOutput?: (chunk: string | Buffer) => void; exited: Promise<{ exitCode: number, signal: string | null }>; exitCode: Promise; @@ -60,6 +61,8 @@ export class TestChildProcess { this.output += String(chunk); if (process.env.PWTEST_DEBUG) process.stdout.write(String(chunk)); + else + this.fullOutput += String(chunk); this.onOutput?.(chunk); for (const cb of this._outputCallbacks) cb(); @@ -140,7 +143,7 @@ export const commonFixtures: Fixtures = { if (testInfo.status !== 'passed' && testInfo.status !== 'skipped' && !process.env.PWTEST_DEBUG) { for (const process of processes) { console.log('====== ' + process.params.command.join(' ')); - console.log(process.output); + console.log(stripAnsi(process.fullOutput)); console.log('========================================='); } } diff --git a/tests/playwright-test/deps.spec.ts b/tests/playwright-test/deps.spec.ts index 308b53197d..daab12bd32 100644 --- a/tests/playwright-test/deps.spec.ts +++ b/tests/playwright-test/deps.spec.ts @@ -261,3 +261,46 @@ test('should report circular dependencies', async ({ runInlineTest }) => { expect(result.exitCode).toBe(1); expect(result.output).toContain('Circular dependency detected between projects.'); }); + +test('should run dependency in each shard', async ({ runInlineTest }) => { + const files = { + 'playwright.config.ts': ` + module.exports = { + projects: [ + { name: 'setup', testMatch: /setup.ts/ }, + { name: 'chromium', dependencies: ['setup'] }, + ], + }; + `, + 'setup.ts': ` + const { test } = pwt; + test('setup', async ({}) => { + console.log('\\n%%setup'); + }); + `, + 'test1.spec.ts': ` + const { test } = pwt; + test('test1', async ({}) => { + console.log('\\n%%test1'); + }); + `, + 'test2.spec.ts': ` + const { test } = pwt; + test('test2', async ({}) => { + console.log('\\n%%test2'); + }); + `, + }; + { + const result = await runInlineTest(files, { workers: 1, shard: '1/2' }); + expect(result.exitCode).toBe(0); + expect(result.passed).toBe(2); + expect(result.outputLines).toEqual(['setup', 'test1']); + } + { + const result = await runInlineTest(files, { workers: 1, shard: '2/2' }); + expect(result.exitCode).toBe(0); + expect(result.passed).toBe(2); + expect(result.outputLines).toEqual(['setup', 'test2']); + } +}); diff --git a/tests/playwright-test/global-scripts.spec.ts b/tests/playwright-test/global-scripts.spec.ts deleted file mode 100644 index 4051170c00..0000000000 --- a/tests/playwright-test/global-scripts.spec.ts +++ /dev/null @@ -1,243 +0,0 @@ -/** - * Copyright (c) Microsoft Corporation. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -import path from 'path'; -import { test, expect } from './playwright-test-fixtures'; - -type Timeline = { titlePath: string[], event: 'begin' | 'end' }[]; - -function formatTimeline(timeline: Timeline) { - return timeline.map(e => `${e.titlePath.slice(1).join(' > ')} [${e.event}]`).join('\n'); -} - -function formatFileNames(timeline: Timeline) { - return timeline.map(e => e.titlePath[2]).join('\n'); -} - -function fileNames(timeline: Timeline) { - const fileNames = Array.from(new Set(timeline.map(({ titlePath }) => { - const name = titlePath[2]; - const index = name.lastIndexOf(path.sep); - if (index === -1) - return name; - return name.slice(index + 1); - })).keys()); - fileNames.sort(); - return fileNames; -} - -function expectFilesRunBefore(timeline: Timeline, before: string[], after: string[]) { - const fileBegin = name => { - const index = timeline.findIndex(({ titlePath }) => titlePath[2] === name); - expect(index, `cannot find ${name} in\n${formatFileNames(timeline)}`).not.toBe(-1); - return index; - }; - const fileEnd = name => { - // There is no Array.findLastIndex in Node < 18. - let index = -1; - for (index = timeline.length - 1; index >= 0; index--) { - if (timeline[index].titlePath[2] === name) - break; - } - expect(index, `cannot find ${name} in\n${formatFileNames(timeline)}`).not.toBe(-1); - return index; - }; - - for (const b of before) { - const bEnd = fileEnd(b); - for (const a of after) { - const aBegin = fileBegin(a); - expect(bEnd < aBegin, `'${b}' expected to finish before ${a}, actual order:\n${formatTimeline(timeline)}`).toBeTruthy(); - } - } -} - -test('should work for one project', async ({ runGroups }, testInfo) => { - const files = { - 'playwright.config.ts': ` - module.exports = { - projects: [ - { - name: 'setup', - testMatch: /.*global.ts/, - }, - { - name: 'p1', - testMatch: /.*.test.ts/, - dependencies: ['setup'], - }, - ] - };`, - 'a.test.ts': ` - const { test } = pwt; - test('test1', async () => { }); - test('test2', async () => { }); - `, - 'global.ts': ` - const { test } = pwt; - test('setup1', async () => { }); - test('setup2', async () => { }); - `, - }; - const { exitCode, passed, timeline } = await runGroups(files); - expect(exitCode).toBe(0); - expect(passed).toBe(4); - expect(formatTimeline(timeline)).toEqual(`setup > global.ts > setup1 [begin] -setup > global.ts > setup1 [end] -setup > global.ts > setup2 [begin] -setup > global.ts > setup2 [end] -p1 > a.test.ts > test1 [begin] -p1 > a.test.ts > test1 [end] -p1 > a.test.ts > test2 [begin] -p1 > a.test.ts > test2 [end]`); -}); - -test('should work for several projects', async ({ runGroups }, testInfo) => { - const files = { - 'playwright.config.ts': ` - module.exports = { - projects: [ - { - name: 'setup', - testMatch: /.*global.ts/, - }, - { - name: 'p1', - testMatch: /.*a.test.ts/, - dependencies: ['setup'], - }, - { - name: 'p2', - testMatch: /.*b.test.ts/, - dependencies: ['setup'], - }, - ] - };`, - 'a.test.ts': ` - const { test } = pwt; - test('test1', async () => { }); - test('test2', async () => { }); - `, - 'b.test.ts': ` - const { test } = pwt; - test('test1', async () => { }); - test('test2', async () => { }); - `, - 'global.ts': ` - const { test } = pwt; - test('setup1', async () => { }); - test('setup2', async () => { }); - `, - }; - const { exitCode, passed, timeline } = await runGroups(files); - expect(exitCode).toBe(0); - expect(passed).toBe(6); - expectFilesRunBefore(timeline, [`global.ts`], [`a.test.ts`, `b.test.ts`]); -}); - -test('should skip tests if global setup fails', async ({ runGroups }, testInfo) => { - const files = { - 'playwright.config.ts': ` - module.exports = { - projects: [ - { - name: 'setup', - testMatch: /.*global.ts/, - }, - { - name: 'p1', - testMatch: /.*a.test.ts/, - dependencies: ['setup'], - }, - { - name: 'p2', - testMatch: /.*b.test.ts/, - dependencies: ['setup'], - }, - ] - };`, - 'a.test.ts': ` - const { test } = pwt; - test('test1', async () => { }); - test('test2', async () => { }); - `, - 'b.test.ts': ` - const { test } = pwt; - test('test1', async () => { }); - `, - 'global.ts': ` - const { test, expect } = pwt; - test('setup1', async () => { }); - test('setup2', async () => { expect(1).toBe(2) }); - `, - }; - const { exitCode, passed, skipped } = await runGroups(files); - expect(exitCode).toBe(1); - expect(passed).toBe(1); - expect(skipped).toBe(3); -}); - -test('should run setup in each project shard', async ({ runGroups }, testInfo) => { - const files = { - 'playwright.config.ts': ` - module.exports = { - projects: [ - { - name: 'setup', - testMatch: /.*global.ts/, - }, - { - name: 'p1', - dependencies: ['setup'], - }, - ] - };`, - 'a.test.ts': ` - const { test } = pwt; - test('test1', async () => { }); - test('test2', async () => { }); - test('test3', async () => { }); - test('test4', async () => { }); - `, - 'b.test.ts': ` - const { test } = pwt; - test('test1', async () => { }); - test('test2', async () => { }); - `, - 'global.ts': ` - const { test, expect } = pwt; - test('setup1', async () => { }); - test('setup2', async () => { }); - `, - }; - - { // Shard 1/2 - const { exitCode, passed, timeline, output } = await runGroups(files, { shard: '1/2' }); - expect(output).toContain('Running 6 tests using 1 worker, shard 1 of 2'); - expect(fileNames(timeline)).toEqual(['a.test.ts', 'global.ts']); - expectFilesRunBefore(timeline, [`global.ts`], [`a.test.ts`]); - expect(exitCode).toBe(0); - expect(passed).toBe(6); - } - { // Shard 2/2 - const { exitCode, passed, timeline, output } = await runGroups(files, { shard: '2/2' }); - expect(output).toContain('Running 4 tests using 1 worker, shard 2 of 2'); - expect(fileNames(timeline)).toEqual(['b.test.ts', 'global.ts']); - expectFilesRunBefore(timeline, [`global.ts`], [`b.test.ts`]); - expect(exitCode).toBe(0); - expect(passed).toBe(4); - } -}); - diff --git a/tests/playwright-test/playwright-test-fixtures.ts b/tests/playwright-test/playwright-test-fixtures.ts index 477c3a7d05..cab6454a40 100644 --- a/tests/playwright-test/playwright-test-fixtures.ts +++ b/tests/playwright-test/playwright-test-fixtures.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import type { JSONReport, JSONReportSuite, JSONReportTest, JSONReportTestResult } from '@playwright/test/reporter'; +import type { JSONReport, JSONReportSpec, JSONReportSuite, JSONReportTest, JSONReportTestResult } from '@playwright/test/reporter'; import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; @@ -267,12 +267,10 @@ type RunOptions = { }; type Fixtures = { writeFiles: (files: Files) => Promise; - runInlineTest: (files: Files, params?: Params, env?: NodeJS.ProcessEnv, options?: RunOptions, beforeRunPlaywrightTest?: ({ baseDir }: { baseDir: string }) => Promise) => Promise; + runInlineTest: (files: Files, params?: Params, env?: NodeJS.ProcessEnv, options?: RunOptions) => Promise; runWatchTest: (files: Files, env?: NodeJS.ProcessEnv, options?: RunOptions) => Promise; runTSC: (files: Files) => Promise; nodeVersion: { major: number, minor: number, patch: number }; - runGroups: (files: Files, params?: Params, env?: NodeJS.ProcessEnv, options?: RunOptions) => Promise<{ timeline: { titlePath: string[], event: 'begin' | 'end' }[] } & RunResult>; - runCommand: (files: Files, args: string[]) => Promise; }; export const test = base @@ -285,10 +283,8 @@ export const test = base runInlineTest: async ({ childProcess }, use, testInfo: TestInfo) => { const cacheDir = await fs.promises.mkdtemp(path.join(os.tmpdir(), 'playwright-test-cache-')); - await use(async (files: Files, params: Params = {}, env: NodeJS.ProcessEnv = {}, options: RunOptions = {}, beforeRunPlaywrightTest?: ({ baseDir }: { baseDir: string }) => Promise) => { + await use(async (files: Files, params: Params = {}, env: NodeJS.ProcessEnv = {}, options: RunOptions = {}) => { const baseDir = await writeFiles(testInfo, files, true); - if (beforeRunPlaywrightTest) - await beforeRunPlaywrightTest({ baseDir }); return await runPlaywrightTest(childProcess, baseDir, params, { ...env, PWTEST_CACHE_DIR: cacheDir }, options); }); await removeFolderAsync(cacheDir); @@ -306,13 +302,6 @@ export const test = base await removeFolderAsync(cacheDir); }, - runCommand: async ({ childProcess }, use, testInfo: TestInfo) => { - await use(async (files: Files, args: string[]) => { - const baseDir = await writeFiles(testInfo, files, true); - return await runPlaywrightCommand(childProcess, baseDir, args, { }); - }); - }, - runTSC: async ({ childProcess }, use, testInfo) => { await use(async files => { const baseDir = await writeFiles(testInfo, { 'tsconfig.json': JSON.stringify(TSCONFIG), ...files }, true); @@ -330,43 +319,6 @@ export const test = base const [major, minor, patch] = process.versions.node.split('.'); await use({ major: +major, minor: +minor, patch: +patch }); }, - - runGroups: async ({ runInlineTest }, use, testInfo) => { - const timelinePath = testInfo.outputPath('timeline.json'); - await use(async (files, params, env, options) => { - const result = await runInlineTest({ - ...files, - 'reporter.ts': ` - import { Reporter, TestCase } from '@playwright/test/reporter'; - import fs from 'fs'; - import path from 'path'; - class TimelineReporter implements Reporter { - private _timeline: {titlePath: string, event: 'begin' | 'end'}[] = []; - onTestBegin(test: TestCase) { - this._timeline.push({ titlePath: test.titlePath(), event: 'begin' }); - } - onTestEnd(test: TestCase) { - this._timeline.push({ titlePath: test.titlePath(), event: 'end' }); - } - onEnd() { - fs.writeFileSync(path.join(${JSON.stringify(timelinePath)}), JSON.stringify(this._timeline, null, 2)); - } - } - export default TimelineReporter; - ` - }, { ...params, reporter: 'list,json,./reporter.ts', workers: 2 }, env, options); - - let timeline; - try { - timeline = JSON.parse((await fs.promises.readFile(timelinePath, 'utf8')).toString()); - } catch (e) { - } - return { - ...result, - timeline - }; - }); - }, }); const TSCONFIG = { @@ -430,11 +382,11 @@ export function paintBlackPixels(image: Buffer, blackPixelsCount: number): Buffe return PNG.sync.write(png); } -export function allTests(result: RunResult) { - const tests: { title: string; expectedStatus: JSONReportTest['expectedStatus'], actualStatus: JSONReportTest['status'], annotations: string[] }[] = []; +function filterTests(result: RunResult, filter: (spec: JSONReportSpec) => boolean) { + const tests: JSONReportTest[] = []; const visit = (suite: JSONReportSuite) => { for (const spec of suite.specs) - spec.tests.forEach(t => tests.push({ title: spec.title, expectedStatus: t.expectedStatus, actualStatus: t.status, annotations: t.annotations.map(a => a.type) })); + spec.tests.forEach(t => filter(spec) && tests.push(t)); suite.suites?.forEach(s => visit(s)); }; visit(result.report.suites[0]); @@ -442,12 +394,12 @@ export function allTests(result: RunResult) { } export function expectTestHelper(result: RunResult) { - return (title: string, expectedStatus: string, status: string, annotations: any) => { - const tests = allTests(result).filter(t => t.title === title); + return (title: string, expectedStatus: string, status: string, annotations: string[]) => { + const tests = filterTests(result, s => s.title === title); for (const test of tests) { expect(test.expectedStatus, `title: ${title}`).toBe(expectedStatus); - expect(test.actualStatus, `title: ${title}`).toBe(status); - expect(test.annotations, `title: ${title}`).toEqual(annotations); + expect(test.status, `title: ${title}`).toBe(status); + expect(test.annotations.map(a => a.type), `title: ${title}`).toEqual(annotations); } }; } diff --git a/tests/playwright-test/reporter-html.spec.ts b/tests/playwright-test/reporter-html.spec.ts index c89b5ac094..ba76d9e125 100644 --- a/tests/playwright-test/reporter-html.spec.ts +++ b/tests/playwright-test/reporter-html.spec.ts @@ -760,23 +760,8 @@ test('open tests from required file', async ({ runInlineTest, showReport, page } }); test.describe('gitCommitInfo plugin', () => { - test('should include metadata', async ({ runInlineTest, showReport, page }) => { - const beforeRunPlaywrightTest = async ({ baseDir }: { baseDir: string }) => { - const execGit = async (args: string[]) => { - const { code, stdout, stderr } = await spawnAsync('git', args, { stdio: 'pipe', cwd: baseDir }); - if (!!code) - throw new Error(`Non-zero exit of:\n$ git ${args.join(' ')}\nConsole:\nstdout:\n${stdout}\n\nstderr:\n${stderr}\n\n`); - return; - }; - - await execGit(['init']); - await execGit(['config', '--local', 'user.email', 'shakespeare@example.local']); - await execGit(['config', '--local', 'user.name', 'William']); - await execGit(['add', '*.ts']); - await execGit(['commit', '-m', 'awesome commit message']); - }; - - const result = await runInlineTest({ + test('should include metadata', async ({ runInlineTest, writeFiles, showReport, page }) => { + const files = { 'uncommitted.txt': `uncommitted file`, 'playwright.config.ts': ` import { gitCommitInfo } from '@playwright/test/lib/plugins'; @@ -787,7 +772,29 @@ test.describe('gitCommitInfo plugin', () => { const { test } = pwt; test('sample', async ({}) => { expect(2).toBe(2); }); `, - }, { reporter: 'dot,html' }, { PW_TEST_HTML_REPORT_OPEN: 'never', GITHUB_REPOSITORY: 'microsoft/playwright-example-for-test', GITHUB_RUN_ID: 'example-run-id', GITHUB_SERVER_URL: 'https://playwright.dev', GITHUB_SHA: 'example-sha' }, undefined, beforeRunPlaywrightTest); + }; + const baseDir = await writeFiles(files); + + const execGit = async (args: string[]) => { + const { code, stdout, stderr } = await spawnAsync('git', args, { stdio: 'pipe', cwd: baseDir }); + if (!!code) + throw new Error(`Non-zero exit of:\n$ git ${args.join(' ')}\nConsole:\nstdout:\n${stdout}\n\nstderr:\n${stderr}\n\n`); + return; + }; + + await execGit(['init']); + await execGit(['config', '--local', 'user.email', 'shakespeare@example.local']); + await execGit(['config', '--local', 'user.name', 'William']); + await execGit(['add', '*.ts']); + await execGit(['commit', '-m', 'awesome commit message']); + + const result = await runInlineTest(files, { reporter: 'dot,html' }, { + PW_TEST_HTML_REPORT_OPEN: 'never', + GITHUB_REPOSITORY: 'microsoft/playwright-example-for-test', + GITHUB_RUN_ID: 'example-run-id', + GITHUB_SERVER_URL: 'https://playwright.dev', + GITHUB_SHA: 'example-sha', + }); await showReport(); diff --git a/tests/playwright-test/reporter-list.spec.ts b/tests/playwright-test/reporter-list.spec.ts index 11f41229cc..ad37104909 100644 --- a/tests/playwright-test/reporter-list.spec.ts +++ b/tests/playwright-test/reporter-list.spec.ts @@ -153,7 +153,7 @@ test('render retries', async ({ runInlineTest }) => { `, }, { reporter: 'list', retries: '1' }, { PW_TEST_DEBUG_REPORTERS: '1', PWTEST_TTY_WIDTH: '80' }); const text = result.output; - const lines = text.split('\n').filter(l => l.startsWith('0 :') || l.startsWith('1 :')).map(l => l.replace(/[\dm]+s/, 'XXms')); + const lines = text.split('\n').filter(l => l.startsWith('0 :') || l.startsWith('1 :')).map(l => l.replace(/\d+(\.\d+)?m?s/, 'XXms')); expect(lines).toEqual([ `0 : 1 a.test.ts:6:7 › flaky`,