From 6522e4f524bdbc1f1b9d040772acf862517ed507 Mon Sep 17 00:00:00 2001 From: Jack Franklin Date: Fri, 3 Apr 2020 12:22:55 +0100 Subject: [PATCH] chore: Use expect for assertions (#5581) Rather than use our own custom expect library, we can use expect from npm [1], which has an API almost identical to the one Puppeteer has, but with more options, better diffing, and is used by many in the community as it's the default assertions library that comes with Jest. It's also thoroughly documented [2]. [1]: https://www.npmjs.com/package/expect [2]: https://jestjs.io/docs/en/expect --- index.js | 4 + package.json | 1 + src/Page.js | 3 - test/puppeteer.spec.js | 11 +- test/target.spec.js | 4 +- test/utils.js | 15 +++ utils/browser/test.js | 4 +- utils/doclint/check_public_api/test/test.js | 22 ++-- utils/doclint/preprocessor/test.js | 4 +- utils/testrunner/Matchers.js | 130 -------------------- utils/testrunner/README.md | 6 +- utils/testrunner/examples/fail.js | 4 +- utils/testrunner/examples/hookfail.js | 4 +- utils/testrunner/examples/hooktimeout.js | 4 +- utils/testrunner/index.js | 3 +- utils/testrunner/test/test.js | 4 +- 16 files changed, 55 insertions(+), 168 deletions(-) delete mode 100644 utils/testrunner/Matchers.js diff --git a/index.js b/index.js index 5254885dfb3d2..9b09868c3c790 100644 --- a/index.js +++ b/index.js @@ -16,12 +16,16 @@ const {helper} = require('./lib/helper'); const api = require('./lib/api'); +const {Page} = require('./lib/Page'); for (const className in api) { // Puppeteer-web excludes certain classes from bundle, e.g. BrowserFetcher. if (typeof api[className] === 'function') helper.installAsyncStackHooks(api[className]); } +// Expose alias for deprecated method. +Page.prototype.emulateMedia = Page.prototype.emulateMediaType; + // If node does not support async await, use the compiled version. const Puppeteer = require('./lib/Puppeteer'); const packageJson = require('./package.json'); diff --git a/package.json b/package.json index 8c9bebdce7e96..4b54d7375141a 100644 --- a/package.json +++ b/package.json @@ -58,6 +58,7 @@ "cross-env": "^5.0.5", "eslint": "^6.8.0", "esprima": "^4.0.0", + "expect": "^25.2.7", "jpeg-js": "^0.3.4", "minimist": "^1.2.0", "ncp": "^2.0.0", diff --git a/src/Page.js b/src/Page.js index 9be58789a2ba9..81ff80483c9d7 100644 --- a/src/Page.js +++ b/src/Page.js @@ -1146,9 +1146,6 @@ class Page extends EventEmitter { } } -// Expose alias for deprecated method. -Page.prototype.emulateMedia = Page.prototype.emulateMediaType; - /** * @typedef {Object} PDFOptions * @property {number=} scale diff --git a/test/puppeteer.spec.js b/test/puppeteer.spec.js index dcc7788b4ac85..a6fa47d435ebf 100644 --- a/test/puppeteer.spec.js +++ b/test/puppeteer.spec.js @@ -16,8 +16,9 @@ const fs = require('fs'); const path = require('path'); const rm = require('rimraf').sync; -const GoldenUtils = require('./golden-utils'); -const {Matchers} = require('../utils/testrunner/'); +const utils = require('./utils'); + +const expect = require('expect'); const YELLOW_COLOR = '\x1b[33m'; const RESET_COLOR = '\x1b[0m'; @@ -59,13 +60,13 @@ module.exports.addTests = ({testRunner, product, puppeteerPath}) => { } const suffix = FFOX ? 'firefox' : product.toLowerCase(); + const GOLDEN_DIR = path.join(__dirname, 'golden-' + suffix); const OUTPUT_DIR = path.join(__dirname, 'output-' + suffix); if (fs.existsSync(OUTPUT_DIR)) rm(OUTPUT_DIR); - const {expect} = new Matchers({ - toBeGolden: GoldenUtils.compare.bind(null, GOLDEN_DIR, OUTPUT_DIR) - }); + + utils.extendExpectWithToBeGolden(GOLDEN_DIR, OUTPUT_DIR); const testOptions = { testRunner, diff --git a/test/target.spec.js b/test/target.spec.js index 23d65fd41e7ad..dd0d81bb2f0e7 100644 --- a/test/target.spec.js +++ b/test/target.spec.js @@ -27,8 +27,8 @@ module.exports.addTests = function({testRunner, expect, puppeteer}) { // The pages will be the testing page and the original newtab page const targets = browser.targets(); expect(targets.some(target => target.type() === 'page' && - target.url() === 'about:blank')).toBeTruthy('Missing blank page'); - expect(targets.some(target => target.type() === 'browser')).toBeTruthy('Missing browser target'); + target.url() === 'about:blank')).toBeTruthy(); + expect(targets.some(target => target.type() === 'browser')).toBeTruthy(); }); it('Browser.pages should return all of the pages', async({page, server, context}) => { // The pages will be the testing page diff --git a/test/utils.js b/test/utils.js index d95a087da9ff9..4ed56e6027fbc 100644 --- a/test/utils.js +++ b/test/utils.js @@ -16,6 +16,8 @@ const fs = require('fs'); const path = require('path'); +const expect = require('expect'); +const GoldenUtils = require('./golden-utils'); const {FlakinessDashboard} = require('../utils/flakiness-dashboard'); const PROJECT_ROOT = fs.existsSync(path.join(__dirname, '..', 'package.json')) ? path.join(__dirname, '..') : path.join(__dirname, '..', '..'); @@ -72,6 +74,19 @@ const utils = module.exports = { }); }, + extendExpectWithToBeGolden: function(goldenDir, outputDir) { + expect.extend({ + toBeGolden: (testScreenshot, goldenFilePath) => { + const result = GoldenUtils.compare(goldenDir, outputDir, testScreenshot, goldenFilePath); + + return { + message: () => result.message, + pass: result.pass + }; + } + }); + }, + /** * @return {string} */ diff --git a/utils/browser/test.js b/utils/browser/test.js index 2e238924b1cfe..2f10acf7b3678 100644 --- a/utils/browser/test.js +++ b/utils/browser/test.js @@ -2,7 +2,8 @@ const path = require('path'); const fs = require('fs'); const puppeteer = require('../..'); const {TestServer} = require('../testserver/'); -const {TestRunner, Reporter, Matchers} = require('../testrunner/'); +const {TestRunner, Reporter} = require('../testrunner/'); +const expect = require('expect'); const puppeteerWebPath = path.join(__dirname, 'puppeteer-web.js'); if (!fs.existsSync(puppeteerWebPath)) @@ -13,7 +14,6 @@ const testRunner = new TestRunner(); const {describe, fdescribe, xdescribe} = testRunner; const {it, xit, fit} = testRunner; const {afterAll, beforeAll, afterEach, beforeEach} = testRunner; -const {expect} = new Matchers(); beforeAll(async state => { const assetsPath = path.join(__dirname, '..', '..', 'test', 'assets'); diff --git a/utils/doclint/check_public_api/test/test.js b/utils/doclint/check_public_api/test/test.js index 0f35ef362d47b..bdf1a67f7f7ff 100644 --- a/utils/doclint/check_public_api/test/test.js +++ b/utils/doclint/check_public_api/test/test.js @@ -22,7 +22,10 @@ const mdBuilder = require('../MDBuilder'); const jsBuilder = require('../JSBuilder'); const GoldenUtils = require('../../../../test/golden-utils'); -const {TestRunner, Reporter, Matchers} = require('../../../testrunner/'); +const testUtils = require('../../../../test/utils') + +const {TestRunner, Reporter} = require('../../../testrunner/'); +const expect = require('expect') const runner = new TestRunner(); const reporter = new Reporter(runner); @@ -60,9 +63,7 @@ runner.run(); async function testLint(state, test) { const dirPath = path.join(__dirname, test.name); - const {expect} = new Matchers({ - toBeGolden: GoldenUtils.compare.bind(null, dirPath, dirPath) - }); + testUtils.extendExpectWithToBeGolden(dirPath, dirPath) const mdSources = await Source.readdir(dirPath, '.md'); const jsSources = await Source.readdir(dirPath, '.js'); @@ -73,9 +74,8 @@ async function testLint(state, test) { async function testMDBuilder(state, test) { const dirPath = path.join(__dirname, test.name); - const {expect} = new Matchers({ - toBeGolden: GoldenUtils.compare.bind(null, dirPath, dirPath) - }); + testUtils.extendExpectWithToBeGolden(dirPath, dirPath) + const sources = await Source.readdir(dirPath, '.md'); const {documentation} = await mdBuilder(page, sources); expect(serialize(documentation)).toBeGolden('result.txt'); @@ -83,9 +83,9 @@ async function testMDBuilder(state, test) { async function testJSBuilder(state, test) { const dirPath = path.join(__dirname, test.name); - const {expect} = new Matchers({ - toBeGolden: GoldenUtils.compare.bind(null, dirPath, dirPath) - }); + testUtils.extendExpectWithToBeGolden(dirPath, dirPath) + + const sources = await Source.readdir(dirPath, '.js'); const {documentation} = await jsBuilder(sources); expect(serialize(documentation)).toBeGolden('result.txt'); @@ -124,4 +124,4 @@ function serializeType(type) { name: type.name, properties: type.properties.length ? type.properties.map(serializeMember) : undefined } -} \ No newline at end of file +} diff --git a/utils/doclint/preprocessor/test.js b/utils/doclint/preprocessor/test.js index 5d6be0ccec15e..d9ba46e000b80 100644 --- a/utils/doclint/preprocessor/test.js +++ b/utils/doclint/preprocessor/test.js @@ -16,14 +16,14 @@ const {runCommands, ensureReleasedAPILinks} = require('.'); const Source = require('../Source'); -const {TestRunner, Reporter, Matchers} = require('../../testrunner/'); +const expect = require('expect'); +const {TestRunner, Reporter} = require('../../testrunner/'); const runner = new TestRunner(); new Reporter(runner); const {describe, xdescribe, fdescribe} = runner; const {it, fit, xit} = runner; const {beforeAll, beforeEach, afterAll, afterEach} = runner; -const {expect} = new Matchers(); describe('ensureReleasedAPILinks', function() { it('should work with non-release version', function() { diff --git a/utils/testrunner/Matchers.js b/utils/testrunner/Matchers.js deleted file mode 100644 index 000751d43c83f..0000000000000 --- a/utils/testrunner/Matchers.js +++ /dev/null @@ -1,130 +0,0 @@ -/** - * Copyright 2017 Google Inc. All rights reserved. - * - * 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. - */ - -module.exports = class Matchers { - constructor(customMatchers = {}) { - this._matchers = {}; - Object.assign(this._matchers, DefaultMatchers); - Object.assign(this._matchers, customMatchers); - this.expect = this.expect.bind(this); - } - - addMatcher(name, matcher) { - this._matchers[name] = matcher; - } - - expect(value) { - return new Expect(value, this._matchers); - } -}; - -class Expect { - constructor(value, matchers) { - this.not = {}; - this.not.not = this; - for (const matcherName of Object.keys(matchers)) { - const matcher = matchers[matcherName]; - this[matcherName] = applyMatcher.bind(null, matcherName, matcher, false /* inverse */, value); - this.not[matcherName] = applyMatcher.bind(null, matcherName, matcher, true /* inverse */, value); - } - - function applyMatcher(matcherName, matcher, inverse, value, ...args) { - const result = matcher.call(null, value, ...args); - const message = `expect.${inverse ? 'not.' : ''}${matcherName} failed` + (result.message ? `: ${result.message}` : ''); - if (result.pass === inverse) - throw new Error(message); - } - } -} - -const DefaultMatchers = { - toBe: function(value, other, message) { - message = message || `${value} == ${other}`; - return { pass: value === other, message }; - }, - - toBeFalsy: function(value, message) { - message = message || `${value}`; - return { pass: !value, message }; - }, - - toBeTruthy: function(value, message) { - message = message || `${value}`; - return { pass: !!value, message }; - }, - - toBeGreaterThan: function(value, other, message) { - message = message || `${value} > ${other}`; - return { pass: value > other, message }; - }, - - toBeGreaterThanOrEqual: function(value, other, message) { - message = message || `${value} >= ${other}`; - return { pass: value >= other, message }; - }, - - toBeLessThan: function(value, other, message) { - message = message || `${value} < ${other}`; - return { pass: value < other, message }; - }, - - toBeLessThanOrEqual: function(value, other, message) { - message = message || `${value} <= ${other}`; - return { pass: value <= other, message }; - }, - - toBeNull: function(value, message) { - message = message || `${value} == null`; - return { pass: value === null, message }; - }, - - toContain: function(value, other, message) { - message = message || `${value} ⊇ ${other}`; - return { pass: value.includes(other), message }; - }, - - toEqual: function(value, other, message) { - const valueJson = stringify(value); - const otherJson = stringify(other); - message = message || `\n${valueJson} ≈ ${otherJson}`; - return { pass: valueJson === otherJson, message }; - }, - - toBeCloseTo: function(value, other, precision, message) { - return { - pass: Math.abs(value - other) < Math.pow(10, -precision), - message - }; - }, - - toBeInstanceOf: function(value, other, message) { - message = message || `${value.constructor.name} instanceof ${other.name}`; - return { pass: value instanceof other, message }; - }, -}; - -function stringify(value) { - function stabilize(key, object) { - if (typeof object !== 'object' || object === undefined || object === null) - return object; - const result = {}; - for (const key of Object.keys(object).sort()) - result[key] = object[key]; - return result; - } - - return JSON.stringify(stabilize(null, value), stabilize, 2); -} diff --git a/utils/testrunner/README.md b/utils/testrunner/README.md index b3ca075fdc89c..5ae83cf15d16f 100644 --- a/utils/testrunner/README.md +++ b/utils/testrunner/README.md @@ -7,6 +7,7 @@ This test runner is used internally by Puppeteer to test Puppeteer itself. - supports async/await - modular - well-isolated state per execution thread +- uses the `expect` library from `npm` for assertions. ### Installation @@ -23,15 +24,14 @@ node test.js ``` ```js -const {TestRunner, Reporter, Matchers} = require('@pptr/testrunner'); +const {TestRunner, Reporter} = require('@pptr/testrunner'); +const expect = require('expect') // Runner holds and runs all the tests const runner = new TestRunner({ parallel: 2, // run 2 parallel threads timeout: 1000, // setup timeout of 1 second per test }); -// Simple expect-like matchers -const {expect} = new Matchers(); // Extract jasmine-like DSL into the global namespace const {describe, xdescribe, fdescribe} = runner; diff --git a/utils/testrunner/examples/fail.js b/utils/testrunner/examples/fail.js index 6e2cd75ac419f..588f0a4feb451 100644 --- a/utils/testrunner/examples/fail.js +++ b/utils/testrunner/examples/fail.js @@ -14,11 +14,11 @@ * limitations under the License. */ -const {TestRunner, Reporter, Matchers} = require('..'); +const {TestRunner, Reporter} = require('..'); const runner = new TestRunner(); const reporter = new Reporter(runner); -const {expect} = new Matchers(); +const expect = require('expect'); const {describe, xdescribe, fdescribe} = runner; const {it, fit, xit} = runner; diff --git a/utils/testrunner/examples/hookfail.js b/utils/testrunner/examples/hookfail.js index 038f58c47f2fd..18922505fa3ba 100644 --- a/utils/testrunner/examples/hookfail.js +++ b/utils/testrunner/examples/hookfail.js @@ -14,11 +14,11 @@ * limitations under the License. */ -const {TestRunner, Reporter, Matchers} = require('..'); +const {TestRunner, Reporter} = require('..'); const runner = new TestRunner(); const reporter = new Reporter(runner); -const {expect} = new Matchers(); +const expect = require('expect'); const {describe, xdescribe, fdescribe} = runner; const {it, fit, xit} = runner; diff --git a/utils/testrunner/examples/hooktimeout.js b/utils/testrunner/examples/hooktimeout.js index 525f2b7c8adf0..cf57d6b00fc3c 100644 --- a/utils/testrunner/examples/hooktimeout.js +++ b/utils/testrunner/examples/hooktimeout.js @@ -14,11 +14,11 @@ * limitations under the License. */ -const {TestRunner, Reporter, Matchers} = require('..'); +const {TestRunner, Reporter} = require('..'); const runner = new TestRunner({ timeout: 100 }); const reporter = new Reporter(runner); -const {expect} = new Matchers(); +const expect = require('expect'); const {describe, xdescribe, fdescribe} = runner; const {it, fit, xit} = runner; diff --git a/utils/testrunner/index.js b/utils/testrunner/index.js index 998ea3d150033..3bd964619a341 100644 --- a/utils/testrunner/index.js +++ b/utils/testrunner/index.js @@ -16,6 +16,5 @@ const TestRunner = require('./TestRunner'); const Reporter = require('./Reporter'); -const Matchers = require('./Matchers'); -module.exports = { TestRunner, Reporter, Matchers }; +module.exports = { TestRunner, Reporter}; diff --git a/utils/testrunner/test/test.js b/utils/testrunner/test/test.js index d4f6b461a39ce..1e3eaf90d7ff1 100644 --- a/utils/testrunner/test/test.js +++ b/utils/testrunner/test/test.js @@ -1,7 +1,7 @@ -const {TestRunner, Matchers, Reporter} = require('..'); +const {TestRunner, Reporter} = require('..'); const testRunner = new TestRunner(); -const {expect} = new Matchers(); +const expect = require('expect'); require('./testrunner.spec.js').addTests({testRunner, expect});