feat(test-runner): do only allow unique spec titles per suite (#7300)

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<RunResultStatus, 'forbid-only' | 'clashing-spec-titles'>;
+} | {
+  status: 'forbid-only',
+  locations: string[]
+} | {
+  status: 'clashing-spec-titles',
+  clashingSpecs: Map<string, Spec[]>
+};
 
 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<RunResult> {
+  async run(list: boolean, filePatternFilters: FilePatternFilter[], projectName?: string): Promise<RunResultStatus> {
     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())
+      if (config.forbidOnly) {
-        return 'forbid-only';
+        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 { status: 'sigint' };
-      return hasWorkerErrors || rootSuite.findSpec(spec => !spec.ok()) ? 'failed' : 'passed';
+      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<string[]> {
   await visit(testDir, [], 'included');
   return files;
 }
+
+function getUniqueSpecsPerSuite(rootSuite: Suite): Map<string, Spec[]> {
+  function visit(suite: Suite, clashingSpecs: Map<string, Spec[]>) {
+    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<string, Spec[]>();
+  for (const fileSuite of rootSuite.suites) {
+    const clashingSpecs = new Map<string, Spec[]>();
+    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}`;
+}

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;
+      items.push(this);
-    if (this.suites.find(suite => suite._hasOnly()))
+    for (const suite of this.suites)
-      return true;
+      items.push(...suite._getOnlyItems());
-    if (this.specs.find(spec => spec._only))
+    items.push(...this.specs.filter(spec => spec._only));
-      return true;
+    return items;
-    return false;
   }
 
   _buildFixtureOverrides(): any {

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);
       });

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);
       });
     `,

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`);
+});

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');
       });
     `,

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);
       });
     `,