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

Conversation

connorjclark
Copy link
Collaborator

  • Use mocha instead of jest for test runner, this PR is just the unit-cli tests
  • Still using Jest mocks via jest-mock. Package works independent of the Jest test runner
  • Also, still using Jest snapshots via jest-snapshot. Set SNAPSHOT_UPDATE=1 to update snapshots
  • Use testdouble to replace es module imports (with the jest mocks)
  • mocha-setup.js does two things: 1) sets global.expect 2) configures the mocha test runner to use jest-snapshot
  • Need to confirm coverage works

@connorjclark connorjclark requested a review from a team as a code owner January 13, 2022 22:27
@connorjclark connorjclark requested review from adamraine and removed request for a team January 13, 2022 22:27
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 {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.

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.

@@ -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"
+  ],
+}

@connorjclark
Copy link
Collaborator Author

@paulirish

quibble

That is part of the testdouble module replacing library. https://github.com/testdouble/quibble

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants