Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tests: start replacing jest with mocha, starting with unit-cli #13568

Closed
wants to merge 39 commits into from
Closed
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
e15d486
initial. external snapshots
connorjclark Nov 17, 2021
b57b6d6
inline snapshot
connorjclark Nov 17, 2021
5f1f6be
one snapshot state per test file
connorjclark Nov 18, 2021
2bd3231
maintain map of snapshot states by test file
connorjclark Nov 18, 2021
f92351a
simplify
connorjclark Nov 18, 2021
e10c0ac
use testdouble to mock modules
connorjclark Nov 23, 2021
2f7f891
yarn unit-cli-mocha mostly works
connorjclark Nov 23, 2021
c4d591b
comment
connorjclark Nov 23, 2021
d4d4830
Merge remote-tracking branch 'origin/master' into mocha
connorjclark Jan 13, 2022
5b2b8b0
fix beforeAll/afterAll timeouts
connorjclark Jan 13, 2022
5b03510
fix config test
connorjclark Jan 13, 2022
b553be9
update
connorjclark Jan 13, 2022
e7eb722
fix types
connorjclark Jan 13, 2022
8f8eaea
update
connorjclark Jan 13, 2022
b084502
ci
connorjclark Jan 13, 2022
939e1d0
todo, rm log
connorjclark Jan 13, 2022
dbb8e60
comment
connorjclark Jan 13, 2022
2b3e56b
patch mocha globals
connorjclark Jan 13, 2022
4f96ce4
dev
connorjclark Jan 13, 2022
d00d94a
typecheck
connorjclark Jan 13, 2022
071ae8e
update
connorjclark Jan 13, 2022
b311566
update
connorjclark Jan 13, 2022
71198a5
Merge remote-tracking branch 'origin/master' into mocha
connorjclark Feb 7, 2022
0cebb33
print instruction on snapshot failure
connorjclark Feb 7, 2022
7f0a250
run-tests.js
connorjclark Feb 7, 2022
1092625
update snapshhots
connorjclark Feb 7, 2022
9bea135
Merge remote-tracking branch 'origin/master' into mocha
connorjclark May 17, 2022
7e3912d
update testdouble and rm hacky workaround
connorjclark May 17, 2022
e4d3a62
--testMatch
connorjclark May 17, 2022
b3c9ecf
update
connorjclark May 17, 2022
39eef45
gitignore
connorjclark May 17, 2022
4cff085
mochaPassThruArgs
connorjclark May 17, 2022
8eedc78
ok
connorjclark May 17, 2022
6656a0b
--parallel
connorjclark May 17, 2022
74336c5
yml
connorjclark May 17, 2022
c8c69a0
lint
connorjclark May 17, 2022
b497806
add custom expect matchers to mocha
connorjclark May 17, 2022
8ab1d52
filter based on abs path
connorjclark May 17, 2022
bc1c959
Merge remote-tracking branch 'origin/master' into mocha
connorjclark May 21, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions .github/workflows/unit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -84,8 +84,9 @@ jobs:
- run: yarn install --frozen-lockfile --network-timeout 1000000
- run: yarn build-report

- name: yarn unit-cli
run: yarn unit-cli
# - name: yarn unit-cli
# run: yarn unit-cli
- run: yarn unit-cli-mocha
- run: yarn diff:sample-json

