tests: use smokehouse runner for test-bundle #9943

brendankenny · 2019-11-07T22:41:20Z

follow up to #9791 and #9843

Moves the bundled lighthouse test to the new smokehouse format. Uses the devtools bundle in this PR, though we can change that easily (to lr, to dt & lr, or to a standalone bundle).

Lower priority right now, but it's really handy for being able to test the devtools bundle locally (at least without a Chromium dev setup) while we try to get the plugin bundled in.

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

brendankenny · 2019-11-07T22:42:50Z

build/build-bundle.js

@@ -38,7 +38,8 @@ const isDevtools = file => path.basename(file).includes('devtools');
 /** @param {string} file */
 const isLightrider = file => path.basename(file).includes('lightrider');

-const BANNER = `// lighthouse, browserified. ${VERSION} (${COMMIT_HASH})\n`;
+const BANNER = `// lighthouse, browserified. ${VERSION} (${COMMIT_HASH})\n` +
+  '// @ts-nocheck\n'; // To prevent tsc stepping into any required bundles.


since dist/lighthouse-dt-bundle.js is required in smokehouse and smokehouse is type checked, tsc automatically starts checking the bundle code too. This is the only reliable way I could think of to stop it, but happy for alternatives :)

brendankenny · 2019-11-07T22:46:06Z

clients/devtools-entry.js

+  // Instead, since this file is only ever run in node for testing, expose a
+  // bundle entry point as global.
+  // @ts-ignore
+  global.runBundledLighthouse = lighthouse;


originally was going to make runLighthouseInWorker global, but we need to substitute in a connection and config (instead of port and categoryIds). Rather than making the function unreadable with a bunch of options or requiring devtools to call the module differently (which we could still do in the future), exposing the inner lighthouse call seemed like the simplest fix.

I am slightly concerned about drift between how devtools calls runLighthouseInWorker and what happens inside that function and the default interface, but for now it's a really simple correspondence.

