fix(test runner): disallow use(workerFixture) in describes (#8119)

Using a worker fixture forces a new worker. This might be unexpected
when part of the test file runs in one worker, and another runs
in another worker. Top-level use of worker fixtures is still fine.

src/test/fixtures.ts

@@ -105,7 +105,7 @@ export class FixturePool {
   readonly digest: string;
   readonly registrations: Map<string, FixtureRegistration>;
 
-  constructor(fixturesList: FixturesWithLocation[], parentPool?: FixturePool) {
+  constructor(fixturesList: FixturesWithLocation[], parentPool?: FixturePool, disallowWorkerFixtures?: boolean) {
     this.registrations = new Map(parentPool ? parentPool.registrations : []);
 
     for (const { fixtures, location } of fixturesList) {
@@ -136,6 +136,8 @@ export class FixturePool {
 
         if (options.scope !== 'test' && options.scope !== 'worker')
           throw errorWithLocations(`Fixture "${name}" has unknown { scope: '${options.scope}' }.`, { location, name });
+        if (options.scope === 'worker' && disallowWorkerFixtures)
+          throw errorWithLocations(`Cannot use({ ${name} }) in a describe group, because it forces a new worker.\nMake it top-level in the test file or put in the configuration file.`, { location, name });
 
         const deps = fixtureParameterNames(fn, location);
         const registration: FixtureRegistration = { id: '', name, location, scope: options.scope, fn, auto: options.auto, deps, super: previous };

src/test/project.ts

@@ -56,19 +56,15 @@ export class ProjectImpl {
   private buildPool(test: TestCase): FixturePool {
     if (!this.testPools.has(test)) {
       let pool = this.buildTestTypePool(test._testType);
-      const overrides: Fixtures = test.parent!._buildFixtureOverrides();
-      if (Object.entries(overrides).length) {
-        const overridesWithLocation = {
-          fixtures: overrides,
-          // TODO: pass location from test.use() callsite.
-          location: test.location,
-        };
-        pool = new FixturePool([overridesWithLocation], pool);
-      }
-      this.testPools.set(test, pool);
 
-      pool.validateFunction(test.fn, 'Test', test.location);
-      for (let parent = test.parent; parent; parent = parent.parent) {
+      const parents: Suite[] = [];
+      for (let parent = test.parent; parent; parent = parent.parent)
+        parents.push(parent);
+      parents.reverse();
+
+      for (const parent of parents) {
+        if (parent._use.length)
+          pool = new FixturePool(parent._use, pool, parent._isDescribe);
         for (const hook of parent._eachHooks)
           pool.validateFunction(hook.fn, hook.type + ' hook', hook.location);
         for (const hook of parent._allHooks)
@@ -76,6 +72,9 @@ export class ProjectImpl {
         for (const modifier of parent._modifiers)
           pool.validateFunction(modifier.fn, modifier.type + ' modifier', modifier.location);
       }
+
+      pool.validateFunction(test.fn, 'Test', test.location);
+      this.testPools.set(test, pool);
     }
     return this.testPools.get(test)!;
   }

src/test/test.ts

@@ -17,7 +17,7 @@
 import type { FixturePool } from './fixtures';
 import * as reporterTypes from '../../types/testReporter';
 import type { TestTypeImpl } from './testType';
-import { Annotations, Location } from './types';
+import { Annotations, FixturesWithLocation, Location } from './types';
 
 class Base {
   title: string;
@@ -48,7 +48,8 @@ export class Suite extends Base implements reporterTypes.Suite {
   suites: Suite[] = [];
   tests: TestCase[] = [];
   location?: Location;
-  _fixtureOverrides: any = {};
+  _use: FixturesWithLocation[] = [];
+  _isDescribe = false;
   _entries: (Suite | TestCase)[] = [];
   _allHooks: TestCase[] = [];
   _eachHooks: { type: 'beforeEach' | 'afterEach', fn: Function, location: Location }[] = [];
@@ -97,20 +98,17 @@ export class Suite extends Base implements reporterTypes.Suite {
     return items;
   }
 
-  _buildFixtureOverrides(): any {
-    return this.parent ? { ...this.parent._buildFixtureOverrides(), ...this._fixtureOverrides } : this._fixtureOverrides;
-  }
-
   _clone(): Suite {
     const suite = new Suite(this.title);
     suite._only = this._only;
     suite.location = this.location;
     suite._requireFile = this._requireFile;
-    suite._fixtureOverrides = this._fixtureOverrides;
+    suite._use = this._use.slice();
     suite._eachHooks = this._eachHooks.slice();
     suite._timeout = this._timeout;
     suite._annotations = this._annotations.slice();
     suite._modifiers = this._modifiers.slice();
+    suite._isDescribe = this._isDescribe;
     return suite;
   }
 }

src/test/testType.ts

@@ -92,6 +92,7 @@ export class TestTypeImpl {
 
     const child = new Suite(title);
     child._requireFile = suite._requireFile;
+    child._isDescribe = true;
     child.location = location;
     suite._addSuite(child);
 
@@ -161,7 +162,7 @@ export class TestTypeImpl {
     const suite = currentlyLoadingFileSuite();
     if (!suite)
       throw errorWithLocation(location, `test.use() can only be called in a test file`);
-    suite._fixtureOverrides = { ...suite._fixtureOverrides, ...fixtures };
+    suite._use.push({ fixtures, location });
   }
 
   private async _step(location: Location, title: string, body: () => Promise<void>): Promise<void> {

tests/playwright-test/shard.spec.ts

@@ -17,28 +17,28 @@
 import { test, expect } from './playwright-test-fixtures';
 
 const tests = {
+  'helper.ts': `
+    export const headlessTest = pwt.test.extend({ headless: false });
+    export const headedTest = pwt.test.extend({ headless: true });
+  `,
   'a.spec.ts': `
-    const test = pwt.test.extend({ foo: 'bar' });
-    test.use({ headless: false });
-    test('test1', async () => {
+    import { headlessTest, headedTest } from './helper';
+    headlessTest('test1', async () => {
       console.log('test1-done');
     });
-    test.describe('suite', () => {
-      test.use({ headless: true });
-      test('test2', async () => {
-        console.log('test2-done');
-      });
+    headedTest('test2', async () => {
+      console.log('test2-done');
     });
-    test('test3', async () => {
+    headlessTest('test3', async () => {
       console.log('test3-done');
     });
   `,
   'b.spec.ts': `
-    const test = pwt.test.extend({ bar: 'foo' });
-    test('test4', async () => {
+    import { headlessTest, headedTest } from './helper';
+    headlessTest('test4', async () => {
       console.log('test4-done');
     });
-    test('test5', async () => {
+    headedTest('test5', async () => {
       console.log('test5-done');
     });
   `,
@@ -51,7 +51,7 @@ test('should respect shard=1/2', async ({ runInlineTest }) => {
   expect(result.skipped).toBe(0);
   expect(result.output).toContain('test1-done');
   expect(result.output).toContain('test3-done');
-  expect(result.output).toContain('test2-done');
+  expect(result.output).toContain('test4-done');
 });
 
 test('should respect shard=2/2', async ({ runInlineTest }) => {
@@ -59,7 +59,7 @@ test('should respect shard=2/2', async ({ runInlineTest }) => {
   expect(result.exitCode).toBe(0);
   expect(result.passed).toBe(2);
   expect(result.skipped).toBe(0);
-  expect(result.output).toContain('test4-done');
+  expect(result.output).toContain('test2-done');
   expect(result.output).toContain('test5-done');
 });
 
@@ -75,5 +75,5 @@ test('should respect shard=1/2 in config', async ({ runInlineTest }) => {
   expect(result.skipped).toBe(0);
   expect(result.output).toContain('test1-done');
   expect(result.output).toContain('test3-done');
-  expect(result.output).toContain('test2-done');
+  expect(result.output).toContain('test4-done');
 });

tests/playwright-test/test-use.spec.ts

@@ -71,6 +71,28 @@ test('should run tests with different test options in the same worker', async ({
   expect(result.passed).toBe(3);
 });
 
+test('should throw when setting worker options in describe', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.test.ts': `
+      const test = pwt.test.extend({
+        foo: [undefined, { scope: 'worker' }],
+      });
+      test.describe('suite', () => {
+        test.use({ foo: 'bar' });
+        test('test', ({ foo }, testInfo) => {
+        });
+      });
+    `
+  }, { workers: 1 });
+
+  expect(result.exitCode).toBe(1);
+  expect(result.output).toContain([
+    `Cannot use({ foo }) in a describe group, because it forces a new worker.`,
+    `Make it top-level in the test file or put in the configuration file.`,
+    `  "foo" defined at a.test.ts:9:14`,
+  ].join('\n'));
+});
+
 test('should run tests with different worker options', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'helper.ts': `
@@ -82,58 +104,35 @@ test('should run tests with different worker options', async ({ runInlineTest })
       import { test } from './helper';
       test('test', ({ foo }, testInfo) => {
         expect(foo).toBe(undefined);
-        console.log('\\n%%test=' + testInfo.workerIndex);
-      });
-
-      test.describe('suite1', () => {
-        test.use({ foo: 'bar' });
-        test('test1', ({ foo }, testInfo) => {
-          expect(foo).toBe('bar');
-          console.log('\\n%%test1=' + testInfo.workerIndex);
-        });
-
-        test.describe('suite2', () => {
-          test.use({ foo: 'baz' });
-          test('test2', ({ foo }, testInfo) => {
-            expect(foo).toBe('baz');
-            console.log('\\n%%test2=' + testInfo.workerIndex);
-          });
-        });
-
-        test('test3', ({ foo }, testInfo) => {
-          expect(foo).toBe('bar');
-          console.log('\\n%%test3=' + testInfo.workerIndex);
-        });
+        expect(testInfo.workerIndex).toBe(0);
       });
     `,
     'b.test.ts': `
       import { test } from './helper';
-      test.use({ foo: 'qux' });
-      test('test4', ({ foo }, testInfo) => {
-        expect(foo).toBe('qux');
-        console.log('\\n%%test4=' + testInfo.workerIndex);
+      test.use({ foo: 'bar' });
+
+      test('test1', ({ foo }, testInfo) => {
+        expect(foo).toBe('bar');
+        expect(testInfo.workerIndex).toBe(1);
+      });
+
+      test('test2', ({ foo }, testInfo) => {
+        expect(foo).toBe('bar');
+        expect(testInfo.workerIndex).toBe(1);
+      });
+    `,
+    'c.test.ts': `
+      import { test } from './helper';
+      test.use({ foo: 'baz' });
+      test('test2', ({ foo }, testInfo) => {
+        expect(foo).toBe('baz');
+        expect(testInfo.workerIndex).toBe(2);
       });
     `
   }, { workers: 1 });
 
   expect(result.exitCode).toBe(0);
-  expect(result.passed).toBe(5);
-
-  const workerIndexMap = new Map();
-  const allWorkers = new Set();
-  for (const line of result.output.split('\n')) {
-    if (line.startsWith('%%')) {
-      const [ name, workerIndex ] = line.substring(2).split('=');
-      allWorkers.add(workerIndex);
-      workerIndexMap.set(name, workerIndex);
-    }
-  }
-
-  expect(workerIndexMap.size).toBe(5);
-  expect(workerIndexMap.get('test1')).toBe(workerIndexMap.get('test3'));
-  expect(allWorkers.size).toBe(4);
-  for (let i = 0; i < 4; i++)
-    expect(allWorkers.has(String(i)));
+  expect(result.passed).toBe(4);
 });
 
 test('should use options from the config', async ({ runInlineTest }) => {