From 5518bac29189b4ec99fc86ea25d45ae4a8d80e2f Mon Sep 17 00:00:00 2001 From: Jack Franklin Date: Thu, 30 Apr 2020 13:00:41 +0100 Subject: [PATCH] chore: update how we track coverage during unit tests (#5779) * chore: update how we track coverage during unit tests The old method of tracking coverage was causing issues. If a test failed on CI, that test's failure would be lost because the test failing would in turn cause the coverage to fail, but the `process.exit(1)` in the coverage code caused Mocha to not output anything useful. Instead the coverage checker now: * tracks the coverage in memory in a Map (this hasn't changed) * after all tests, writes that to disk in test/coverage.json (which is gitignored) * we then run a single Mocha test that asserts every method was called. This means if the test run fails, the build will fail and give the error about that test run, and that output won't be lost when the coverage then fails too. Co-authored-by: Mathias Bynens --- .gitignore | 1 + .travis.yml | 5 ++-- mocha-config/coverage-tests.js | 6 ++++ package.json | 3 +- test/assert-coverage-test.js | 24 ++++++++++++++++ test/coverage-utils.js | 50 ++++++++++++++++++++++++++-------- test/mocha-utils.js | 4 +-- travis/chromium.sh | 6 ---- 8 files changed, 76 insertions(+), 23 deletions(-) create mode 100644 mocha-config/coverage-tests.js create mode 100644 test/assert-coverage-test.js delete mode 100755 travis/chromium.sh diff --git a/.gitignore b/.gitignore index 929700b27190f..a1ac524ec1d69 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,4 @@ yarn.lock /utils/browser/puppeteer-web.js /index.d.ts /lib +test/coverage.json diff --git a/.travis.yml b/.travis.yml index b91387ce17f84..338d14fe063c8 100644 --- a/.travis.yml +++ b/.travis.yml @@ -29,13 +29,14 @@ jobs: # Runs unit tests on Linux + Chromium - node_js: "10.19.0" - name: 'Unit tests: Linux/Chromium' + name: 'Unit tests [with coverage]: Linux/Chromium' env: - CHROMIUM=true before_install: - PUPPETEER_PRODUCT=firefox npm install script: - - travis_retry npm run coverage + - travis_retry npm run unit-with-coverage + - npm run assert-unit-coverage # This bot runs all the extra checks that aren't the main Puppeteer unit tests - node_js: "10.19.0" diff --git a/mocha-config/coverage-tests.js b/mocha-config/coverage-tests.js new file mode 100644 index 0000000000000..6814f404cf092 --- /dev/null +++ b/mocha-config/coverage-tests.js @@ -0,0 +1,6 @@ +const base = require('./base'); + +module.exports = { + ...base, + spec: 'test/assert-coverage-test.js', +}; diff --git a/package.json b/package.json index 71e28c58c78f3..fa65219079ba2 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,8 @@ }, "scripts": { "unit": "mocha --config mocha-config/puppeteer-unit-tests.js", - "coverage": "cross-env COVERAGE=1 npm run unit", + "unit-with-coverage": "cross-env COVERAGE=1 npm run unit", + "assert-unit-coverage": "cross-env COVERAGE=1 mocha --config mocha-config/coverage-tests.js", "funit": "PUPPETEER_PRODUCT=firefox npm run unit", "debug-unit": "node --inspect-brk test/test.js", "test-doclint": "mocha --config mocha-config/doclint-tests.js", diff --git a/test/assert-coverage-test.js b/test/assert-coverage-test.js new file mode 100644 index 0000000000000..39b7b535c6e03 --- /dev/null +++ b/test/assert-coverage-test.js @@ -0,0 +1,24 @@ +const {describe, it} = require('mocha'); +const {getCoverageResults} = require('./coverage-utils'); +const expect = require('expect'); + +describe('API coverage test', () => { + it('calls every method', () => { + if (!process.env.COVERAGE) return; + + const coverageMap = getCoverageResults(); + const missingMethods = []; + for (const method of coverageMap.keys()) { + if (!coverageMap.get(method)) + missingMethods.push(method); + } + if (missingMethods.length) { + console.error('\nCoverage check failed: not all API methods called. See above output for list of missing methods.'); + console.error(missingMethods.join('\n')); + } + + // We know this will fail because we checked above + // but we need the actual test to fail. + expect(missingMethods.length).toEqual(0); + }); +}); diff --git a/test/coverage-utils.js b/test/coverage-utils.js index 34dda1b5cbb72..4f73a9e725a3c 100644 --- a/test/coverage-utils.js +++ b/test/coverage-utils.js @@ -26,6 +26,9 @@ * We run this when COVERAGE=1. */ +const path = require('path'); +const fs = require('fs'); + /** * @param {Map} apiCoverage * @param {Object} events @@ -59,9 +62,37 @@ function traceAPICoverage(apiCoverage, events, className, classType) { } } -module.exports = function() { +const coverageLocation = path.join(__dirname, 'coverage.json'); + +const clearOldCoverage = () => { + try { + fs.unlinkSync(coverageLocation); + } catch (error) { + // do nothing, the file didn't exist + } +}; +const writeCoverage = coverage => { + fs.writeFileSync(coverageLocation, JSON.stringify([...coverage.entries()])); +}; + +const getCoverageResults = () => { + let contents; + try { + contents = fs.readFileSync(coverageLocation, {encoding: 'utf8'}); + } catch (error) { + console.error('Warning: coverage file does not exist or is not readable.'); + } + + const coverageMap = new Map(JSON.parse(contents)); + return coverageMap; +}; + +const trackCoverage = () => { + clearOldCoverage(); const coverageMap = new Map(); + before(() => { + const api = require('../lib/api'); const events = require('../lib/Events'); for (const [className, classType] of Object.entries(api)) @@ -69,16 +100,11 @@ module.exports = function() { }); after(() => { - const missingMethods = []; - for (const method of coverageMap.keys()) { - if (!coverageMap.get(method)) - missingMethods.push(method); - } - if (missingMethods.length) { - console.error('\nCoverage check failed: not all API methods called. See above ouptut for list of missing methods.'); - console.error(Array.from(missingMethods).join('\n')); - process.exit(1); - } - console.log('\nAll Puppeteer API methods were called. Coverage test passed.\n'); + writeCoverage(coverageMap); }); }; + +module.exports = { + trackCoverage, + getCoverageResults +}; diff --git a/test/mocha-utils.js b/test/mocha-utils.js index bb66866d6f315..496b7e21b4b97 100644 --- a/test/mocha-utils.js +++ b/test/mocha-utils.js @@ -20,7 +20,7 @@ const fs = require('fs'); const os = require('os'); const puppeteer = require('../'); const utils = require('./utils'); -const assertCoverage = require('./coverage-utils'); +const {trackCoverage} = require('./coverage-utils'); const setupServer = async() => { const assetsPath = path.join(__dirname, 'assets'); @@ -115,7 +115,7 @@ global.describeChromeOnly = (...args) => { }; if (process.env.COVERAGE) - assertCoverage(); + trackCoverage(); console.log( `Running unit tests with: diff --git a/travis/chromium.sh b/travis/chromium.sh deleted file mode 100755 index 0e1058dae95c7..0000000000000 --- a/travis/chromium.sh +++ /dev/null @@ -1,6 +0,0 @@ -#!/usr/bin/env sh - -npm run lint && -npm run coverage && -npm run test-doclint && -npm run test-types