This drift sounds fine to me. When I have the smoke tests run against the Audits panel, it will be at a higher level than this bundle, so the drift between runBundledLighthouse and runLighthouseInWorker is irrelevant (won't even used the former). I will need to add a config, but that's simple.

So while runBundledLighthouse happens to live here, it's got nothing to do with devtools. That is a little confusing - what about making a new client for this, bundle-entry.js?

When I have the smoke tests run against the Audits panel, it will be at a higher level than this bundle

ah, good to know, I didn't realize that, though it makes sense.

So while runBundledLighthouse happens to live here, it's got nothing to do with devtools. That is a little confusing - what about making a new client for this, bundle-entry.js?

Agree it's kind of confusing, but the benefit is it's getting all the bundle customizations--all the isDevTools tweaks, for instance--so it's making sure the real bundles are working. I was thinking we could add a smoke test just for the pubAds integration, for instance.

Originally I had it something like

function runLighthouseInWorker() { // ... runLighthouse(...args); } /** * Intermediate function to expose for testing. */ function runLighthouse() { lighthouse(...args) } global.runBundledLighthouse = runLighthouse;

that spelled it out a little more clearly but also seemed a bit silly.

We could change the runLighthouseInWorker interface to make it more flexible, too (like make it take a config and devtools code could call getDefaultConfigForCategories with the categoryIds itself and then pass the config back in), but I figured now's probably not the time for such changes :)

Other ways we could make things more explicit and clearer?

brendankenny · 2019-11-07T22:48:41Z

lighthouse-cli/test/smokehouse/lighthouse-runners/bundle.js

@@ -0,0 +1,54 @@
+/**


maybe this should be launch-chrome-and-bundle.js or something to differentiate from the pure bundle runner needed by #9873. We could bikeshed now or later

brendankenny · 2019-11-07T22:53:02Z

lighthouse-cli/test/smokehouse/lighthouse-runners/bundle.js

+  return {
+    lhr: runnerResult.lhr,
+    artifacts: runnerResult.artifacts,
+    log: '', // TODO: if want to run in parallel, need to capture lighthouse-logger output.


there is a way to control where debug output goes, but I'm skipping looking into the complications of that (singletons! :shakes_fist:) for now since I don't think we're going to want to run the bundled version multiple times in the same process. If it's only ever run with -j=1, the output will always be properly ordered.

connorjclark

good changes. sad to see my magic bundled cli go, but for the best.

connorjclark · 2019-11-07T22:53:39Z

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

@@ -76,7 +79,7 @@ async function begin() {
    .alias({
      'jobs': 'j',
    })
-    .choices('runner', ['cli'])
+    .choices('runner', ['cli', 'bundle'])


should there be a --runner-path, so others could make use a custom runner w/o changing this file?

makes sense to just wait for someone to request this feature, but it's easy enough to do perhaps we should just do it.

Actually, I will be one such user in the near future :)

should there be a --runner-path, so others could make use a custom runner w/o changing this file?

Could we just add it when you need it? Agreed it fits in easily, these are more or less just shorthand forms of that, but I'd like to avoid messing with yargs more right now :)

should there be a --runner-path, so others could make use a custom runner w/o changing this file?

though smokehouse.js can already take any custom runner without changes. I kind of figured that when we needed a really custom runner, we would also need firehouse's test filtering and so could just make a new frontend (smoke-mask :D). Like 80% of this file is parsing argv anyways.

connorjclark · 2019-11-07T22:54:37Z

build/build-bundle.js

@@ -38,7 +38,8 @@ const isDevtools = file => path.basename(file).includes('devtools');
 /** @param {string} file */
 const isLightrider = file => path.basename(file).includes('lightrider');

-const BANNER = `// lighthouse, browserified. ${VERSION} (${COMMIT_HASH})\n`;
+const BANNER = `// lighthouse, browserified. ${VERSION} (${COMMIT_HASH})\n` +
+  '// @ts-nocheck\n'; // To prevent tsc stepping into any required bundles.


can we just exclude the bundle in tsconfig?

can we just exclude the bundle in tsconfig?

from my experience this isn't possible (include and exclude are just including and excluding root files that tsc starts walking from), but I would be happy if I was missing something :)

See https://www.typescriptlang.org/docs/handbook/tsconfig-json.html#details ("Any files that are referenced by files included...")

connorjclark · 2019-11-07T22:55:23Z

clients/devtools-entry.js

-    lookupLocale,
-  };
+  // Ideally this could be exposed via browserify's `standalone`, but it doesn't
+  // work for LH because of https://github.com/browserify/browserify/issues/968.


Suggested change

// work for LH because of https://github.com/browserify/browserify/issues/968.

// work for LH because of https://github.com/browserify/browserify/issues/968

(period broke link)

tests: move test-bundle to a smokehouse runner

ef68393

brendankenny requested review from patrickhulce and paulirish as code owners November 7, 2019 22:41

googlebot added the cla: yes label Nov 7, 2019

connorjclark reviewed Nov 7, 2019

View reviewed changes

lighthouse-cli/test/smokehouse/frontends/smokehouse-bin.js Show resolved Hide resolved

brendankenny commented Nov 7, 2019

View reviewed changes

connorjclark approved these changes Nov 7, 2019

View reviewed changes

fix link

5298b0b

vercel bot deployed to staging November 7, 2019 23:33 View deployment

brendankenny mentioned this pull request Nov 21, 2019

clients(devtools): split up runLighthouseInWorker and expose to worker #10005

Merged

Merge remote-tracking branch 'origin/master' into smokebundle

82e9793

vercel bot deployed to staging December 13, 2019 23:57 View deployment

comments

b3576b1

vercel bot deployed to staging December 14, 2019 00:06 View deployment

connorjclark approved these changes Dec 14, 2019

View reviewed changes

connorjclark added the land-when-ci-is-green label Dec 14, 2019

connorjclark changed the title ~~tests: move test-bundle to a smokehouse runner~~ tests: use smokehouse runner for test-bundle Dec 14, 2019

connorjclark merged commit 8f75443 into master Dec 14, 2019

connorjclark deleted the smokebundle branch December 14, 2019 01:46

brendankenny mentioned this pull request Apr 28, 2022

tests(ci): use BuildJet for ubuntu jobs #13943

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: use smokehouse runner for test-bundle #9943

tests: use smokehouse runner for test-bundle #9943

brendankenny commented Nov 7, 2019

brendankenny Nov 7, 2019

brendankenny Nov 7, 2019

connorjclark Nov 7, 2019 •

edited

Loading

brendankenny Nov 7, 2019

brendankenny Nov 7, 2019

brendankenny Nov 7, 2019

connorjclark left a comment

connorjclark Nov 7, 2019

connorjclark Nov 7, 2019

brendankenny Nov 7, 2019

brendankenny Nov 7, 2019

connorjclark Nov 7, 2019

brendankenny Nov 7, 2019

connorjclark Nov 7, 2019

	// work for LH because of https://github.com/browserify/browserify/issues/968.
	// work for LH because of https://github.com/browserify/browserify/issues/968

tests: use smokehouse runner for test-bundle #9943

tests: use smokehouse runner for test-bundle #9943

Conversation

brendankenny commented Nov 7, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark Nov 7, 2019 • edited Loading

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 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 Nov 7, 2019 •

edited

Loading