tests: sequester mocks to `-test.mocks.js` files #14216

connorjclark · 2022-07-15T19:43:24Z

The purpose of this refactor is to avoid needing to think deeply about the module graph when mocking modules in a test. Since static imports are run first in ESM, there is no way to (within the same module) mock modules in a way that lets you access those mocked modules through the initial static imports (that's why the dynamic import mess exists right now). Instead, with this PR we define all the td.replaceEsm calls in a separate file, which our custom Mocha runner will import before loading the test file. If the test needs access to anything this mocks.js file sets up, it will be available via global.lighthouseTestContext.

Initially I explored a Mocha-centric solution:

Where the canonical test file name is the "wrapper", and hacks at Mocha's lifecycle via suite.beforeAll to setup test suites dynamically just in time. This would have allowed for the Mocha runner to be given multiple such tests (instead of the one-at-a-time method required currently) because suite.beforeAll runs just before Mocha runs the tests for that file. Do the setup in a normal root-scope beforeAll or inside a describe wouldn't work because tests added dynamically at that point are ignored by Mocha. Some drawbacks here:

it.only and --grep cease to work for these files
It really abuses Mocha's lifecycles
It's too tied to Mocha for my liking, and becomes a (potential) hurdle for any future attempt to change the test runner again (hopefully we won't need to)

connorjclark · 2022-07-15T19:46:30Z

lighthouse-core/test/scripts/run-mocha-tests.js

@@ -299,6 +300,11 @@ async function runMocha(tests, mochaArgs, invocationNumber) {
  const rootHooksPath = mochaArgs.require || '../test-env/mocha-setup.js';
  const {rootHooks} = await import(rootHooksPath);

+  let mocksFilePath;
+  if (tests.length === 1) {


Leaning on the definitions of "test isolation" above to indirectly control when these mocks files can be called. When isolation stuff is dropped (I'm working on it now), the runner can be rewritten slightly to instead run files by themselves if they have a mocks file defined. It wouldn't be a big change to do that in this PR but, I didn't think it was necessary, preferring a more incremental change.

adamraine · 2022-07-18T20:05:07Z

lighthouse-cli/test/cli/bin-test.js

-});
+/** @type {import('./bin-test.mocks.js').TestContext} */
+// @ts-expect-error
+const testContext = global.lighthouseTestContext;


This seems like a roundabout way of doing import {testContext} from './bin-test.mocks.js' as the first import of the file. Could we do that instead?

Eslint could be updated to enforce this import order.

No, because the import cache is refreshed after every call to td.replaceEsm, hence this whole setup.

Also I don't think order of static import evaluation is even specified in ESM.

^that's old knowledge. now, it's "source defined order" except for when there is async code in the root module scope, which all of these setup files have.

https://stackoverflow.com/questions/35551366/what-is-the-defined-execution-order-of-es6-imports

connorjclark · 2022-08-09T16:54:29Z

This is blocking #14202 and removal of "isolation" from the test runner.

adamraine · 2022-07-18T20:50:11Z

lighthouse-core/test/scripts/run-mocha-tests.js

@@ -171,6 +171,8 @@ const rawArgv = y
 const argv =
  /** @type {Awaited<typeof rawArgv> & CamelCasify<Awaited<typeof rawArgv>>} */ (rawArgv);

+process.env.SNAPSHOT_UPDATE = argv.update ? '1' : '';


We're already doing this in main right?

brendankenny · 2022-08-09T18:35:38Z

This is blocking #14202 and removal of "isolation" from the test runner.

What is the blocking part? I thought this was a reorganization for clarity of mocking/import order?

connorjclark · 2022-08-09T18:58:16Z

For windows: #14202 (comment)

The errors seen in CI here are similar to what this PR tries to avoid. It could be the issue is elsewhere–I figured I'd wait for this to land and remove an extra source of issues before debugging further. Debugging potential issues being Windows....or quibble....or mocks.... was getting too much so I wanted to just solve one first.

For isolation: With this PR we can remove the manual listing of what test files to isolate, and instead either 1) isolate the files implicitly if they have an associated mocks file or 2) go further, and figure out how to run the mocks files just before a test module is loaded within a single Mocha node-api invocation.

connorjclark · 2022-10-24T23:09:54Z

I confirmed that this PR + #14202 fixes windows CI.

https://github.com/GoogleChrome/lighthouse/actions/runs/3316847458

adamraine · 2022-10-24T23:35:54Z

Is it possible to unblock #14202, without this PR?

TBH I would prefer avoiding this solution if we can. It's pretty messy and I'm becoming less concerned about "thinking deeply about the module graph".

We could use dynamic imports with toplevel await to avoid the messy before callbacks and stuff. This would make it easy to convert a bunch of static imports into dynamic imports if something needs to be mocked. We wouldn't even need to think that deeply, we could just err on the side of dynamic imports when importing from core.

connorjclark · 2022-10-25T01:15:34Z

Please send a draft PR converting navigation-runner-test.js and snapshot-runner-test.js (and enabling those tests for windows CI job) so we can see what that looks like.

connorjclark · 2022-11-15T20:10:22Z

We are going with the approach in #14468 - see my comment there: #14468 (review)

connorjclark added 11 commits July 14, 2022 16:03

