diff --git a/packages/playwright-test/src/dispatcher.ts b/packages/playwright-test/src/dispatcher.ts index 734ed1aa52..57aa468c93 100644 --- a/packages/playwright-test/src/dispatcher.ts +++ b/packages/playwright-test/src/dispatcher.ts @@ -65,7 +65,34 @@ export class Dispatcher { } } + private _processFullySkippedJobs() { + // If all the tests in a group are skipped, we report them immediately + // without sending anything to a worker. This avoids creating unnecessary worker processes. + // + // However, if there is at least one non-skipped test in a group, we'll send + // the whole group to the worker process and report tests in the natural order, + // with skipped tests mixed in-between non-skipped. This makes + // for a better reporter experience. + while (!this._isStopped && this._queue.length) { + const group = this._queue[0]; + const allTestsSkipped = group.tests.every(test => test.expectedStatus === 'skipped'); + if (!allTestsSkipped) + break; + + for (const test of group.tests) { + const result = test._appendTestResult(); + result.status = 'skipped'; + this._reporter.onTestBegin?.(test, result); + test.annotations = [...test._staticAnnotations]; + this._reportTestEnd(test, result); + } + this._queue.shift(); + } + } + private async _scheduleJob() { + this._processFullySkippedJobs(); + // 1. Find a job to run. if (this._isStopped || !this._queue.length) return; diff --git a/packages/playwright-test/src/suiteUtils.ts b/packages/playwright-test/src/suiteUtils.ts index 8b2ee6f925..b31c1c8982 100644 --- a/packages/playwright-test/src/suiteUtils.ts +++ b/packages/playwright-test/src/suiteUtils.ts @@ -49,19 +49,31 @@ export function buildFileSuiteForProject(project: FullProjectInternal, suite: Su result.forEachTest((test, suite) => { suite._fileId = fileId; const repeatEachIndexSuffix = repeatEachIndex ? ` (repeat:${repeatEachIndex})` : ''; + // At the point of the query, suite is not yet attached to the project, so we only get file, describe and test titles. const testIdExpression = `[project=${project._id}]${test.titlePath().join('\x1e')}${repeatEachIndexSuffix}`; const testId = fileId + '-' + calculateSha1(testIdExpression).slice(0, 20); test.id = testId; test.repeatEachIndex = repeatEachIndex; test._projectId = project._id; + + // Inherit properties from parent suites. let inheritedRetries: number | undefined; + let inheritedTimeout: number | undefined; for (let parentSuite: Suite | undefined = suite; parentSuite; parentSuite = parentSuite.parent) { test._staticAnnotations.push(...parentSuite._staticAnnotations); if (inheritedRetries === undefined && parentSuite._retries !== undefined) inheritedRetries = parentSuite._retries; + if (inheritedTimeout === undefined && parentSuite._timeout !== undefined) + inheritedTimeout = parentSuite._timeout; } test.retries = inheritedRetries ?? project.retries; + test.timeout = inheritedTimeout ?? project.timeout; + + // Skip annotations imply skipped expectedStatus. + if (test._staticAnnotations.some(a => a.type === 'skip' || a.type === 'fixme')) + test.expectedStatus = 'skipped'; + // We only compute / set digest in the runner. if (test._poolDigest) test._workerHash = `${project._id}-${test._poolDigest}-${repeatEachIndex}`; diff --git a/packages/playwright-test/src/test.ts b/packages/playwright-test/src/test.ts index c7353885e4..f5bff25012 100644 --- a/packages/playwright-test/src/test.ts +++ b/packages/playwright-test/src/test.ts @@ -41,7 +41,6 @@ export class Suite extends Base implements reporterTypes.Suite { location?: Location; parent?: Suite; _use: FixturesWithLocation[] = []; - _skipped = false; _entries: (Suite | TestCase)[] = []; _hooks: { type: 'beforeEach' | 'afterEach' | 'beforeAll' | 'afterAll', fn: Function, location: Location }[] = []; _timeout: number | undefined; @@ -164,7 +163,6 @@ export class Suite extends Base implements reporterTypes.Suite { staticAnnotations: this._staticAnnotations.slice(), modifiers: this._modifiers.slice(), parallelMode: this._parallelMode, - skipped: this._skipped, hooks: this._hooks.map(h => ({ type: h.type, location: h.location })), }; } @@ -179,7 +177,6 @@ export class Suite extends Base implements reporterTypes.Suite { suite._staticAnnotations = data.staticAnnotations; suite._modifiers = data.modifiers; suite._parallelMode = data.parallelMode; - suite._skipped = data.skipped; suite._hooks = data.hooks.map((h: any) => ({ type: h.type, location: h.location, fn: () => { } })); return suite; } diff --git a/packages/playwright-test/src/testType.ts b/packages/playwright-test/src/testType.ts index 7f05388453..bf0c261c1f 100644 --- a/packages/playwright-test/src/testType.ts +++ b/packages/playwright-test/src/testType.ts @@ -92,14 +92,8 @@ export class TestTypeImpl { if (type === 'only') test._only = true; - if (type === 'skip' || type === 'fixme') { + if (type === 'skip' || type === 'fixme') test._staticAnnotations.push({ type }); - test.expectedStatus = 'skipped'; - } - for (let parent: Suite | undefined = suite; parent; parent = parent.parent) { - if (parent._skipped) - test.expectedStatus = 'skipped'; - } } private _describe(type: 'default' | 'only' | 'serial' | 'serial.only' | 'parallel' | 'parallel.only' | 'skip' | 'fixme', location: Location, title: string | Function, fn?: Function) { @@ -124,10 +118,8 @@ export class TestTypeImpl { child._parallelMode = 'serial'; if (type === 'parallel' || type === 'parallel.only') child._parallelMode = 'parallel'; - if (type === 'skip' || type === 'fixme') { - child._skipped = true; + if (type === 'skip' || type === 'fixme') child._staticAnnotations.push({ type }); - } for (let parent: Suite | undefined = suite; parent; parent = parent.parent) { if (parent._parallelMode === 'serial' && child._parallelMode === 'parallel') diff --git a/packages/playwright-test/src/workerMain.ts b/packages/playwright-test/src/workerMain.ts index f7a5687ebf..731b1d6883 100644 --- a/packages/playwright-test/src/workerMain.ts +++ b/packages/playwright-test/src/workerMain.ts @@ -279,16 +279,10 @@ export class WorkerMain extends ProcessRunner { const reversedSuites = suites.slice().reverse(); const nextSuites = new Set(getSuites(nextTest)); - // Inherit test.setTimeout() from parent suites, deepest has the priority. - for (const suite of reversedSuites) { - if (suite._timeout !== undefined) { - testInfo._timeoutManager.setTimeout(suite._timeout); - break; - } - } - + testInfo._timeoutManager.setTimeout(test.timeout); for (const annotation of test._staticAnnotations) processAnnotation(annotation); + // Process existing annotations dynamically set for parent suites. for (const suite of suites) { const extraAnnotations = this._extraSuiteAnnotations.get(suite) || []; diff --git a/tests/playwright-test/playwright-test-fixtures.ts b/tests/playwright-test/playwright-test-fixtures.ts index 31b06ddd20..19991592e9 100644 --- a/tests/playwright-test/playwright-test-fixtures.ts +++ b/tests/playwright-test/playwright-test-fixtures.ts @@ -211,6 +211,8 @@ async function runPlaywrightCommand(childProcess: CommonFixtures['childProcess'] PW_TEST_REPORTER_WS_ENDPOINT: undefined, PW_TEST_SOURCE_TRANSFORM: undefined, PW_TEST_SOURCE_TRANSFORM_SCOPE: undefined, + TEST_WORKER_INDEX: undefined, + TEST_PARLLEL_INDEX: undefined, NODE_OPTIONS: undefined, ...env, }, diff --git a/tests/playwright-test/test-modifiers.spec.ts b/tests/playwright-test/test-modifiers.spec.ts index fb16b7727d..a40b8c1edc 100644 --- a/tests/playwright-test/test-modifiers.spec.ts +++ b/tests/playwright-test/test-modifiers.spec.ts @@ -543,3 +543,54 @@ test('should not run hooks if modifier throws', async ({ runInlineTest }) => { '%%modifier', ]); }); + +test('should report skipped tests in-order with correct properties', async ({ runInlineTest }) => { + const result = await runInlineTest({ + 'reporter.ts': ` + class Reporter { + onTestBegin(test) { + console.log('\\n%%begin-' + test.title); + } + onTestEnd(test, result) { + console.log('\\n%%end-' + test.title); + console.log('\\n%%expectedStatus-' + test.expectedStatus); + console.log('\\n%%timeout-' + test.timeout); + console.log('\\n%%retries-' + test.retries); + } + } + export default Reporter; + `, + 'playwright.config.ts': ` + module.exports = { reporter: [['./reporter.ts']] }; + `, + 'a.test.ts': ` + const { test } = pwt; + test.describe.configure({ timeout: 1234, retries: 3 }); + test('test1', async ({}) => { + }); + test.skip('test2', async ({}) => { + }); + test('test3', async ({}) => { + }); + `, + }, { reporter: '', workers: 1 }); + + expect(result.exitCode).toBe(0); + expect(result.output.split('\n').filter(line => line.startsWith('%%'))).toEqual([ + '%%begin-test1', + '%%end-test1', + '%%expectedStatus-passed', + '%%timeout-1234', + '%%retries-3', + '%%begin-test2', + '%%end-test2', + '%%expectedStatus-skipped', + '%%timeout-1234', + '%%retries-3', + '%%begin-test3', + '%%end-test3', + '%%expectedStatus-passed', + '%%timeout-1234', + '%%retries-3', + ]); +}); diff --git a/tests/playwright-test/worker-index.spec.ts b/tests/playwright-test/worker-index.spec.ts index 62c5c2d5d0..45070c19d3 100644 --- a/tests/playwright-test/worker-index.spec.ts +++ b/tests/playwright-test/worker-index.spec.ts @@ -219,3 +219,23 @@ test('parallelIndex should be in 0..workers-1', async ({ runInlineTest }) => { expect(result.exitCode).toBe(0); expect(result.passed).toBe(20); }); + +test('should not spawn workers for statically skipped tests', async ({ runInlineTest }) => { + test.info().annotations.push({ type: 'issue', description: 'https://github.com/microsoft/playwright/issues/20156' }); + + const result = await runInlineTest({ + 'a.test.js': ` + console.log('%%workerIndex=' + process.env.TEST_WORKER_INDEX); + const { test } = pwt; + test.describe.configure({ mode: 'parallel' }); + test('success', () => {}); + test.skip('skipped', () => {}); + `, + }, { workers: 2 }); + expect(result.exitCode).toBe(0); + expect(result.passed).toBe(1); + expect(result.skipped).toBe(1); + expect(result.output).toContain('workerIndex=undefined'); + expect(result.output).toContain('workerIndex=0'); + expect(result.output).not.toContain('workerIndex=1'); +});