From 3aae8c6be181800ec1082119eab85e787d5bd229 Mon Sep 17 00:00:00 2001 From: Pavel Feldman Date: Mon, 17 Aug 2020 10:33:42 -0700 Subject: [PATCH] test: run tests by ordinals, not ranges (#3497) --- test/base.fixture.ts | 17 ++++++------ test/focus.spec.ts | 1 + test/page-goto.spec.ts | 8 +++--- test/page-wait-for-navigation.spec.ts | 4 +-- test/runner/fixtures.js | 17 +++++++++++- test/runner/fixturesUI.js | 1 + test/runner/index.js | 2 +- test/runner/runner.js | 12 ++++----- test/runner/testRunner.js | 37 ++++++++++++++------------- test/runner/worker.js | 2 +- test/utils.js | 3 +-- 11 files changed, 61 insertions(+), 43 deletions(-) diff --git a/test/base.fixture.ts b/test/base.fixture.ts index ed1e75f9bb..12836a4222 100644 --- a/test/base.fixture.ts +++ b/test/base.fixture.ts @@ -25,7 +25,7 @@ import { Transport } from '../lib/rpc/transport'; import { setUnderTest } from '../lib/helper'; import { installCoverageHooks } from './runner/coverage'; import { valueFromEnv } from './runner/utils'; -import { registerFixture, registerWorkerFixture} from './runner/fixtures'; +import { registerFixture, registerWorkerFixture } from './runner/fixtures'; import './runner/builtin.fixtures'; import {mkdtempAsync, removeFolderAsync} from './utils'; @@ -42,6 +42,7 @@ declare global { golden: (path: string) => string; playwright: typeof import('../index'); browserType: BrowserType; + browserName: string; browser: Browser; } interface FixtureState { @@ -81,7 +82,7 @@ registerWorkerFixture('httpService', async ({parallelIndex}, test) => { ]); }); -const getExecutablePath = () => { +const getExecutablePath = (browserName) => { if (browserName === 'chromium' && process.env.CRPATH) return process.env.CRPATH; if (browserName === 'firefox' && process.env.FFPATH) @@ -91,8 +92,8 @@ const getExecutablePath = () => { return } -registerWorkerFixture('defaultBrowserOptions', async({}, test) => { - let executablePath = getExecutablePath(); +registerWorkerFixture('defaultBrowserOptions', async({browserName}, test) => { + let executablePath = getExecutablePath(browserName); if (executablePath) console.error(`Using executable at ${executablePath}`); @@ -104,7 +105,7 @@ registerWorkerFixture('defaultBrowserOptions', async({}, test) => { }); }); -registerWorkerFixture('playwright', async({parallelIndex}, test) => { +registerWorkerFixture('playwright', async({parallelIndex, browserName}, test) => { const {coverage, uninstall} = installCoverageHooks(browserName); if (process.env.PWWIRE) { const connection = new Connection(); @@ -146,9 +147,9 @@ registerFixture('toImpl', async ({playwright}, test) => { await test((playwright as any)._toImpl); }); -registerWorkerFixture('browserType', async ({playwright}, test) => { +registerWorkerFixture('browserType', async ({playwright, browserName}, test) => { const browserType = playwright[process.env.BROWSER || 'chromium'] - const executablePath = getExecutablePath() + const executablePath = getExecutablePath(browserName) if (executablePath) browserType._executablePath = executablePath await test(browserType); @@ -180,7 +181,7 @@ registerFixture('server', async ({httpService}, test) => { await test(httpService.server); }); -registerFixture('browserName', async ({}, test) => { +registerWorkerFixture('browserName', async ({}, test) => { await test(browserName); }); diff --git a/test/focus.spec.ts b/test/focus.spec.ts index 28053f4516..17f468b1bc 100644 --- a/test/focus.spec.ts +++ b/test/focus.spec.ts @@ -74,6 +74,7 @@ it('should traverse focus in all directions', async function({page}) { await page.keyboard.press('Shift+Tab'); expect(await page.evaluate(() => (document.activeElement as HTMLInputElement).value)).toBe('1'); }); + // Chromium and WebKit both have settings for tab traversing all links, but // it is only on by default in WebKit. it.skip(!MAC || !WEBKIT)('should traverse only form elements', async function({page}) { diff --git a/test/page-goto.spec.ts b/test/page-goto.spec.ts index 39e97a98e8..666d23eb4c 100644 --- a/test/page-goto.spec.ts +++ b/test/page-goto.spec.ts @@ -173,7 +173,7 @@ it('should fail when navigating to bad url', async({page, server}) => { expect(error.message).toContain('Invalid url'); }); -it('should fail when navigating to bad SSL', async({page, httpsServer}) => { +it('should fail when navigating to bad SSL', async({page, httpsServer, browserName}) => { // Make sure that network events do not emit 'undefined'. // @see https://crbug.com/750469 page.on('request', request => expect(request).toBeTruthy()); @@ -181,15 +181,15 @@ it('should fail when navigating to bad SSL', async({page, httpsServer}) => { page.on('requestfailed', request => expect(request).toBeTruthy()); let error = null; await page.goto(httpsServer.EMPTY_PAGE).catch(e => error = e); - utils.expectSSLError(error.message, ); + utils.expectSSLError(browserName, error.message, ); }); -it('should fail when navigating to bad SSL after redirects', async({page, server, httpsServer}) => { +it('should fail when navigating to bad SSL after redirects', async({page, server, httpsServer, browserName}) => { server.setRedirect('/redirect/1.html', '/redirect/2.html'); server.setRedirect('/redirect/2.html', '/empty.html'); let error = null; await page.goto(httpsServer.PREFIX + '/redirect/1.html').catch(e => error = e); - utils.expectSSLError(error.message); + utils.expectSSLError(browserName, error.message); }); it('should not crash when navigating to bad SSL after a cross origin navigation', async({page, server, httpsServer}) => { diff --git a/test/page-wait-for-navigation.spec.ts b/test/page-wait-for-navigation.spec.ts index 5e8e6930d5..8ecab93879 100644 --- a/test/page-wait-for-navigation.spec.ts +++ b/test/page-wait-for-navigation.spec.ts @@ -71,14 +71,14 @@ it('should work with clicking on anchor links', async({page, server}) => { expect(page.url()).toBe(server.EMPTY_PAGE + '#foobar'); }); -it('should work with clicking on links which do not commit navigation', async({page, server, httpsServer}) => { +it('should work with clicking on links which do not commit navigation', async({page, server, httpsServer, browserName}) => { await page.goto(server.EMPTY_PAGE); await page.setContent(`foobar`); const [error] = await Promise.all([ page.waitForNavigation().catch(e => e), page.click('a'), ]); - utils.expectSSLError(error.message); + utils.expectSSLError(browserName, error.message); }); it('should work with history.pushState()', async({page, server}) => { diff --git a/test/runner/fixtures.js b/test/runner/fixtures.js index 89deeff705..318d31a5a0 100644 --- a/test/runner/fixtures.js +++ b/test/runner/fixtures.js @@ -120,6 +120,21 @@ class FixturePool { } }; } + + fixtures(callback) { + const result = new Set(); + const visit = (callback) => { + for (const name of fixtureParameterNames(callback)) { + if (name in result) + continue; + result.add(name); + const { fn } = registrations.get(name) + visit(fn); + } + }; + visit(callback); + return result; + } } function fixtureParameterNames(fn) { @@ -146,7 +161,7 @@ function registerFixture(name, fn) { innerRegisterFixture(name, 'test', fn); }; -function registerWorkerFixture (name, fn) { +function registerWorkerFixture(name, fn) { innerRegisterFixture(name, 'worker', fn); }; diff --git a/test/runner/fixturesUI.js b/test/runner/fixturesUI.js index 235e1b7027..769d793b94 100644 --- a/test/runner/fixturesUI.js +++ b/test/runner/fixturesUI.js @@ -81,6 +81,7 @@ function fixturesUI(testRunner, suite) { wrapper.__original = fn; } const test = new Test(title, wrapper); + test.__fixtures = fixturePool.fixtures(fn); test.file = file; suite.addTest(test); const only = specs.only && specs.only[0]; diff --git a/test/runner/index.js b/test/runner/index.js index e09ee6ab87..2523eb8d97 100644 --- a/test/runner/index.js +++ b/test/runner/index.js @@ -43,7 +43,7 @@ program let total = 0; // Build the test model, suite per file. for (const file of files) { - const testRunner = new TestRunner(file, 0, { + const testRunner = new TestRunner(file, [], { forbidOnly: command.forbidOnly || undefined, grep: command.grep, reporter: NullReporter, diff --git a/test/runner/runner.js b/test/runner/runner.js index ea9b9ac741..f303cbf0be 100644 --- a/test/runner/runner.js +++ b/test/runner/runner.js @@ -67,8 +67,8 @@ class Runner extends EventEmitter { _filesSortedByWorkerHash() { const result = []; - for (const file of this._files.keys()) - result.push({ file, hash: computeWorkerHash(file), startOrdinal: 0 }); + for (const [file, count] of this._files.entries()) + result.push({ file, hash: computeWorkerHash(file), ordinals: new Array(count).fill(0).map((_, i) => i) }); result.sort((a, b) => a.hash < b.hash ? -1 : (a.hash === b.hash ? 0 : 1)); return result; } @@ -111,8 +111,8 @@ class Runner extends EventEmitter { if (params.error) { this._restartWorker(worker); // If there are remaining tests, we will queue them. - if (params.remaining) - this._queue.unshift({ ...entry, startOrdinal: params.total - params.remaining }); + if (params.remaining.length) + this._queue.unshift({ ...entry, ordinals: params.remaining }); } else { this._workerAvailable(worker); } @@ -231,7 +231,7 @@ class OopWorker extends EventEmitter { run(entry) { this.hash = entry.hash; - this.process.send({ method: 'run', params: { file: entry.file, startOrdinal: entry.startOrdinal, options: this.runner._options } }); + this.process.send({ method: 'run', params: { file: entry.file, ordinals: entry.ordinals, options: this.runner._options } }); } stop() { @@ -268,7 +268,7 @@ class InProcessWorker extends EventEmitter { async run(entry) { delete require.cache[entry.file]; const { TestRunner } = require('./testRunner'); - const testRunner = new TestRunner(entry.file, entry.startOrdinal, this.runner._options); + const testRunner = new TestRunner(entry.file, entry.ordinals, this.runner._options); for (const event of ['test', 'pending', 'pass', 'fail', 'done']) testRunner.on(event, this.emit.bind(this, event)); testRunner.run(); diff --git a/test/runner/testRunner.js b/test/runner/testRunner.js index b64aeb1e7b..be3443d724 100644 --- a/test/runner/testRunner.js +++ b/test/runner/testRunner.js @@ -26,7 +26,7 @@ const GoldenUtils = require('./GoldenUtils'); class NullReporter {} class TestRunner extends EventEmitter { - constructor(file, startOrdinal, options) { + constructor(file, ordinals, options) { super(); this.mocha = new Mocha({ forbidOnly: options.forbidOnly, @@ -38,7 +38,8 @@ class TestRunner extends EventEmitter { this.mocha.grep(options.grep); this._currentOrdinal = -1; this._failedWithError = false; - this._startOrdinal = startOrdinal; + this._ordinals = new Set(ordinals); + this._remaining = new Set(ordinals); this._trialRun = options.trialRun; this._passes = 0; this._failures = 0; @@ -54,39 +55,39 @@ class TestRunner extends EventEmitter { let callback; const result = new Promise(f => callback = f); const runner = this.mocha.run(callback); - let remaining = 0; const constants = Mocha.Runner.constants; runner.on(constants.EVENT_TEST_BEGIN, test => { relativeTestFile = this._relativeTestFile; - if (this._failedWithError) { - ++remaining; + if (this._failedWithError) return; - } - if (++this._currentOrdinal < this._startOrdinal) + const ordinal = ++this._currentOrdinal; + if (this._ordinals.size && !this._ordinals.has(ordinal)) return; - this.emit('test', { test: serializeTest(test, this._currentOrdinal) }); + this._remaining.delete(ordinal); + this.emit('test', { test: serializeTest(test, ordinal) }); }); runner.on(constants.EVENT_TEST_PENDING, test => { - if (this._failedWithError) { - ++remaining; + if (this._failedWithError) return; - } - if (++this._currentOrdinal < this._startOrdinal) + const ordinal = ++this._currentOrdinal; + if (this._ordinals.size && !this._ordinals.has(ordinal)) return; + this._remaining.delete(ordinal); ++this._pending; - this.emit('pending', { test: serializeTest(test, this._currentOrdinal) }); + this.emit('pending', { test: serializeTest(test, ordinal) }); }); runner.on(constants.EVENT_TEST_PASS, test => { if (this._failedWithError) return; - if (this._currentOrdinal < this._startOrdinal) + const ordinal = this._currentOrdinal; + if (this._ordinals.size && !this._ordinals.has(ordinal)) return; ++this._passes; - this.emit('pass', { test: serializeTest(test, this._currentOrdinal) }); + this.emit('pass', { test: serializeTest(test, ordinal) }); }); runner.on(constants.EVENT_TEST_FAIL, (test, error) => { @@ -104,7 +105,7 @@ class TestRunner extends EventEmitter { this.emit('done', { stats: this._serializeStats(runner.stats), error: this._failedWithError, - remaining, + remaining: [...this._remaining], total: runner.stats.tests }); }); @@ -116,10 +117,10 @@ class TestRunner extends EventEmitter { return false; if (hook) { // Hook starts before we bump the test ordinal. - if (this._currentOrdinal + 1 < this._startOrdinal) + if (!this._ordinals.has(this._currentOrdinal + 1)) return false; } else { - if (this._currentOrdinal < this._startOrdinal) + if (!this._ordinals.has(this._currentOrdinal)) return false; } return true; diff --git a/test/runner/worker.js b/test/runner/worker.js index 001ce4cfa9..12409f9873 100644 --- a/test/runner/worker.js +++ b/test/runner/worker.js @@ -62,7 +62,7 @@ process.on('message', async message => { return; } if (message.method === 'run') { - const testRunner = new TestRunner(message.params.file, message.params.startOrdinal, message.params.options); + const testRunner = new TestRunner(message.params.file, message.params.ordinals, message.params.options); for (const event of ['test', 'pending', 'pass', 'fail', 'done']) testRunner.on(event, sendMessageToParent.bind(null, event)); await testRunner.run(); diff --git a/test/utils.js b/test/utils.js index 7fa60c7461..3a1fea07b3 100644 --- a/test/utils.js +++ b/test/utils.js @@ -23,7 +23,6 @@ const removeFolder = require('rimraf'); const {FlakinessDashboard} = require('../utils/flakiness-dashboard'); const PROJECT_ROOT = fs.existsSync(path.join(__dirname, '..', 'package.json')) ? path.join(__dirname, '..') : path.join(__dirname, '..', '..'); -const browserName = process.env.BROWSER || 'chromium'; let platform = os.platform(); @@ -227,7 +226,7 @@ const utils = module.exports = { return logger; }, - expectSSLError(errorMessage) { + expectSSLError(browserName, errorMessage) { if (browserName === 'chromium') { expect(errorMessage).toContain('net::ERR_CERT_AUTHORITY_INVALID'); } else if (browserName === 'webkit') {