tests: use workers and Mocha node api instead of calling the CLI

b00625a

force in process when just one test

70ae6a5

comment about parallel mode

a7faedc

pass bail thru

ae5da3f

remove old baseArgs stuff

09a5189

log error

5680cc3

fix snapshot update

5076b65

fix unit flow

91e35cf

fix fetcher test

0de0320

oof forgot to exit in worker

a95659c

wip: .mocks test files

db293b2

connorjclark changed the title ~~tests: sequester mocks to separate -test.mocks.js files~~ tests: sequester mocks to -test.mocks.js files Jul 15, 2022

connorjclark changed the base branch from master to mocha-runner July 15, 2022 19:44

connorjclark commented Jul 15, 2022

View reviewed changes

fix unit flow lol

22b6724

vercel bot deployed to Preview July 15, 2022 19:52 View deployment

user-flow-test.js

64a4a75

vercel bot deployed to Preview July 15, 2022 22:13 View deployment

snapshot-runner-test.mocks.js

2d75f0b

vercel bot deployed to Preview July 15, 2022 22:23 View deployment

connorjclark added 3 commits July 15, 2022 15:41

timespan-runner-test.mocks.js

3b7e7b6

navigation-test.js ugh

40925b2

prepare-test.js

e5db1f6

vercel bot deployed to Preview July 15, 2022 22:45 View deployment

link-elements-test.js

d459cea

vercel bot deployed to Preview July 15, 2022 23:50 View deployment

script-elements-test.js

e2a2fa9

vercel bot deployed to Preview July 15, 2022 23:52 View deployment

service-worker-test.js

b59d133

vercel bot deployed to Preview July 15, 2022 23:56 View deployment

connorjclark requested review from brendankenny and removed request for a team July 16, 2022 00:06

devtools-bot assigned brendankenny Jul 16, 2022

devtools-bot added the waiting4reviewer label Jul 16, 2022

Base automatically changed from mocha-runner to master July 18, 2022 18:23

Merge remote-tracking branch 'origin/master' into mocks-files

1f6bd56

vercel bot deployed to Preview July 18, 2022 18:30 View deployment

fix the worst mock shennanigans evers

e63e384

vercel bot deployed to Preview July 18, 2022 19:23 View deployment

adamraine reviewed Jul 18, 2022

View reviewed changes

connorjclark mentioned this pull request Jul 18, 2022

tests: fix mocha test runner and mocks on windows #14202

Merged

tsc

ed5911a

vercel bot deployed to Preview July 18, 2022 22:17 View deployment

adamraine reviewed Aug 9, 2022

View reviewed changes

connorjclark added 2 commits October 24, 2022 12:11

Merge remote-tracking branch 'origin/main' into mocks-files

e758655

Merge remote-tracking branch 'origin/main' into mocks-files

b557fa0

vercel bot deployed to Preview October 24, 2022 19:14 View deployment

fix merge

da8a4d7

vercel bot deployed to Preview October 24, 2022 19:24 View deployment

hmm

564c542

vercel bot deployed to Preview October 24, 2022 19:51 View deployment

adamraine mentioned this pull request Oct 25, 2022

tests: dynamically import all modules when using mocks #14468

Merged

connorjclark closed this Nov 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: sequester mocks to `-test.mocks.js` files #14216

tests: sequester mocks to `-test.mocks.js` files #14216

connorjclark commented Jul 15, 2022 •

edited

Loading

connorjclark Jul 15, 2022 •

edited

Loading

adamraine Jul 18, 2022

connorjclark Jul 18, 2022

connorjclark Jul 18, 2022 •

edited

Loading

connorjclark Jul 18, 2022 •

edited

Loading

connorjclark commented Aug 9, 2022

adamraine Jul 18, 2022

brendankenny commented Aug 9, 2022

connorjclark commented Aug 9, 2022 •

edited

Loading

connorjclark commented Oct 24, 2022

adamraine commented Oct 24, 2022

connorjclark commented Oct 25, 2022

connorjclark commented Nov 15, 2022

tests: sequester mocks to -test.mocks.js files #14216

tests: sequester mocks to -test.mocks.js files #14216

Conversation

connorjclark commented Jul 15, 2022 • edited Loading

connorjclark Jul 15, 2022 • edited Loading

Choose a reason for hiding this comment

adamraine Jul 18, 2022

Choose a reason for hiding this comment

connorjclark Jul 18, 2022

Choose a reason for hiding this comment

connorjclark Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

connorjclark Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

connorjclark commented Aug 9, 2022

adamraine Jul 18, 2022

Choose a reason for hiding this comment

brendankenny commented Aug 9, 2022

connorjclark commented Aug 9, 2022 • edited Loading

connorjclark commented Oct 24, 2022

adamraine commented Oct 24, 2022

connorjclark commented Oct 25, 2022

connorjclark commented Nov 15, 2022

tests: sequester mocks to `-test.mocks.js` files #14216

tests: sequester mocks to `-test.mocks.js` files #14216

connorjclark commented Jul 15, 2022 •

edited

Loading

connorjclark Jul 15, 2022 •

edited

Loading

connorjclark Jul 18, 2022 •

edited

Loading

connorjclark Jul 18, 2022 •

edited

Loading

connorjclark commented Aug 9, 2022 •

edited

Loading