chore(test): restore api coverage checks (#3068)

* chore(test): restore api coverage checks

* try to catch the correct error on the bots

* try to trick github into taking the new code

* uninstall coverage

* dgozman comments

.github/workflows/tests.yml

@@ -37,7 +37,7 @@ jobs:
     # XVFB-RUN merges both STDOUT and STDERR, whereas we need only STDERR
     # Wrap `npm run` in a subshell to redirect STDERR to file.
     # Enable core dumps in the subshell.
-    - run: xvfb-run --auto-servernum -- bash -c "ulimit -c unlimited && npm run jest -- --testTimeout=30000"
+    - run: xvfb-run --auto-servernum -- bash -c "ulimit -c unlimited && npm run jest -- --testTimeout=30000 && npm run coverage"
       env:
         BROWSER: ${{ matrix.browser }}
         DEBUG: "pw:*,-pw:wrapped*"

package.json

@@ -28,7 +28,8 @@
     "generate-types": "node utils/generate_types/",
     "typecheck-tests": "tsc -p ./test/",
     "roll-browser": "node utils/roll_browser.js",
-    "jest": "jest"
+    "jest": "jest",
+    "coverage": "node test/jest/checkCoverage.js"
   },
   "author": {
     "name": "Microsoft Corporation"

test/jest/checkCoverage.js

@@ -0,0 +1,67 @@
+/**
+ * Copyright Microsoft Corporation. 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 path = require('path');
+const fs = require('fs');
+const {installCoverageHooks} = require('./coverage');
+
+const browserName = process.env.BROWSER || 'chromium';
+
+const api = new Set(installCoverageHooks(browserName).coverage.keys());
+
+// coverage exceptions
+
+if (browserName === 'chromium') {
+  // Sometimes we already have a background page while launching, before adding a listener.
+  api.delete('chromiumBrowserContext.emit("backgroundpage")');
+}
+
+if (browserName !== 'chromium') {
+  // we don't have CDPSession in non-chromium browsers
+  api.delete('cDPSession.send');
+  api.delete('cDPSession.detach');
+}
+
+// Some permissions tests are disabled in webkit. See permissions.jest.js
+if (browserName === 'webkit')
+  api.delete('browserContext.clearPermissions');
+
+const coverageDir = path.join(__dirname, '..', 'output-' + browserName, 'coverage');
+
+const coveredMethods = new Set();
+for (const file of getCoverageFiles(coverageDir)) {
+  for (const method of JSON.parse(fs.readFileSync(file, 'utf8')))
+    coveredMethods.add(method);
+}
+
+
+let success = true;
+for (const method of api) {
+  if (coveredMethods.has(method))
+    continue;
+  success = false;
+  console.log(`ERROR: Missing coverage for "${method}"`)
+}
+
+process.exit(success ? 0 : 1);
+
+function * getCoverageFiles(dir) {
+  for (const entry of fs.readdirSync(dir, {withFileTypes: true})) {
+    if (entry.isDirectory())
+      yield * getCoverageFiles(path.join(dir, entry.name))
+    else
+      yield path.join(dir, entry.name);
+  }
+}

test/jest/coverage.js

@@ -22,6 +22,7 @@
  * @param {!Object} classType
  */
 function traceAPICoverage(apiCoverage, events, className, classType) {
+  const uninstalls = [];
   className = className.substring(0, 1).toLowerCase() + className.substring(1);
   for (const methodName of Reflect.ownKeys(classType.prototype)) {
     const method = Reflect.get(classType.prototype, methodName);
@@ -34,6 +35,7 @@ function traceAPICoverage(apiCoverage, events, className, classType) {
     };
     Object.defineProperty(override, 'name', { writable: false, value: methodName });
     Reflect.set(classType.prototype, methodName, override);
+    uninstalls.push(() => Reflect.set(classType.prototype, methodName, method));
   }
 
   if (events[classType.name]) {
@@ -47,62 +49,67 @@ function traceAPICoverage(apiCoverage, events, className, classType) {
         apiCoverage.set(`${className}.emit(${JSON.stringify(event)})`, true);
       return method.call(this, event, ...args);
     });
+    uninstalls.push(() => Reflect.set(classType.prototype, 'emit', method));
   }
+
+  return () => uninstalls.forEach(u => u());
 }
 
