tests: sequester mocks to `-test.mocks.js` files
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:
- https://github.com/GoogleChrome/lighthouse/blob/mock-wrapper-test-file/lighthouse-core/test/runner-test.js
- https://github.com/GoogleChrome/lighthouse/blob/mock-wrapper-test-file/lighthouse-core/test/runner-test.impl.js
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.onlyand--grepcease 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)
This is blocking #14202 and removal of "isolation" from the test runner.
This is blocking https://github.com/GoogleChrome/lighthouse/pull/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?
For windows: https://github.com/GoogleChrome/lighthouse/pull/14202#issuecomment-1188289930
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.
I confirmed that this PR + #14202 fixes windows CI.
https://github.com/GoogleChrome/lighthouse/actions/runs/3316847458
Is it possible to unblock https://github.com/GoogleChrome/lighthouse/pull/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.
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.
We are going with the approach in #14468 - see my comment there: https://github.com/GoogleChrome/lighthouse/pull/14468#pullrequestreview-1174827046