# Fail if any changes were written to any source files or generated untracked files (ex, from -GA).
Expand Down
1 change: 0 additions & 1 deletion jest.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ module.exports = {
testEnvironment: 'node',
testMatch: [
'**/lighthouse-core/**/*-test.js',
'**/lighthouse-cli/**/*-test.js',
'**/report/**/*-test.js',
'**/lighthouse-core/test/fraggle-rock/**/*-test-pptr.js',
'**/treemap/**/*-test.js',
Expand Down
621 changes: 315 additions & 306 deletions lighthouse-cli/cli-flags.js

Large diffs are not rendered by default.

61 changes: 28 additions & 33 deletions lighthouse-cli/test/cli/bin-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,47 +9,42 @@

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eslint-env mocha now?

import fs from 'fs';

import {jest} from '@jest/globals';
import * as td from 'testdouble';
import jestMock from 'jest-mock';

import {LH_ROOT} from '../../../root.js';
import {LH_ROOT, readJson} from '../../../root.js';

const mockRunLighthouse = jest.fn();

jest.unstable_mockModule('../../run.js', () => {
return {runLighthouse: mockRunLighthouse};
});

const mockGetFlags = jest.fn();
jest.unstable_mockModule('../../cli-flags.js', () => {
return {getFlags: mockGetFlags};
});

const mockAskPermission = jest.fn();
jest.unstable_mockModule('../../sentry-prompt.js', () => {
return {askPermission: mockAskPermission};
});

const mockSentryInit = jest.fn();
jest.mock('../../../lighthouse-core/lib/sentry.js', () => {
return {init: mockSentryInit};
});

const mockLoggerSetLevel = jest.fn();
jest.unstable_mockModule('lighthouse-logger', () => {
return {default: {setLevel: mockLoggerSetLevel}};
});

const mockNotify = jest.fn();
jest.unstable_mockModule('update-notifier', () => {
return {default: () => ({notify: mockNotify})};
});
const mockRunLighthouse = jestMock.fn();
const mockGetFlags = jestMock.fn();
const mockAskPermission = jestMock.fn();
const mockSentryInit = jestMock.fn();
const mockLoggerSetLevel = jestMock.fn();
const mockNotify = jestMock.fn();

/** @type {import('../../bin.js')} */
let bin;
beforeAll(async () => {
await td.replaceEsm('../../run.js', {
runLighthouse: mockRunLighthouse,
});
await td.replaceEsm('../../cli-flags.js', {
getFlags: mockGetFlags,
});
await td.replaceEsm('../../sentry-prompt.js', {
askPermission: mockAskPermission,
});
await td.replace('../../../lighthouse-core/lib/sentry.js', {
init: mockSentryInit,
});
await td.replaceEsm('lighthouse-logger', undefined, {setLevel: mockLoggerSetLevel});
await td.replaceEsm('update-notifier', undefined, () => ({notify: mockNotify}));
bin = await import('../../bin.js');
});

afterAll(async () => {
await td.reset();
});

/** @type {LH.CliFlags} */
let cliFlags;

Expand Down Expand Up @@ -186,7 +181,7 @@ describe('CLI bin', function() {
await bin.begin();

expect(getRunLighthouseArgs()[1]).toMatchObject({
precomputedLanternData: (await import(lanternDataFile)).default,
precomputedLanternData: readJson(lanternDataFile),
precomputedLanternDataPath: lanternDataFile,
});
});
Expand Down
11 changes: 4 additions & 7 deletions lighthouse-cli/test/cli/cli-flags-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,21 @@
import {strict as assert} from 'assert';
import fs from 'fs';

import yargs from 'yargs';

import {getFlags} from '../../cli-flags.js';
import {getFlags, getYargsParser} from '../../cli-flags.js';
import {LH_ROOT} from '../../../root.js';

describe('CLI flags', function() {
it('all options should have descriptions', () => {
getFlags('chrome://version');
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with jest import yargs from 'yargs here and in the test file results in yargs.getGroups() working in the test file. That function is added to the module dynamically after being called, which is why this test did this seemingly no-op call to getFlags.

But, that property is only added to the module object in jest because ... reasons? The modules aren't really es modules, I guess, and es module objects are read-only, IIRC.

Anyhow, the getGroups method is also added to the yargs parser object, so to get to that I split up the implementation across two functions.


const parser = getYargsParser();
// @ts-expect-error - getGroups is private
const optionGroups = yargs.getGroups();
const optionGroups = parser.getGroups();
/** @type {string[]} */
const allOptions = [];
Object.keys(optionGroups).forEach(key => {
allOptions.push(...optionGroups[key]);
});
// @ts-expect-error - getUsageInstance is private
const optionsWithDescriptions = Object.keys(yargs.getUsageInstance().getDescriptions());
const optionsWithDescriptions = Object.keys(parser.getUsageInstance().getDescriptions());

allOptions.forEach(opt => {
assert.ok(optionsWithDescriptions.includes(opt), `cli option '${opt}' has no description`);
Expand Down
4 changes: 3 additions & 1 deletion lighthouse-cli/test/cli/printer-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,9 @@ import {strict as assert} from 'assert';
import fs from 'fs';

import * as Printer from '../../printer.js';
import sampleResults from '../../../lighthouse-core/test/results/sample_v2.json';
import {readJson} from '../../../root.js';

const sampleResults = readJson('lighthouse-core/test/results/sample_v2.json');

describe('Printer', () => {
it('accepts valid output paths', () => {
Expand Down
8 changes: 6 additions & 2 deletions lighthouse-cli/test/cli/run-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ describe('CLI run', function() {
let fileResults;

beforeAll(async () => {
fs.symlinkSync(
`${LH_ROOT}/lighthouse-core/test/fixtures/config-plugins/lighthouse-plugin-simple`,
`${LH_ROOT}/lighthouse-plugin-simple`
);

const url = 'http://localhost:10200/dobetterweb/dbw_tester.html';
// eslint-disable-next-line max-len
const samplev2ArtifactsPath = LH_ROOT + '/lighthouse-core/test/results/artifacts/';
Expand All @@ -41,8 +46,6 @@ describe('CLI run', function() {
const flags = getFlags([
'--output=json',
`--output-path=${filename}`,
// Jest allows us to resolve this module with no setup.
// https://github.com/GoogleChrome/lighthouse/pull/13045#discussion_r708690607
'--plugins=lighthouse-plugin-simple',
// Use sample artifacts to avoid gathering during a unit test.
`--audit-mode=${samplev2ArtifactsPath}`,
Expand All @@ -62,6 +65,7 @@ describe('CLI run', function() {

afterAll(() => {
fs.unlinkSync(filename);
fs.unlinkSync(`${LH_ROOT}/lighthouse-plugin-simple`);
});

it('returns results that match the saved results', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ describe('ExecutionContext', () => {
expect(executionContext.getContextId()).toEqual(undefined);
});

it.todo('should cache native objects in page');
it('TODO: should cache native objects in page');
});

describe('.evaluateAsync', () => {
Expand Down
148 changes: 148 additions & 0 deletions lighthouse-core/test/mocha-setup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
/**
* @license Copyright 2021 The Lighthouse Authors. 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.
*/
'use strict';

/**
* @fileoverview
* 1) sets global.expect
* 2) configures the mocha test runner to use jest-snapshot
*/

const path = require('path');
const expect = require('expect');
const {SnapshotState, toMatchSnapshot, toMatchInlineSnapshot} = require('jest-snapshot');

/** @type {Map<string, SnapshotState['prototype']>} */
const snapshotStatesByTestFile = new Map();

/**
* @param {string} testFile
*/
function getSnapshotState(testFile) {
// For every test file, persist the same snapshot state object so there is
// not a read/write per snapshot access/change, but one per file.
let snapshotState = snapshotStatesByTestFile.get(testFile);
if (snapshotState) return snapshotState;

const snapshotDir = path.join(path.dirname(testFile), '__snapshots__');
const snapshotFile = path.join(snapshotDir, path.basename(testFile) + '.snap');
snapshotState = new SnapshotState(snapshotFile, {
updateSnapshot: process.env.SNAPSHOT_UPDATE ? 'all' : 'new',
prettierPath: '',
snapshotFormat: {},
});
snapshotStatesByTestFile.set(testFile, snapshotState);
return snapshotState;
}

/**
* @param {Mocha.Test} test
* @return {string}
*/
function makeTestTitle(test) {
/** @type {Mocha.Test | Mocha.Suite} */
let next = test;
const title = [];

for (;;) {
if (!next.parent) {
break;
}

title.push(next.title);
next = next.parent;
}

return title.reverse().join(' ');
}

expect.extend({
/**
* @param {any} actual
*/
toMatchSnapshot(actual) {
const test = mochaCurrentTest;
if (!test.file) throw new Error('unexpected value');

const title = makeTestTitle(test);
const snapshotState = getSnapshotState(test.file);
/** @type {import('jest-snapshot/build/types').Context} */
// @ts-expect-error - this is enough for snapshots to work.
const context = {snapshotState, currentTestName: title};
const matcher = toMatchSnapshot.bind(context);

return matcher(actual);
},
/**
* @param {any} actual
* @param {any} expected
*/
toMatchInlineSnapshot(actual, expected) {
const test = mochaCurrentTest;
if (!test.file) throw new Error('unexpected value');

const title = makeTestTitle(test);
const snapshotState = getSnapshotState(test.file);
/** @type {import('jest-snapshot/build/types').Context} */
// @ts-expect-error - this is enough for snapshots to work.
const context = {snapshotState, currentTestName: title};
const matcher = toMatchInlineSnapshot.bind(context);

return matcher(actual, expected);
},
});

// @ts-expect-error
global.expect = expect;

/**
* @param {Mocha.HookFunction} mochaFn
* @return {jest.Lifecycle}
*/
const makeFn = (mochaFn) => (fn, timeout) => {
mochaFn(function() {
// eslint-disable-next-line no-invalid-this
if (timeout !== undefined) this.timeout(timeout);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no timeout parameter for mocha hook functions, but there is for jest lifecycle functions.


/** @type {jest.DoneCallback} */
const cb = () => {};
cb.fail = (error) => {
throw new Error(typeof error === 'string' ? error : error?.message);
};
return fn(cb);
});
};

const {before, after} = require('mocha');
global.beforeAll = makeFn(before);
global.afterAll = makeFn(after);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mocha does global.before, jest does global.beforeAll. To make transition slightly easier, I thought having the same interface for some parts would be helpful.


/** @type {Mocha.Test} */
let mochaCurrentTest;
module.exports = {
mochaHooks: {
/** @this {Mocha.Context} */
beforeEach() {
if (!this.currentTest) throw new Error('unexpected value');

// Needed so `expect` extension method can access information about the current test.
mochaCurrentTest = this.currentTest;
},
afterAll() {
for (const snapshotState of snapshotStatesByTestFile.values()) {
// Jest adds `file://` to inline snapshot paths, and uses its own fs module to read things,
// falling back to fs.readFileSync if not defined. node `fs` does not support
// protocols in the path specifier, so we remove it here.
// @ts-expect-error - private property.
for (const snapshot of snapshotState._inlineSnapshots) {
snapshot.frame.file = snapshot.frame.file.replace('file://', '');
}

snapshotState.save();
}
},
},
};
11 changes: 10 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
"debug": "node --inspect-brk ./lighthouse-cli/index.js",
"start": "yarn build-report --standalone && node ./lighthouse-cli/index.js",
"jest": "node --experimental-vm-modules ./node_modules/jest/bin/jest.js",
"____comment": "echo https://github.com/cucumber/cucumber-js/issues/1844",
"mocha": "cp node_modules/mocha/bin/mocha node_modules/mocha/bin/mocha.js && node --loader=testdouble node_modules/mocha/bin/mocha.js --require=lighthouse-core/test/mocha-setup.js",
"test": "yarn diff:sample-json && yarn lint --quiet && yarn unit && yarn type-check",
"test-bundle": "yarn smoke --runner bundle -j=1 --retries=2 --invert-match forms",
"test-clients": "yarn jest \"$PWD/clients/\" && yarn jest --testMatch=\"**/clients/test/**/*-test-pptr.js\"",
Expand All @@ -47,12 +49,14 @@
"test-proto": "yarn compile-proto && yarn build-proto-roundtrip",
"unit-core": "yarn jest \"lighthouse-core\"",
"unit-cli": "yarn jest \"lighthouse-cli/\"",
"unit-cli-mocha": "yarn mocha \"lighthouse-cli/test/**/*-test.js\"",
"unit-report": "yarn jest \"report/\"",
"unit-treemap": "yarn jest \"treemap/.*-test.js\"",
"unit-viewer": "yarn jest \"viewer/.*-test.js\"",
"unit-flow": "yarn jest \"flow-report/.*-test.[tj]s[x]?\"",
"unit": "yarn jest",
"unit:ci": "NODE_OPTIONS=--max-old-space-size=8192 npm run jest --ci .",
"unit-mocha": "yarn mocha",
"unit:ci": "NODE_OPTIONS=--max-old-space-size=8192 npm run jest --ci . && npm run mocha",
"core-unit": "yarn unit-core",
"cli-unit": "yarn unit-cli",
"viewer-unit": "yarn unit-viewer",
Expand Down Expand Up @@ -119,8 +123,10 @@
"@types/lodash.get": "^4.4.6",
"@types/lodash.isequal": "^4.5.2",
"@types/lodash.set": "^4.3.6",
"@types/mocha": "^9.0.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some type collisions here inside node_modules (across globals declared by jest and mocha) that I need help dealing with.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

patch-package could work, just deleting the lines that declare the global vars in mocha (and relying on the mocha-setup to coerce the jest-defined interface and use the mocha functions there)

I think this is unavoidable, unless we make the switch to mocha in one giant PR.

cypress-io/cypress#6690 (comment)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the problem that we have jest and mocha tests in the same namespace? We could use project references to split cli unit tests up in that case. Or is using jest-mock/snapshots/expect enough to clash with the mocha types?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They both do toplevel declare var, so the global namespace.

I don't see how project references would resolve it–rather, I don't know enough about using projects in TS to know how it'd solve things.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const {describe, it} = require('mocha') works (though the globals still exist), so we could also convert to using actual imports rather than the terrible global thing all these test runners do. This person apparently keeps a fork of @types/mocha but with only local declarations: mochajs/mocha#956 (comment) 🤷

But it might just be easier to use project references.

I don't see how project references would resolve it–rather, I don't know enough about using projects in TS to know how it'd solve things.

Really they wouldn't even be project references here, it would just be a different tsconfig which doesn't include the jest types (or the mocha types, depending on where we put it). This is the same as e.g. the treemap tsconfig, where we only include the tabulator-tables types from node_modules, and exclude all the other types (like we don't want any of the @types/node globals muddying up code suggestions in code that's only going to run in the browser):

// Selectively include types from node_modules/.
"types": ["tabulator-tables"],

FWIW projects references are just that, multiple tsconfigs, with the addition of the references section where one tsconfig can reference another as a type dependency, e.g. treemap pulling in '../report/' as a dependency in the exact same way it's pulling in tabulator-tables as one. It's much more analogous to our multiple eslintrc files than yarn/npm workspaces or lerna or whatever.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've run into complications with this approach because referencing files outside of a tsconfig project is not allowed.

image

diff --git a/lighthouse-cli/test/cli/mocha-globals.d.ts b/lighthouse-cli/test/cli/mocha-globals.d.ts
new file mode 100644
index 000000000..23f979f1f
--- /dev/null
+++ b/lighthouse-cli/test/cli/mocha-globals.d.ts
@@ -0,0 +1,5 @@
+import expect_ from 'expect';
+
+declare global {
+  const expect: typeof expect_;
+}
diff --git a/lighthouse-cli/test/cli/tsconfig.json b/lighthouse-cli/test/cli/tsconfig.json
new file mode 100644
index 000000000..88fce8081
--- /dev/null
+++ b/lighthouse-cli/test/cli/tsconfig.json
@@ -0,0 +1,15 @@
+{
+  "extends": "../../../tsconfig-base.json",
+  "compilerOptions": {
+    // "rootDir": "../../..",
+    // "outDir": "../../../.tmp/tsbuildinfo/cli-tests",
+    // Selectively include types from node_modules/.
+    "types": ["tabulator-tables", "mocha"],
+  },
+  "include": [
+    "**/*.js",
+    "mocha-globals.d.ts",
+    "../../../types/global-lh.d.ts",
+    // "../../../**/*.js"
+  ],
+}

"@types/node": "*",
"@types/pako": "^1.0.1",
"@types/proxyquire": "^1.3.28",
"@types/raven": "^2.5.1",
"@types/resize-observer-browser": "^0.1.1",
"@types/semver": "^5.5.0",
Expand Down Expand Up @@ -156,12 +162,14 @@
"idb-keyval": "2.2.0",
"intl-messageformat-parser": "^1.8.1",
"jest": "27.1.1",
"jest-mock": "^27.3.0",
"jsdom": "^12.2.0",
"jsonld": "^5.2.0",
"jsonlint-mod": "^1.7.6",
"lighthouse-plugin-publisher-ads": "^1.5.4",
"magic-string": "^0.25.7",
"mime-types": "^2.1.30",
"mocha": "^9.1.3",
"node-fetch": "^2.6.1",
"npm-run-posix-or-windows": "^2.0.2",
"pako": "^2.0.3",
Expand All @@ -177,6 +185,7 @@
"rollup-plugin-terser": "^7.0.2",
"tabulator-tables": "^4.9.3",
"terser": "^5.3.8",
"testdouble": "^3.16.3",
"ts-jest": "^27.0.4",
"typed-query-selector": "^2.6.1",
"typescript": "^4.5.2",
Expand Down
Loading