From 62da2366c65b335751896afbb0206f23c61436f1 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Fri, 25 Jan 2019 23:21:14 -0500 Subject: [PATCH] chore: introduce //lib/api.js (#3835) Introduce `//lib/api.js` that declares a list of publicly exposed classes. The `//lib/api.js` list superceedes dynamic `helper.tracePublicAPI()` calls and is used in the following places: - [ASYNC STACKS]: generate "async stacks" for publicy exposed API in `//index.js` - [COVERAGE]: move coverage support from `//lib/helper` to `//test/utils` - [DOCLINT]: get rid of 'exluded classes' hardcoded list This will help us to re-use our coverage and doclint infrastructure for Puppeteer-Firefox. Drive-By: it turns out we didn't run coverage for `SecurityDetails` class, so we lack coverage for a few methods there. These are excluded for now, sanity tests will be added in a follow-up. --- index.js | 10 +++++ lib/Accessibility.js | 2 - lib/Browser.js | 3 -- lib/Connection.js | 4 +- lib/Coverage.js | 1 - lib/Dialog.js | 3 +- lib/ExecutionContext.js | 2 - lib/FrameManager.js | 1 - lib/Input.js | 5 +-- lib/JSHandle.js | 3 -- lib/NetworkManager.js | 5 +-- lib/Page.js | 3 +- lib/Puppeteer.js | 2 - lib/Target.js | 19 +++++++-- lib/Tracing.js | 1 - lib/Worker.js | 3 +- lib/api.js | 42 ++++++++++++++++++++ lib/helper.js | 51 +------------------------ test/test.js | 30 ++++++--------- test/utils.js | 44 +++++++++++++++++++++ utils/doclint/check_public_api/index.js | 33 +++++----------- 21 files changed, 139 insertions(+), 128 deletions(-) create mode 100644 lib/api.js diff --git a/index.js b/index.js index 133f0551..e7143fb3 100644 --- a/index.js +++ b/index.js @@ -21,6 +21,16 @@ try { asyncawait = false; } +if (asyncawait) { + const {helper} = require('./lib/helper'); + const api = require('./lib/api'); + for (const className in api) { + // Puppeteer-web excludes certain classes from bundle, e.g. BrowserFetcher. + if (typeof api[className] === 'function') + helper.installAsyncStackHooks(api[className]); + } +} + // If node does not support async await, use the compiled version. const Puppeteer = asyncawait ? require('./lib/Puppeteer') : require('./node6/lib/Puppeteer'); const packageJson = require('./package.json'); diff --git a/lib/Accessibility.js b/lib/Accessibility.js index 3bb7deb4..f65642bf 100644 --- a/lib/Accessibility.js +++ b/lib/Accessibility.js @@ -13,7 +13,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -const {helper} = require('./helper'); /** * @typedef {Object} SerializedAXNode @@ -390,4 +389,3 @@ class AXNode { } module.exports = {Accessibility}; -helper.tracePublicAPI(Accessibility); diff --git a/lib/Browser.js b/lib/Browser.js index eaa1584a..8d2a5929 100644 --- a/lib/Browser.js +++ b/lib/Browser.js @@ -373,7 +373,4 @@ class BrowserContext extends EventEmitter { } } -helper.tracePublicAPI(BrowserContext); -helper.tracePublicAPI(Browser); - module.exports = {Browser, BrowserContext}; diff --git a/lib/Connection.js b/lib/Connection.js index 616974a6..bd9be3d4 100644 --- a/lib/Connection.js +++ b/lib/Connection.js @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -const {helper, assert} = require('./helper'); +const {assert} = require('./helper'); const {Events} = require('./Events'); const debugProtocol = require('debug')('puppeteer:protocol'); const EventEmitter = require('events'); @@ -216,8 +216,6 @@ class CDPSession extends EventEmitter { } } -helper.tracePublicAPI(CDPSession); - /** * @param {!Error} error * @param {string} method diff --git a/lib/Coverage.js b/lib/Coverage.js index f4e3d31f..5f88c10e 100644 --- a/lib/Coverage.js +++ b/lib/Coverage.js @@ -64,7 +64,6 @@ class Coverage { } module.exports = {Coverage}; -helper.tracePublicAPI(Coverage); class JSCoverage { /** diff --git a/lib/Dialog.js b/lib/Dialog.js index 89e3092c..cbdc2719 100644 --- a/lib/Dialog.js +++ b/lib/Dialog.js @@ -14,7 +14,7 @@ * limitations under the License. */ -const {helper, assert} = require('./helper'); +const {assert} = require('./helper'); class Dialog { /** @@ -81,4 +81,3 @@ Dialog.Type = { }; module.exports = {Dialog}; -helper.tracePublicAPI(Dialog); diff --git a/lib/ExecutionContext.js b/lib/ExecutionContext.js index b832039d..c17a5de1 100644 --- a/lib/ExecutionContext.js +++ b/lib/ExecutionContext.js @@ -175,6 +175,4 @@ class ExecutionContext { } } -helper.tracePublicAPI(ExecutionContext); - module.exports = {ExecutionContext, EVALUATION_SCRIPT_URL}; diff --git a/lib/FrameManager.js b/lib/FrameManager.js index 1f5d967e..513dbf9e 100644 --- a/lib/FrameManager.js +++ b/lib/FrameManager.js @@ -680,7 +680,6 @@ class Frame { this._parentFrame = null; } } -helper.tracePublicAPI(Frame); function assertNoLegacyNavigationOptions(options) { assert(options['networkIdleTimeout'] === undefined, 'ERROR: networkIdleTimeout option is no longer supported.'); diff --git a/lib/Input.js b/lib/Input.js index b4c02f95..a0789b1e 100644 --- a/lib/Input.js +++ b/lib/Input.js @@ -14,7 +14,7 @@ * limitations under the License. */ -const {helper, assert} = require('./helper'); +const {assert} = require('./helper'); const keyDefinitions = require('./USKeyboardLayout'); /** @@ -302,6 +302,3 @@ class Touchscreen { } module.exports = { Keyboard, Mouse, Touchscreen}; -helper.tracePublicAPI(Keyboard); -helper.tracePublicAPI(Mouse); -helper.tracePublicAPI(Touchscreen); diff --git a/lib/JSHandle.js b/lib/JSHandle.js index 226b1ed0..2190a9d3 100644 --- a/lib/JSHandle.js +++ b/lib/JSHandle.js @@ -122,8 +122,6 @@ class JSHandle { } } -helper.tracePublicAPI(JSHandle); - class ElementHandle extends JSHandle { /** * @param {!Puppeteer.ExecutionContext} context @@ -507,5 +505,4 @@ function computeQuadArea(quad) { * @property {number} height */ -helper.tracePublicAPI(ElementHandle); module.exports = {createJSHandle, JSHandle, ElementHandle}; diff --git a/lib/NetworkManager.js b/lib/NetworkManager.js index 3e63ce88..79263cd9 100644 --- a/lib/NetworkManager.js +++ b/lib/NetworkManager.js @@ -507,8 +507,6 @@ const errorReasons = { 'failed': 'Failed', }; -helper.tracePublicAPI(Request); - class Response { /** * @param {!Puppeteer.CDPSession} client @@ -649,7 +647,6 @@ class Response { return this._request.frame(); } } -helper.tracePublicAPI(Response); const IGNORED_HEADERS = new Set(['accept', 'referer', 'x-devtools-emulate-network-conditions-client-id', 'cookie', 'origin', 'content-type', 'intervention']); @@ -798,4 +795,4 @@ const statusTexts = { '511': 'Network Authentication Required', }; -module.exports = {Request, Response, NetworkManager}; +module.exports = {Request, Response, NetworkManager, SecurityDetails}; diff --git a/lib/Page.js b/lib/Page.js index 1bd7334b..2e588efc 100644 --- a/lib/Page.js +++ b/lib/Page.js @@ -1270,5 +1270,4 @@ class ConsoleMessage { } -module.exports = {Page}; -helper.tracePublicAPI(Page); +module.exports = {Page, ConsoleMessage}; diff --git a/lib/Puppeteer.js b/lib/Puppeteer.js index 314aff6f..cf69b027 100644 --- a/lib/Puppeteer.js +++ b/lib/Puppeteer.js @@ -13,7 +13,6 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -const {helper} = require('./helper'); const Launcher = require('./Launcher'); const BrowserFetcher = require('./BrowserFetcher'); @@ -68,4 +67,3 @@ module.exports = class { } }; -helper.tracePublicAPI(module.exports, 'Puppeteer'); diff --git a/lib/Target.js b/lib/Target.js index b42ffe90..e1e4ae4a 100644 --- a/lib/Target.js +++ b/lib/Target.js @@ -1,6 +1,21 @@ +/** + * Copyright 2019 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. + */ + const {Events} = require('./Events'); const {Page} = require('./Page'); -const {helper} = require('./helper'); class Target { /** @@ -113,6 +128,4 @@ class Target { } } -helper.tracePublicAPI(Target); - module.exports = {Target}; diff --git a/lib/Tracing.js b/lib/Tracing.js index d8c8ac7f..37fc8410 100644 --- a/lib/Tracing.js +++ b/lib/Tracing.js @@ -101,6 +101,5 @@ class Tracing { } } } -helper.tracePublicAPI(Tracing); module.exports = Tracing; diff --git a/lib/Worker.js b/lib/Worker.js index 1161e6b8..76611652 100644 --- a/lib/Worker.js +++ b/lib/Worker.js @@ -14,7 +14,7 @@ * limitations under the License. */ const EventEmitter = require('events'); -const {helper, debugError} = require('./helper'); +const {debugError} = require('./helper'); const {ExecutionContext} = require('./ExecutionContext'); const {JSHandle} = require('./JSHandle'); @@ -78,4 +78,3 @@ class Worker extends EventEmitter { } module.exports = {Worker}; -helper.tracePublicAPI(Worker); diff --git a/lib/api.js b/lib/api.js new file mode 100644 index 00000000..135a69ea --- /dev/null +++ b/lib/api.js @@ -0,0 +1,42 @@ +/** + * Copyright 2019 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 = { + Accessibility: require('./Accessibility').Accessibility, + Browser: require('./Browser').Browser, + BrowserContext: require('./Browser').BrowserContext, + BrowserFetcher: require('./BrowserFetcher'), + CDPSession: require('./Connection').CDPSession, + ConsoleMessage: require('./Page').ConsoleMessage, + Coverage: require('./Coverage').Coverage, + Dialog: require('./Dialog').Dialog, + ElementHandle: require('./JSHandle').ElementHandle, + ExecutionContext: require('./ExecutionContext').ExecutionContext, + Frame: require('./FrameManager').Frame, + JSHandle: require('./JSHandle').JSHandle, + Keyboard: require('./Input').Keyboard, + Mouse: require('./Input').Mouse, + Page: require('./Page').Page, + Puppeteer: require('./Puppeteer'), + Request: require('./NetworkManager').Request, + Response: require('./NetworkManager').Response, + SecurityDetails: require('./NetworkManager').SecurityDetails, + Target: require('./Target').Target, + TimeoutError: require('./Errors').TimeoutError, + Touchscreen: require('./Input').Touchscreen, + Tracing: require('./Tracing'), + Worker: require('./Worker').Worker, +}; diff --git a/lib/helper.js b/lib/helper.js index 0bc94199..ee1055c7 100644 --- a/lib/helper.js +++ b/lib/helper.js @@ -16,41 +16,6 @@ const {TimeoutError} = require('./Errors'); const debugError = require('debug')(`puppeteer:error`); -/** @type {?Map} */ -let apiCoverage = null; - -/** - * @param {!Object} classType - * @param {string=} publicName - */ -function traceAPICoverage(classType, publicName) { - if (!apiCoverage) - return; - - let className = publicName || classType.prototype.constructor.name; - className = className.substring(0, 1).toLowerCase() + className.substring(1); - for (const methodName of Reflect.ownKeys(classType.prototype)) { - const method = Reflect.get(classType.prototype, methodName); - if (methodName === 'constructor' || typeof methodName !== 'string' || methodName.startsWith('_') || typeof method !== 'function') - continue; - apiCoverage.set(`${className}.${methodName}`, false); - Reflect.set(classType.prototype, methodName, function(...args) { - apiCoverage.set(`${className}.${methodName}`, true); - return method.call(this, ...args); - }); - } - - if (classType.Events) { - for (const event of Object.values(classType.Events)) - apiCoverage.set(`${className}.emit(${JSON.stringify(event)})`, false); - const method = Reflect.get(classType.prototype, 'emit'); - Reflect.set(classType.prototype, 'emit', function(event, ...args) { - if (this.listenerCount(event)) - apiCoverage.set(`${className}.emit(${JSON.stringify(event)})`, true); - return method.call(this, event, ...args); - }); - } -} class Helper { /** @@ -133,9 +98,8 @@ class Helper { /** * @param {!Object} classType - * @param {string=} publicName */ - static tracePublicAPI(classType, publicName) { + static installAsyncStackHooks(classType) { for (const methodName of Reflect.ownKeys(classType.prototype)) { const method = Reflect.get(classType.prototype, methodName); if (methodName === 'constructor' || typeof methodName !== 'string' || methodName.startsWith('_') || typeof method !== 'function' || method.constructor.name !== 'AsyncFunction') @@ -151,8 +115,6 @@ class Helper { }); }); } - - traceAPICoverage(classType, publicName); } /** @@ -175,17 +137,6 @@ class Helper { listeners.splice(0, listeners.length); } - /** - * @return {?Map} - */ - static publicAPICoverage() { - return apiCoverage; - } - - static recordPublicAPICoverage() { - apiCoverage = new Map(); - } - /** * @param {!Object} obj * @return {boolean} diff --git a/test/test.js b/test/test.js index 392cfec1..10b1a6a8 100644 --- a/test/test.js +++ b/test/test.js @@ -75,32 +75,24 @@ beforeEach(async({server, httpsServer}) => { httpsServer.reset(); }); -describe('Chromium', () => { - const {helper} = require('../lib/helper'); - if (process.env.COVERAGE) - helper.recordPublicAPICoverage(); +const CHROMIUM_NO_COVERAGE = new Set([ + 'page.bringToFront', + 'securityDetails.subjectName', + 'securityDetails.issuer', + 'securityDetails.validFrom', + 'securityDetails.validTo', + ...(headless ? [] : ['page.pdf']), +]); +describe('Chromium', () => { require('./puppeteer.spec.js').addTests({ product: 'Chromium', puppeteer: utils.requireRoot('index'), defaultBrowserOptions, testRunner, }); - if (process.env.COVERAGE) { - describe('COVERAGE', function() { - const coverage = helper.publicAPICoverage(); - const disabled = new Set(['page.bringToFront']); - if (!headless) - disabled.add('page.pdf'); - - for (const method of coverage.keys()) { - (disabled.has(method) ? xit : it)(`public api '${method}' should be called`, async({page, server}) => { - if (!coverage.get(method)) - throw new Error('NOT CALLED!'); - }); - } - }); - } + if (process.env.COVERAGE) + utils.recordAPICoverage(testRunner, require('../lib/api'), CHROMIUM_NO_COVERAGE); }); if (process.env.CI && testRunner.hasFocusedTestsOrSuites()) { diff --git a/test/utils.js b/test/utils.js index 05e1c982..debe12cd 100644 --- a/test/utils.js +++ b/test/utils.js @@ -18,7 +18,51 @@ const fs = require('fs'); const path = require('path'); const PROJECT_ROOT = fs.existsSync(path.join(__dirname, '..', 'package.json')) ? path.join(__dirname, '..') : path.join(__dirname, '..', '..'); +/** + * @param {Map} apiCoverage + * @param {string} className + * @param {!Object} classType + */ +function traceAPICoverage(apiCoverage, className, classType) { + className = className.substring(0, 1).toLowerCase() + className.substring(1); + for (const methodName of Reflect.ownKeys(classType.prototype)) { + const method = Reflect.get(classType.prototype, methodName); + if (methodName === 'constructor' || typeof methodName !== 'string' || methodName.startsWith('_') || typeof method !== 'function') + continue; + apiCoverage.set(`${className}.${methodName}`, false); + Reflect.set(classType.prototype, methodName, function(...args) { + apiCoverage.set(`${className}.${methodName}`, true); + return method.call(this, ...args); + }); + } + + if (classType.Events) { + for (const event of Object.values(classType.Events)) + apiCoverage.set(`${className}.emit(${JSON.stringify(event)})`, false); + const method = Reflect.get(classType.prototype, 'emit'); + Reflect.set(classType.prototype, 'emit', function(event, ...args) { + if (this.listenerCount(event)) + apiCoverage.set(`${className}.emit(${JSON.stringify(event)})`, true); + return method.call(this, event, ...args); + }); + } +} + const utils = module.exports = { + recordAPICoverage: function(testRunner, api, disabled) { + const coverage = new Map(); + for (const [className, classType] of Object.entries(api)) + traceAPICoverage(coverage, className, classType); + testRunner.describe('COVERAGE', () => { + for (const method of coverage.keys()) { + (disabled.has(method) ? testRunner.xit : testRunner.it)(`public api '${method}' should be called`, async({page, server}) => { + if (!coverage.get(method)) + throw new Error('NOT CALLED!'); + }); + } + }); + }, + /** * @return {string} */ diff --git a/utils/doclint/check_public_api/index.js b/utils/doclint/check_public_api/index.js index 0deb9019..94861aae 100644 --- a/utils/doclint/check_public_api/index.js +++ b/utils/doclint/check_public_api/index.js @@ -19,26 +19,6 @@ const mdBuilder = require('./MDBuilder'); const Documentation = require('./Documentation'); const Message = require('../Message'); -const EXCLUDE_CLASSES = new Set([ - 'AXNode', - 'CSSCoverage', - 'Connection', - 'CustomError', - 'DOMWorld', - 'EmulationManager', - 'FrameManager', - 'JSCoverage', - 'Helper', - 'Launcher', - 'Multimap', - 'LifecycleWatcher', - 'NetworkManager', - 'PipeTransport', - 'TaskQueue', - 'WaitTask', - 'WebSocketTransport', -]); - const EXCLUDE_PROPERTIES = new Set([ 'Browser.create', 'Headers.fromPayload', @@ -55,7 +35,7 @@ const EXCLUDE_PROPERTIES = new Set([ module.exports = async function lint(page, mdSources, jsSources) { const mdResult = await mdBuilder(page, mdSources); const jsResult = await jsBuilder(jsSources); - const jsDocumentation = filterJSDocumentation(jsResult.documentation); + const jsDocumentation = filterJSDocumentation(jsSources, jsResult.documentation); const mdDocumentation = mdResult.documentation; const jsErrors = jsResult.errors; @@ -126,14 +106,19 @@ function checkSorting(doc) { } /** + * @param {!Array} jsSources * @param {!Documentation} jsDocumentation * @return {!Documentation} */ -function filterJSDocumentation(jsDocumentation) { - // Filter classes and methods. +function filterJSDocumentation(jsSources, jsDocumentation) { + const apijs = jsSources.find(source => source.name() === 'api.js'); + let includedClasses = null; + if (apijs) + includedClasses = new Set(Object.keys(require(apijs.filePath()))); + // Filter private classes and methods. const classes = []; for (const cls of jsDocumentation.classesArray) { - if (EXCLUDE_CLASSES.has(cls.name)) + if (includedClasses && !includedClasses.has(cls.name)) continue; const members = cls.membersArray.filter(member => !EXCLUDE_PROPERTIES.has(`${cls.name}.${member.name}`)); classes.push(new Documentation.Class(cls.name, members));