misc: convert lighthouse-core/test to ES modules #13295

connorjclark · 2021-11-01T17:01:28Z

I suggest reviewing this PR one commit at a time. I converted entire folders at a time, ensuring that yarn jest continued to work throughout.

# These first two commits were very straightforward
core/test/config
core/test/audits
# Mocks were the main challenge here.
core/test/gather and core/test/fraggle-rock/gather
# Resolving the TS issues was a lot so I did that separately here
workaround the new jest unknown types
core/test/computed
core/test/lib
core/test/fraggle-rock
core/test
delete extra package.json s

Jest currently does not support the hoisting behavior of mocks in esmodules, meaning that test files using mocks must dynamically import some of their dependencies (just the ones that use files being mocked). I hope this is just a temporary workaround, so I left the regular imports as uncommented. If Jest ever supports this we'll be able to revert back to regular imports simply.

I didn't add a linter yet, can do so in later PR to reduce churn in this one.

connorjclark · 2021-11-01T17:04:12Z

lighthouse-core/test/.prettierrc

@@ -1,6 +0,0 @@
-printWidth: 100


connorjclark · 2021-11-01T17:05:11Z

lighthouse-core/test/audits/byte-efficiency/byte-efficiency-audit-test.js

+import PageDependencyGraph from '../../../computed/page-dependency-graph.js';
+import LoadSimulator from '../../../computed/load-simulator.js';
+import trace from '../../fixtures/traces/progressive-app-m60.json';
+import devtoolsLog from '../../fixtures/traces/progressive-app-m60.devtools.log.json';


I was surprised to see json imports working here. I guess Jest makes some allowances for it.

I was surprised to see json imports working here. I guess Jest makes some allowances for it.

how is this working? We have the jest transforms turned off and node should only do the import with --experimental-json-modules turned on (and even then only as import devtoolsLog from '../../fixtures/traces/progressive-app-m60.devtools.log.json' assert { type: 'json' };)

No idea! I was converting these to readJson, but once I noticed they still worked I stopped doing that midway and just allowed these to stay as json imports. I should probably finish the job...

wdyt: should all these become readJson or should we stick to importing them since that (somehow) works?

wdyt: should all these become readJson or should we stick to importing them since that (somehow) works?

Since the import is so simple I'm good with relying on whatever overly-magical thing jest is doing behind the scenes, with the hopes that by the time we need to do something different, import assertions will be stable in whatever minimum Node version we're using

connorjclark · 2021-11-01T17:10:25Z

lighthouse-core/test/fraggle-rock/gather/mock-driver.js

- emulationMock.clearThrottling = jest.fn();
- emulationMock.emulate = jest.fn();
- networkMock.fetchResponseBodyFromCache = jest.fn().mockResolvedValue('');
+ navigationMock.gotoURL = fnAny().mockResolvedValue({finalUrl: 'https://example.com', warnings: [], timedOut: false});


the jest here is now using types privided from jest directly via @jest/globals, whereas before it was from @types/jest (which is pinned to an older set of types). In the newer version, jest uses unknown where we relied on any for our types to work.

It's pretty awkward to use jsdoc to apply the correct template arguments, so I introduced a fnAny helper.

connorjclark · 2021-11-01T17:11:24Z

lighthouse-core/test/gather/gather-runner-test.js

+ 'assertNoSameOriginServiceWorkerClients'),
+ };
+}
+


This is the most complex example of the dynamic import workaround required for mocks to work.

lighthouse-core/test/gather/gather-runner-test.js

connorjclark · 2021-11-01T17:18:46Z

lighthouse-core/test/test-utils.js

