From 3066ffd5774cef8261ad21388d8c8852550521b3 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Fri, 20 Jan 2023 08:36:31 -0800 Subject: [PATCH] chore: use fake pool on the runner side (#20241) --- packages/playwright-test/src/poolBuilder.ts | 33 ++++++++++++++------ packages/playwright-test/src/runner.ts | 32 ++++++++++++------- packages/playwright-test/src/suiteUtils.ts | 3 ++ packages/playwright-test/src/test.ts | 4 ++- packages/playwright-test/src/testLoader.ts | 2 +- packages/playwright-test/src/workerRunner.ts | 4 +-- tests/playwright-test/fixtures.spec.ts | 27 ---------------- tests/playwright-test/playwright.config.ts | 1 + tests/playwright-test/web-server.spec.ts | 8 ++--- 9 files changed, 57 insertions(+), 57 deletions(-) diff --git a/packages/playwright-test/src/poolBuilder.ts b/packages/playwright-test/src/poolBuilder.ts index f3ab69c11d..e4d1409515 100644 --- a/packages/playwright-test/src/poolBuilder.ts +++ b/packages/playwright-test/src/poolBuilder.ts @@ -20,18 +20,30 @@ import type { TestTypeImpl } from './testType'; import type { Fixtures, FixturesWithLocation, FullProjectInternal } from './types'; export class PoolBuilder { - private _project: FullProjectInternal; + private _project: FullProjectInternal | undefined; private _testTypePools = new Map(); + private _type: 'loader' | 'worker'; - constructor(project: FullProjectInternal) { + static buildForLoader(suite: Suite) { + new PoolBuilder('loader').buildPools(suite); + } + + static createForWorker(project: FullProjectInternal) { + return new PoolBuilder('worker', project); + } + + private constructor(type: 'loader' | 'worker', project?: FullProjectInternal) { + this._type = type; this._project = project; } - buildPools(suite: Suite, repeatEachIndex: number) { + buildPools(suite: Suite) { suite.forEachTest(test => { const pool = this._buildPoolForTest(test); - test._workerHash = `run${this._project._id}-${pool.digest}-repeat${repeatEachIndex}`; - test._pool = pool; + if (this._type === 'loader') + test._poolDigest = pool.digest; + if (this._type === 'worker') + test._pool = pool; }); } @@ -58,15 +70,16 @@ export class PoolBuilder { private _buildTestTypePool(testType: TestTypeImpl): FixturePool { if (!this._testTypePools.has(testType)) { - const fixtures = this._applyConfigUseOptions(testType, this._project.use || {}); + const fixtures = this._project ? this._applyConfigUseOptions(this._project, testType) : testType.fixtures; const pool = new FixturePool(fixtures); this._testTypePools.set(testType, pool); } return this._testTypePools.get(testType)!; } - private _applyConfigUseOptions(testType: TestTypeImpl, configUse: Fixtures): FixturesWithLocation[] { - const configKeys = new Set(Object.keys(configUse)); + private _applyConfigUseOptions(project: FullProjectInternal, testType: TestTypeImpl): FixturesWithLocation[] { + const projectUse = project.use || {}; + const configKeys = new Set(Object.keys(projectUse)); if (!configKeys.size) return testType.fixtures; const result: FixturesWithLocation[] = []; @@ -75,12 +88,12 @@ export class PoolBuilder { const optionsFromConfig: Fixtures = {}; for (const [key, value] of Object.entries(f.fixtures)) { if (isFixtureOption(value) && configKeys.has(key)) - (optionsFromConfig as any)[key] = [(configUse as any)[key], value[1]]; + (optionsFromConfig as any)[key] = [(projectUse as any)[key], value[1]]; } if (Object.entries(optionsFromConfig).length) { // Add config options immediately after original option definition, // so that any test.use() override it. - result.push({ fixtures: optionsFromConfig, location: { file: `project#${this._project._id}`, line: 1, column: 1 }, fromConfig: true }); + result.push({ fixtures: optionsFromConfig, location: { file: `project#${project._id}`, line: 1, column: 1 }, fromConfig: true }); } } return result; diff --git a/packages/playwright-test/src/runner.ts b/packages/playwright-test/src/runner.ts index dbf17d28da..21ed8bdbfa 100644 --- a/packages/playwright-test/src/runner.ts +++ b/packages/playwright-test/src/runner.ts @@ -283,19 +283,12 @@ export class Runner { const config = this._configLoader.fullConfig(); const fatalErrors: TestError[] = []; const allTestFiles = new Set(); - const testLoader = new TestLoader(config); for (const files of filesByProject.values()) files.forEach(file => allTestFiles.add(file)); - // Add all tests. - const preprocessRoot = new Suite('', 'root'); - for (const file of allTestFiles) { - const fileSuite = await testLoader.loadTestFile(file, 'runner'); - if (fileSuite._loadError) - fatalErrors.push(fileSuite._loadError); - // We have to clone only if there maybe subsequent calls of this method. - preprocessRoot._addSuite(fileSuite); - } + // Load all tests. + const { rootSuite: preprocessRoot, loadErrors } = await this._loadTests(allTestFiles); + fatalErrors.push(...loadErrors); // Complain about duplicate titles. fatalErrors.push(...createDuplicateTitlesErrors(config, preprocessRoot)); @@ -321,7 +314,6 @@ export class Runner { const rootSuite = new Suite('', 'root'); for (const [project, files] of filesByProject) { - const poolBuilder = new PoolBuilder(project); const grepMatcher = createTitleMatcher(project.grep); const grepInvertMatcher = project.grepInvert ? createTitleMatcher(project.grepInvert) : null; @@ -346,13 +338,29 @@ export class Runner { if (!filterTests(builtSuite, titleMatcher)) continue; projectSuite._addSuite(builtSuite); - poolBuilder.buildPools(builtSuite, repeatEachIndex); } } } return { rootSuite, fatalErrors }; } + private async _loadTests(testFiles: Set): Promise<{ rootSuite: Suite, loadErrors: TestError[] }> { + const config = this._configLoader.fullConfig(); + const testLoader = new TestLoader(config); + const loadErrors: TestError[] = []; + const rootSuite = new Suite('', 'root'); + for (const file of testFiles) { + const fileSuite = await testLoader.loadTestFile(file, 'loader'); + if (fileSuite._loadError) + loadErrors.push(fileSuite._loadError); + // We have to clone only if there maybe subsequent calls of this method. + rootSuite._addSuite(fileSuite); + } + // Generate hashes. + PoolBuilder.buildForLoader(rootSuite); + return { rootSuite, loadErrors }; + } + private _filterForCurrentShard(rootSuite: Suite, testGroups: TestGroup[]) { const shard = this._configLoader.fullConfig().shard; if (!shard) diff --git a/packages/playwright-test/src/suiteUtils.ts b/packages/playwright-test/src/suiteUtils.ts index 9ac2ec62a7..502d649496 100644 --- a/packages/playwright-test/src/suiteUtils.ts +++ b/packages/playwright-test/src/suiteUtils.ts @@ -52,6 +52,9 @@ export function buildFileSuiteForProject(project: FullProjectInternal, suite: Su break; } } + // We only compute / set digest in the runner. + if (test._poolDigest) + test._workerHash = `${project._id}-${test._poolDigest}-${repeatEachIndex}`; }); return result; diff --git a/packages/playwright-test/src/test.ts b/packages/playwright-test/src/test.ts index 442272c9ed..33e2fb8f0a 100644 --- a/packages/playwright-test/src/test.ts +++ b/packages/playwright-test/src/test.ts @@ -160,8 +160,9 @@ export class TestCase extends Base implements reporterTypes.TestCase { _testType: TestTypeImpl; id = ''; - _workerHash = ''; _pool: FixturePool | undefined; + _poolDigest = ''; + _workerHash = ''; _projectId = ''; // Annotations that are not added from within a test (like fixme and skip), should not // be re-added each time we retry a test. @@ -200,6 +201,7 @@ export class TestCase extends Base implements reporterTypes.TestCase { const test = new TestCase(this.title, this.fn, this._testType, this.location); test._only = this._only; test._requireFile = this._requireFile; + test._poolDigest = this._poolDigest; test.expectedStatus = this.expectedStatus; test.annotations = this.annotations.slice(); test._annotateWithInheritence = this._annotateWithInheritence; diff --git a/packages/playwright-test/src/testLoader.ts b/packages/playwright-test/src/testLoader.ts index a8ac9683f5..815696420e 100644 --- a/packages/playwright-test/src/testLoader.ts +++ b/packages/playwright-test/src/testLoader.ts @@ -34,7 +34,7 @@ export class TestLoader { this._fullConfig = fullConfig; } - async loadTestFile(file: string, environment: 'runner' | 'worker'): Promise { + async loadTestFile(file: string, environment: 'loader' | 'worker'): Promise { if (cachedFileSuites.has(file)) return cachedFileSuites.get(file)!; const suite = new Suite(path.relative(this._fullConfig.rootDir, file) || path.basename(file), 'file'); diff --git a/packages/playwright-test/src/workerRunner.ts b/packages/playwright-test/src/workerRunner.ts index 2e29b2eea8..94cf38e670 100644 --- a/packages/playwright-test/src/workerRunner.ts +++ b/packages/playwright-test/src/workerRunner.ts @@ -175,7 +175,7 @@ export class WorkerRunner extends ProcessRunner { this._configLoader = await ConfigLoader.deserialize(this._params.config); this._testLoader = new TestLoader(this._configLoader.fullConfig()); this._project = this._configLoader.fullConfig().projects.find(p => p._id === this._params.projectId)!; - this._poolBuilder = new PoolBuilder(this._project); + this._poolBuilder = PoolBuilder.createForWorker(this._project); } async runTestGroup(runPayload: RunPayload) { @@ -188,7 +188,7 @@ export class WorkerRunner extends ProcessRunner { const suite = buildFileSuiteForProject(this._project, fileSuite, this._params.repeatEachIndex); const hasEntries = filterTests(suite, test => entries.has(test.id)); if (hasEntries) { - this._poolBuilder.buildPools(suite, this._params.repeatEachIndex); + this._poolBuilder.buildPools(suite); this._extraSuiteAnnotations = new Map(); this._activeSuites = new Set(); this._didRunFullCleanup = false; diff --git a/tests/playwright-test/fixtures.spec.ts b/tests/playwright-test/fixtures.spec.ts index 34938d0c9c..7609e873af 100644 --- a/tests/playwright-test/fixtures.spec.ts +++ b/tests/playwright-test/fixtures.spec.ts @@ -714,30 +714,3 @@ test('worker teardown errors reflected in timed-out tests', async ({ runInlineTe expect(result.output).toContain('Test timeout of 1000ms exceeded.'); expect(result.output).toContain('Rejecting!'); }); - -test('should not complain about fixtures of non-focused tests', async ({ runInlineTest }) => { - const result = await runInlineTest({ - 'a.test.js': ` - const { test } = pwt; - test.only('works', () => {}); - test('unknown fixture', ({ foo }) => {}); - `, - }); - expect(result.exitCode).toBe(0); - expect(result.passed).toBe(1); -}); - -test('should not complain about fixtures of unused hooks', async ({ runInlineTest }) => { - const result = await runInlineTest({ - 'a.test.js': ` - const { test } = pwt; - test.only('works', () => {}); - test.describe('unused suite', () => { - test.beforeAll(({ foo }) => {}); - test('unused test', () => {}); - }); - `, - }); - expect(result.exitCode).toBe(0); - expect(result.passed).toBe(1); -}); diff --git a/tests/playwright-test/playwright.config.ts b/tests/playwright-test/playwright.config.ts index 8aa62bb688..f342a50851 100644 --- a/tests/playwright-test/playwright.config.ts +++ b/tests/playwright-test/playwright.config.ts @@ -24,6 +24,7 @@ const outputDir = path.join(__dirname, '..', '..', 'test-results'); const config: Config = { timeout: 30000, forbidOnly: !!process.env.CI, + fullyParallel: !process.env.CI, workers: process.env.CI ? 2 : undefined, preserveOutput: process.env.CI ? 'failures-only' : 'always', snapshotPathTemplate: '__screenshots__/{testFilePath}/{arg}{ext}', diff --git a/tests/playwright-test/web-server.spec.ts b/tests/playwright-test/web-server.spec.ts index e70b197b4d..10c4c5ccf9 100644 --- a/tests/playwright-test/web-server.spec.ts +++ b/tests/playwright-test/web-server.spec.ts @@ -279,7 +279,7 @@ test('should be able to specify the baseURL without the server', async ({ runInl }); test('should be able to specify a custom baseURL with the server', async ({ runInlineTest }, { workerIndex }) => { - const customWebServerPort = workerIndex + 10500; + const customWebServerPort = workerIndex * 2 + 10500; const webServerPort = customWebServerPort + 1; const server = http.createServer((req: http.IncomingMessage, res: http.ServerResponse) => { res.end('hello'); @@ -458,7 +458,7 @@ test('should send Accept header', async ({ runInlineTest, server }) => { }); test('should create multiple servers', async ({ runInlineTest }, { workerIndex }) => { - const port = workerIndex + 10500; + const port = workerIndex * 2 + 10500; const result = await runInlineTest({ 'test.spec.ts': ` const { test } = pwt; @@ -533,7 +533,7 @@ test('should create multiple servers', async ({ runInlineTest }, { workerIndex } test.describe('baseURL with plugins', () => { test('plugins do not set it', async ({ runInlineTest }, { workerIndex }) => { - const port = workerIndex + 10500; + const port = workerIndex * 2 + 10500; const result = await runInlineTest({ 'test.spec.ts': ` import { webServer } from '@playwright/test/lib/plugins'; @@ -553,7 +553,7 @@ test.describe('baseURL with plugins', () => { }); test('legacy config sets it alongside plugin', async ({ runInlineTest }, { workerIndex }) => { - const port = workerIndex + 10500; + const port = workerIndex * 2 + 10500; const result = await runInlineTest({ 'test.spec.ts': ` import { webServer } from '@playwright/test/lib/plugins';