diff --git a/src/test/runner.ts b/src/test/runner.ts index 18e22f8801..50954e2d0b 100644 --- a/src/test/runner.ts +++ b/src/test/runner.ts @@ -40,7 +40,17 @@ const removeFolderAsync = promisify(rimraf); const readDirAsync = promisify(fs.readdir); const readFileAsync = promisify(fs.readFile); -type RunResult = 'passed' | 'failed' | 'sigint' | 'forbid-only' | 'no-tests' | 'timedout'; +type RunResultStatus = 'passed' | 'failed' | 'sigint' | 'forbid-only' | 'clashing-spec-titles' | 'no-tests' | 'timedout'; + +type RunResult = { + status: Exclude; +} | { + status: 'forbid-only', + locations: string[] +} | { + status: 'clashing-spec-titles', + clashingSpecs: Map +}; export class Runner { private _loader: Loader; @@ -81,7 +91,7 @@ export class Runner { this._loader.loadEmptyConfig(rootDir); } - async run(list: boolean, filePatternFilters: FilePatternFilter[], projectName?: string): Promise { + async run(list: boolean, filePatternFilters: FilePatternFilter[], projectName?: string): Promise { this._reporter = this._createReporter(); const config = this._loader.fullConfig(); const globalDeadline = config.globalTimeout ? config.globalTimeout + monotonicTime() : undefined; @@ -93,17 +103,28 @@ export class Runner { await this._flushOutput(); return 'failed'; } - if (result === 'forbid-only') { + if (result?.status === 'forbid-only') { console.error('====================================='); console.error(' --forbid-only found a focused test.'); + for (const location of result?.locations) + console.error(` - ${location}`); console.error('====================================='); - } else if (result === 'no-tests') { + } else if (result!.status === 'no-tests') { console.error('================='); console.error(' no tests found.'); console.error('================='); + } else if (result?.status === 'clashing-spec-titles') { + console.error('================='); + console.error(' duplicate test titles are not allowed.'); + for (const [title, specs] of result?.clashingSpecs.entries()) { + console.error(` - title: ${title}`); + for (const spec of specs) + console.error(` - ${buildItemLocation(config.rootDir, spec)}`); + console.error('================='); + } } await this._flushOutput(); - return result!; + return result!.status!; } async _flushOutput() { @@ -155,8 +176,14 @@ export class Runner { const rootSuite = new Suite(''); for (const fileSuite of this._loader.fileSuites().values()) rootSuite._addSuite(fileSuite); - if (config.forbidOnly && rootSuite._hasOnly()) - return 'forbid-only'; + if (config.forbidOnly) { + const onlySpecAndSuites = rootSuite._getOnlyItems(); + if (onlySpecAndSuites.length > 0) + return { status: 'forbid-only', locations: onlySpecAndSuites.map(specOrSuite => `${buildItemLocation(config.rootDir, specOrSuite)} > ${specOrSuite.fullTitle()}`) }; + } + const uniqueSpecs = getUniqueSpecsPerSuite(rootSuite); + if (uniqueSpecs.size > 0) + return { status: 'clashing-spec-titles', clashingSpecs: uniqueSpecs }; filterOnly(rootSuite); filterByFocusedLine(rootSuite, testFileReFilters); @@ -185,7 +212,7 @@ export class Runner { const total = rootSuite.totalTestCount(); if (!total) - return 'no-tests'; + return { status: 'no-tests' }; await Promise.all(Array.from(outputDirs).map(outputDir => removeFolderAsync(outputDir).catch(e => {}))); @@ -227,8 +254,8 @@ export class Runner { this._reporter.onEnd(); if (sigint) - return 'sigint'; - return hasWorkerErrors || rootSuite.findSpec(spec => !spec.ok()) ? 'failed' : 'passed'; + return { status: 'sigint' }; + return { status: hasWorkerErrors || rootSuite.findSpec(spec => !spec.ok()) ? 'failed' : 'passed' }; } finally { if (globalSetupResult && typeof globalSetupResult === 'function') await globalSetupResult(this._loader.fullConfig()); @@ -335,3 +362,30 @@ async function collectFiles(testDir: string): Promise { await visit(testDir, [], 'included'); return files; } + +function getUniqueSpecsPerSuite(rootSuite: Suite): Map { + function visit(suite: Suite, clashingSpecs: Map) { + for (const childSuite of suite.suites) + visit(childSuite, clashingSpecs); + for (const spec of suite.specs) { + const fullTitle = spec.fullTitle(); + if (!clashingSpecs.has(fullTitle)) + clashingSpecs.set(fullTitle, []); + clashingSpecs.set(fullTitle, clashingSpecs.get(fullTitle)!.concat(spec)); + } + } + const out = new Map(); + for (const fileSuite of rootSuite.suites) { + const clashingSpecs = new Map(); + visit(fileSuite, clashingSpecs); + for (const [title, specs] of clashingSpecs.entries()) { + if (specs.length > 1) + out.set(title, specs); + } + } + return out; +} + +function buildItemLocation(rootDir: string, specOrSuite: Suite | Spec) { + return `${path.relative(rootDir, specOrSuite.file)}:${specOrSuite.line}`; +} diff --git a/src/test/test.ts b/src/test/test.ts index b763760418..f8dae60164 100644 --- a/src/test/test.ts +++ b/src/test/test.ts @@ -39,6 +39,10 @@ class Base { return this.parent.titlePath(); return [...this.parent.titlePath(), this.title]; } + + fullTitle(): string { + return this.titlePath().join(' '); + } } export class Spec extends Base implements reporterTypes.Spec { @@ -59,10 +63,6 @@ export class Spec extends Base implements reporterTypes.Spec { return !this.tests.find(r => !r.ok()); } - fullTitle(): string { - return this.titlePath().join(' '); - } - _testFullTitle(projectName: string) { return (projectName ? `[${projectName}] ` : '') + this.fullTitle(); } @@ -77,7 +77,7 @@ export class Suite extends Base implements reporterTypes.Suite { type: 'beforeEach' | 'afterEach' | 'beforeAll' | 'afterAll', fn: Function, location: Location, - } [] = []; + }[] = []; _addSpec(spec: Spec) { spec.parent = this; @@ -145,14 +145,14 @@ export class Suite extends Base implements reporterTypes.Suite { return result; } - _hasOnly(): boolean { + _getOnlyItems(): (Spec | Suite)[] { + const items: (Spec | Suite)[] = []; if (this._only) - return true; - if (this.suites.find(suite => suite._hasOnly())) - return true; - if (this.specs.find(spec => spec._only)) - return true; - return false; + items.push(this); + for (const suite of this.suites) + items.push(...suite._getOnlyItems()); + items.push(...this.specs.filter(spec => spec._only)); + return items; } _buildFixtureOverrides(): any { diff --git a/tests/playwright-test/basic.spec.ts b/tests/playwright-test/basic.spec.ts index 4e5866e757..a982d2543b 100644 --- a/tests/playwright-test/basic.spec.ts +++ b/tests/playwright-test/basic.spec.ts @@ -104,12 +104,12 @@ test('should respect excluded tests', async ({ runInlineTest }) => { expect(1 + 1).toBe(2); }); - test('excluded test', () => { + test('excluded test 1', () => { test.skip(); expect(1 + 1).toBe(3); }); - test('excluded test', () => { + test('excluded test 2', () => { test.skip(); expect(1 + 1).toBe(3); }); diff --git a/tests/playwright-test/fixtures.spec.ts b/tests/playwright-test/fixtures.spec.ts index f45e8193e8..fcc00f3505 100644 --- a/tests/playwright-test/fixtures.spec.ts +++ b/tests/playwright-test/fixtures.spec.ts @@ -191,13 +191,13 @@ test('should run the fixture every time', async ({ runInlineTest }) => { const test = pwt.test.extend({ asdf: async ({}, test) => await test(counter++), }); - test('should use asdf', async ({asdf}) => { + test('should use asdf 1', async ({asdf}) => { expect(asdf).toBe(0); }); - test('should use asdf', async ({asdf}) => { + test('should use asdf 2', async ({asdf}) => { expect(asdf).toBe(1); }); - test('should use asdf', async ({asdf}) => { + test('should use asdf 3', async ({asdf}) => { expect(asdf).toBe(2); }); `, @@ -212,13 +212,13 @@ test('should only run worker fixtures once', async ({ runInlineTest }) => { const test = pwt.test.extend({ asdf: [ async ({}, test) => await test(counter++), { scope: 'worker' } ], }); - test('should use asdf', async ({asdf}) => { + test('should use asdf 1', async ({asdf}) => { expect(asdf).toBe(0); }); - test('should use asdf', async ({asdf}) => { + test('should use asdf 2', async ({asdf}) => { expect(asdf).toBe(0); }); - test('should use asdf', async ({asdf}) => { + test('should use asdf 3', async ({asdf}) => { expect(asdf).toBe(0); }); `, diff --git a/tests/playwright-test/runner.spec.ts b/tests/playwright-test/runner.spec.ts new file mode 100644 index 0000000000..7fecfc01b8 --- /dev/null +++ b/tests/playwright-test/runner.spec.ts @@ -0,0 +1,61 @@ +/** + * 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'; + +test('it should not allow multiple tests with the same name per suite', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'tests/example.spec.js': ` + const { test } = pwt; + test('i-am-a-duplicate', async () => {}); + test('i-am-a-duplicate', async () => {}); + ` + }); + expect(result.exitCode).toBe(1); + expect(result.output).toContain('duplicate test titles are not allowed'); + expect(result.output).toContain(`- title: i-am-a-duplicate`); + expect(result.output).toContain(` - tests${path.sep}example.spec.js:6`); + expect(result.output).toContain(` - tests${path.sep}example.spec.js:7`); +}); + +test('it should enforce unique test names based on the describe block name', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'tests/example.spec.js': ` + const { test } = pwt; + test.describe('hello', () => { test('my world', () => {}) }); + test.describe('hello my', () => { test('world', () => {}) }); + test('hello my world', () => {}); + ` + }); + expect(result.exitCode).toBe(1); + expect(result.output).toContain('duplicate test titles are not allowed'); + expect(result.output).toContain(`- title: hello my world`); + expect(result.output).toContain(` - tests${path.sep}example.spec.js:6`); + expect(result.output).toContain(` - tests${path.sep}example.spec.js:7`); + expect(result.output).toContain(` - tests${path.sep}example.spec.js:8`); +}); + +test('it should not allow a focused test when forbid-only is used', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'tests/focused-test.spec.js': ` + const { test } = pwt; + test.only('i-am-focused', async () => {}); + ` + }, { 'forbid-only': true }); + expect(result.exitCode).toBe(1); + expect(result.output).toContain('--forbid-only found a focused test.'); + expect(result.output).toContain(`- tests${path.sep}focused-test.spec.js:6 > i-am-focused`); +}); diff --git a/tests/playwright-test/test-extend.spec.ts b/tests/playwright-test/test-extend.spec.ts index 045ae6da35..c6ee17ed50 100644 --- a/tests/playwright-test/test-extend.spec.ts +++ b/tests/playwright-test/test-extend.spec.ts @@ -71,10 +71,10 @@ test('test.extend should work', async ({ runInlineTest }) => { `, 'a.test.ts': ` import { test1, test2 } from './helper'; - test1('should work', async ({ derivedTest }) => { + test1('should work 1', async ({ derivedTest }) => { global.logs.push('test1'); }); - test2('should work', async ({ derivedTest }) => { + test2('should work 2', async ({ derivedTest }) => { global.logs.push('test2'); }); `, diff --git a/tests/playwright-test/worker-index.spec.ts b/tests/playwright-test/worker-index.spec.ts index 36ef0ea59a..b2afac1df6 100644 --- a/tests/playwright-test/worker-index.spec.ts +++ b/tests/playwright-test/worker-index.spec.ts @@ -52,15 +52,15 @@ test('should reuse worker for multiple tests', async ({ runInlineTest }) => { const result = await runInlineTest({ 'a.test.js': ` const { test } = pwt; - test('succeeds', async ({}, testInfo) => { + test('succeeds 1', async ({}, testInfo) => { expect(testInfo.workerIndex).toBe(0); }); - test('succeeds', async ({}, testInfo) => { + test('succeeds 2', async ({}, testInfo) => { expect(testInfo.workerIndex).toBe(0); }); - test('succeeds', async ({}, testInfo) => { + test('succeeds 3', async ({}, testInfo) => { expect(testInfo.workerIndex).toBe(0); }); `,