@@ -238,41 +177,49 @@ async function flushAllTimersAndMicrotasks(ms = 1000) {
 * shouldn't concern themselves about.
 */
 function makeMocksForGatherRunner() {
- jest.mock('../gather/driver/environment.js', () => ({
+ jest.mock(require.resolve('../gather/driver/environment.js'), () => ({
 getBenchmarkIndex: () => Promise.resolve(150),


Even though this module is calling jest.mock, it seems that in esmodule test files the file is mocked relative to the test file instead. Using require.resolve to obtain an absolute path fixes that.

lighthouse-core/scripts/esm-utils.js

lighthouse-core/gather/driver/navigation.js

connorjclark · 2022-04-19T21:20:58Z

@adamraine @brendankenny want to look at this again?

brendankenny

ok, I'm like 80% through by file count, now only 80% of the complexity left :) Flushing comments...

brendankenny · 2022-04-20T18:09:48Z

lighthouse-core/test/audits/accessibility/aria-allowed-attr-test.js

-const Audit = require('../../../audits/accessibility/aria-allowed-attr.js');
-const assert = require('assert').strict;
+import Audit from '../../../audits/accessibility/aria-allowed-attr.js';
+import {strict as assert} from 'assert';


nit: do we want to start doing import assert from 'assert/strict';? Seems way better to me

We use this current form in other places to, so let's handle it everywhere after this PR lands. tracking #13883

brendankenny · 2022-04-20T18:20:00Z

lighthouse-core/test/audits/byte-efficiency/byte-efficiency-audit-test.js

+import PageDependencyGraph from '../../../computed/page-dependency-graph.js';
+import LoadSimulator from '../../../computed/load-simulator.js';
+import trace from '../../fixtures/traces/progressive-app-m60.json';
+import devtoolsLog from '../../fixtures/traces/progressive-app-m60.devtools.log.json';


wdyt: should all these become readJson or should we stick to importing them since that (somehow) works?

Since the import is so simple I'm good with relying on whatever overly-magical thing jest is doing behind the scenes, with the hopes that by the time we need to do something different, import assertions will be stable in whatever minimum Node version we're using

brendankenny · 2022-04-20T18:24:40Z

lighthouse-core/test/audits/maskable-icon-test.js

 const manifestWithoutMaskableSrc =
- JSON.stringify(require('../fixtures/manifest-no-maskable-icon.json'));
+ JSON.stringify(readJson('lighthouse-core/test/fixtures/manifest-no-maskable-icon.json'));


should these change to the bare json imports too?

import manifest from 'lighthouse-core/test/fixtures/manifest.json'; import manifestWithoutMaskable = 'lighthouse-core/test/fixtures/manifest-no-maskable-icon.json'; const manifestSrc = JSON.stringify(manifest); const manifestWithoutMaskableSrc = JSON.stringify(manifestWithoutMaskable);

lighthouse-core/test/audits/splash-screen-test.js

lighthouse-core/test/audits/themed-omnibox-test.js

brendankenny · 2022-04-20T18:48:48Z

lighthouse-core/test/fraggle-rock/config/config-test.js

-const BaseGatherer = require('../../../fraggle-rock/gather/base-gatherer.js');
-const {initializeConfig} = require('../../../fraggle-rock/config/config.js');
-const {LH_ROOT} = require('../../../../root.js');
+import {jest} from '@jest/globals';


this isn't injected but describe, it, etc is?

lighthouse-core/test/gather/gatherers/main-document-content-test.js

lighthouse-core/test/gather/gatherers/network-user-agent-test.js

brendankenny · 2022-04-20T18:50:27Z

lighthouse-core/test/jest-setup/package.json

@@ -0,0 +1,4 @@
+{
+ "type": "commonjs",
+ "//": "jest only supports commonjs for setup modules"


lighthouse-core/test/lib/tracehouse/trace-processor-test.js

connorjclark · 2022-04-25T19:56:30Z

@brendankenny did you have more review to share?

brendankenny

Sorry I was the blocker on this. Changes look good to me, but I only looked at a high level at the mocking changes and I'm hoping @adamraine looked closer than I did :)

Yay big progress on esm!

brendankenny · 2022-05-03T19:26:37Z

lighthouse-core/scripts/esm-utils.js

+/**
+ * @param {ImportMeta} importMeta
+ */
+function createCommonjsRefs(importMeta) {


I can see the benefit of reducing churn in this change by providing this, but it does seem like we should be looking to the future, too. e.g. new files that need a _dirname + '/path/to/something', should really be doing new URL('/path/to/something', import.meta.url) rather than encouraging further use of __filename and _dirname

agreed-#13953

connorjclark requested a review from a team as a code owner November 1, 2021 17:01

connorjclark requested review from adamraine and removed request for a team November 1, 2021 17:01

google-cla bot added the cla: yes label Nov 1, 2021

connorjclark changed the title ~~Merge remote-tracking branch 'origin/master' into esmodules-rollup~~ core: convert lighthouse-core/test to ES modules Nov 1, 2021

connorjclark changed the title ~~core: convert lighthouse-core/test to ES modules~~ misc: convert lighthouse-core/test to ES modules Nov 1, 2021

devtools-bot assigned adamraine Nov 1, 2021

devtools-bot added the waiting4reviewer label Nov 1, 2021

connorjclark commented Nov 1, 2021

View reviewed changes

lighthouse-core/test/.prettierrc

@@ -1,6 +0,0 @@

printWidth: 100

Copy link

Collaborator Author

connorjclark Nov 1, 2021

Choose a reason for hiding this comment

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

👀

connorjclark commented Nov 1, 2021

View reviewed changes

vercel bot deployed to Preview November 1, 2021 17:13 View deployment

connorjclark commented Nov 1, 2021

View reviewed changes

lighthouse-core/test/gather/gather-runner-test.js Show resolved Hide resolved

connorjclark commented Nov 1, 2021

View reviewed changes

vercel bot deployed to Preview November 1, 2021 17:31 View deployment

Base automatically changed from esmodules-rollup to master November 1, 2021 17:34

vercel bot deployed to Preview November 1, 2021 17:36 View deployment

connorjclark force-pushed the esm-core-1 branch from 7daa80e to eb48cfc Compare November 1, 2021 17:42

vercel bot deployed to Preview November 1, 2021 17:42 View deployment

connorjclark added 10 commits November 1, 2021 10:43

core/test/config

51516a7

core/test/audits

f4b3152

core/test/gather and core/test/fraggle-rock/gather

c25a378

workaround the new jest unknown types

b3d35a9

core/test/computed

aa077f9

core/test/lib

67ead9b

core/test/fraggle-rock

2d2944d

core/test

ec26108

delete extra package.json s

5f75927

undo some async stuff

0bfb34d

vercel bot deployed to Preview November 17, 2021 01:04 View deployment

adamraine approved these changes Nov 17, 2021

View reviewed changes

lighthouse-core/scripts/esm-utils.js Outdated Show resolved Hide resolved

lighthouse-core/gather/driver/navigation.js Outdated Show resolved Hide resolved

connorjclark added 2 commits November 17, 2021 11:57

pr

a92c993

Merge remote-tracking branch 'origin/master' into esm-core-1

0a2942e

vercel bot deployed to Preview November 17, 2021 19:58 View deployment

vercel bot deployed to Preview April 19, 2022 18:02 View deployment

merge from master

5272656

connorjclark force-pushed the esm-core-1 branch from 5cd40eb to 5272656 Compare April 19, 2022 18:16

vercel bot deployed to Preview April 19, 2022 18:17 View deployment

adamraine approved these changes Apr 19, 2022

View reviewed changes

brendankenny reviewed Apr 20, 2022

View reviewed changes

use json import where possible

17aed3e

vercel bot deployed to Preview April 20, 2022 23:36 View deployment

connorjclark added 2 commits April 20, 2022 16:38

Merge remote-tracking branch 'origin/master' into esm-core-1

ba645e1

Merge remote-tracking branch 'origin/master' into esm-core-1

5f55fbf

vercel bot deployed to Preview April 21, 2022 00:12 View deployment

connorjclark added waiting4reviewer and removed waiting4committer labels Apr 25, 2022

Merge remote-tracking branch 'origin/master' into esm-core-1

0f639e2

vercel bot deployed to Preview April 27, 2022 18:29 View deployment

brendankenny approved these changes May 3, 2022

View reviewed changes

connorjclark mentioned this pull request May 3, 2022

Remove createCommonjsRefs #13953

Closed

connorjclark merged commit 5786b79 into master May 3, 2022

connorjclark deleted the esm-core-1 branch May 3, 2022 23:14

This was referenced May 4, 2022

tests(eslint): add import/order rule for core tests #13955

Merged

tests: fix unconverted test #13959

Merged

☂️ Convert to ES modules #12689

Closed

brendankenny mentioned this pull request May 18, 2022

tests: use readJson instead of imports for json #14020

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: convert lighthouse-core/test to ES modules #13295

misc: convert lighthouse-core/test to ES modules #13295

connorjclark commented Nov 1, 2021 •

edited

Loading

connorjclark Nov 1, 2021

connorjclark Nov 1, 2021

brendankenny Nov 9, 2021

connorjclark Nov 16, 2021 •

edited

Loading

connorjclark Apr 19, 2022 •

edited

Loading

brendankenny Apr 20, 2022

connorjclark Nov 1, 2021

connorjclark Nov 1, 2021 •

edited

Loading

connorjclark Nov 1, 2021

connorjclark commented Apr 19, 2022

brendankenny left a comment

brendankenny Apr 20, 2022

connorjclark Apr 20, 2022

brendankenny Apr 20, 2022

brendankenny Apr 20, 2022

brendankenny Apr 20, 2022

connorjclark Apr 20, 2022

brendankenny Apr 20, 2022

connorjclark commented Apr 25, 2022

brendankenny left a comment

brendankenny May 3, 2022

connorjclark May 3, 2022

misc: convert lighthouse-core/test to ES modules #13295

misc: convert lighthouse-core/test to ES modules #13295

Conversation

connorjclark commented Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Nov 16, 2021 • edited Loading

Choose a reason for hiding this comment

connorjclark Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Apr 19, 2022

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Apr 25, 2022

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Nov 1, 2021 •

edited

Loading

connorjclark Nov 16, 2021 •

edited

Loading

connorjclark Apr 19, 2022 •

edited

Loading

connorjclark Nov 1, 2021 •

edited

Loading