-describe.skip(!process.env.COVERAGE)('**API COVERAGE**', () => {
+/**
+ * @param {string} browserName 
+ */
+function apiForBrowser(browserName) {
   const BROWSER_CONFIGS = [
     {
       name: 'Firefox',
-      events: require('../lib/events').Events,
-      missingCoverage: ['cDPSession.send', 'cDPSession.detach'],
+      events: require('../../lib/events').Events,
     },
     {
       name: 'WebKit',
-      events: require('../lib/events').Events,
-      missingCoverage: ['browserContext.clearPermissions', 'cDPSession.send', 'cDPSession.detach'],
+      events: require('../../lib/events').Events,
     },
     {
       name: 'Chromium',
       events: {
-        ...require('../lib/events').Events,
-        ...require('../lib/chromium/events').Events,
-      },
-      missingCoverage: [],
+        ...require('../../lib/events').Events,
+        ...require('../../lib/chromium/events').Events,
+      }
     },
   ];
-  const browserConfig = BROWSER_CONFIGS.find(config => config.name.toLowerCase() === browserType.name());
+  const browserConfig = BROWSER_CONFIGS.find(config => config.name.toLowerCase() === browserName);
   const events = browserConfig.events;
   // TODO: we should rethink our api.ts approach to ensure coverage and async stacks.
   const api = {
-    ...require('../lib/api'),
-    Browser: require('../lib/browser').BrowserBase,
-    BrowserContext: require('../lib/browserContext').BrowserContextBase,
+    ...require('../../lib/api'),
+    Browser: require('../../lib/browser').BrowserBase,
+    BrowserContext: require('../../lib/browserContext').BrowserContextBase,
   };
 
-  const coverage = new Map();
-  Object.keys(api).forEach(apiName => {
-    if (BROWSER_CONFIGS.some(config => apiName.startsWith(config.name)) && !apiName.startsWith(browserConfig.name))
-      return;
-    traceAPICoverage(coverage, events, apiName, api[apiName]);
+  const filteredKeys = Object.keys(api).filter(apiName => {
+    return !BROWSER_CONFIGS.some(config => apiName.startsWith(config.name)) || apiName.startsWith(browserConfig.name);
   });
+  const filteredAPI = {};
+  for (const key of filteredKeys)
+    filteredAPI[key] = api[key];
+  return {
+    api: filteredAPI,
+    events
+  }
+}
 
-  it('should call all API methods', () => {
-    const ignoredMethods = new Set(browserConfig.missingCoverage);
-    const missingMethods = [];
-    const extraIgnoredMethods = [];
-    for (const method of coverage.keys()) {
-      // Sometimes we already have a background page while launching, before adding a listener.
-      if (method === 'chromiumBrowserContext.emit("backgroundpage")')
-        continue;
-      if (!coverage.get(method) && !ignoredMethods.has(method))
-        missingMethods.push(method);
-      else if (coverage.get(method) && ignoredMethods.has(method))
-        extraIgnoredMethods.push(method);
-    }
-    if (extraIgnoredMethods.length)
-      throw new Error('Certain API Methods are called and should not be ignored: ' + extraIgnoredMethods.join(', '));
-    if (missingMethods.length)
-      throw new Error('Certain API Methods are not called: ' + missingMethods.join(', '));
-  });
-});
+/**
+ * @param {string} browserName 
+ */
+function installCoverageHooks(browserName) {
+  const uninstalls = [];
+  const {api, events} = apiForBrowser(browserName);
+  const coverage = new Map();
+  for (const [name, value] of Object.entries(api))
+    uninstalls.push(traceAPICoverage(coverage, events, name, value));
+  const uninstall = () => uninstalls.map(u => u());
+  return {coverage, uninstall};
+}
+
+module.exports = {
+  installCoverageHooks
+};

test/jest/playwrightEnvironment.js

@@ -18,9 +18,10 @@ const NodeEnvironment = require('jest-environment-node');
 const registerFixtures = require('./fixtures');
 const os = require('os');
 const path = require('path');
+const fs = require('fs');
 const platform = os.platform();
 const GoldenUtils = require('../../utils/testrunner/GoldenUtils');
-
+const {installCoverageHooks} = require('./coverage');
 const browserName = process.env.BROWSER || 'chromium';
 
 class PlaywrightEnvironment extends NodeEnvironment {
@@ -41,6 +42,7 @@ class PlaywrightEnvironment extends NodeEnvironment {
     testOptions.GOLDEN_DIR = path.join(__dirname, '..', 'golden-' + browserName);
     testOptions.OUTPUT_DIR = path.join(__dirname, '..', 'output-' + browserName);
     this.global.testOptions = testOptions;
+    this.testPath = context.testPath;
 
     this.global.registerFixture = (name, fn) => {
       this.fixturePool.registerFixture(name, 'test', fn);
@@ -53,11 +55,27 @@ class PlaywrightEnvironment extends NodeEnvironment {
 
   async setup() {
     await super.setup();
+    const {coverage, uninstall} = installCoverageHooks(browserName);
+    this.coverage = coverage;
+    this.uninstallCoverage = uninstall;
   }
 
   async teardown() {
     await this.fixturePool.teardownScope('worker');
     await super.teardown();
+    // If the setup throws an error, we don't want to override it
+    // with a useless error about this.coverage not existing.
+    if (!this.coverage)
+      return;
+    this.uninstallCoverage();
+    const testRoot = path.join(__dirname, '..');
+    const relativeTestPath = path.relative(testRoot, this.testPath);
+    const coveragePath = path.join(this.global.testOptions.OUTPUT_DIR, 'coverage', relativeTestPath + '.json');
+    const coverageJSON = [...this.coverage.keys()].filter(key => this.coverage.get(key));
+    await fs.promises.mkdir(path.dirname(coveragePath), { recursive: true });
+    await fs.promises.writeFile(coveragePath, JSON.stringify(coverageJSON, undefined, 2), 'utf8');
+    delete this.coverage;
+    delete this.uninstallCoverage;
   }
 
   runScript(script) {

test/test.config.js

@@ -76,7 +76,6 @@ module.exports = {
     },
     {
       files: [
-        './apicoverage.spec.js',
       ],
       environments: [],
     },