tests(smokehouse): run smoketests in parallel #4748

paulirish · 2018-03-13T23:27:33Z

From 149s (2.5 minutes!) down to 45s!

Mostly a matter of running them in parallel.

Changes

Replaced those gnarly run-test.sh scripts with a flexible JS-based smoketest runner. Runs each smokehouse.js invocation in its own child-process.
Moved a run of dbw-expectations into offline, as it was testing the exact same URL, just asserting different aspects out the output.
yarn smoke will run all, but you can do yarn smoke byte and yarn smoke byte ttci to run individual smokehouse expectations
Split the PWA smoketests into two files (pwa and pwa2), as they were some of the slowest smoketests we did.
yarn smokehouse was removed. it's effectively replaced with yarn smoke pwa && yarn smoke pwa2
Restore the shop.polymer-project.org smoketest.
- Add a workaround for start_url manifest fetch failure for keep.google.com? #4898, since it reproduces in this test.

brendankenny

this seems good most of the time, but I am concerned about the debuggability if something does go wrong and the output is all interleaved together :)

also, whats the feasibility of getting rid of all the run-tests.sh files for just one :)

paulirish · 2018-03-14T23:10:33Z

I am concerned about the debuggability if something does go wrong and the output is all interleaved together :)

Aye. I added an extra line of output where "checking " is emitted right before the assertions are checked. IMO i think that lowers the probability of confusing results a LOT. But it's still possible and to fix that means buffering output which means any failures you won't see until ALL smoketests are done. Also requires lots more work. :)

also, whats the feasibility of getting rid of all the run-tests.sh files for just one :)

And going to JS? Mmmmmmm that's also an option.

paulirish · 2018-03-14T23:11:29Z

@patrickhulce (no rush, but..) thoughts? right now it looks like this work will go into ❄️ chillin state.

brendankenny · 2018-03-15T19:03:26Z

And going to JS? Mmmmmmm that's also an option.

shelljs is somewhat active again so could work ok for this (and get us easier windows support)

paulirish · 2018-03-15T23:15:39Z

TBH it could move to JS without shelljs, even. really not too terrible.

wardpeet · 2018-03-25T15:04:27Z

100% js would be nice.

For now we could still buffer the output inside a variable and output it where needed if we are afraid of debugging.

paulirish · 2018-03-25T21:52:44Z

For now we could still buffer the output inside a variable and output it where needed if we are afraid of debugging.

Yeah.. Now with basic pattern of https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/scripts/update-report-fixtures.js available, we can much more easily do this in JS. And yeah we can avoid interleaving much easier. 👍

brendankenny

once you're feeling good about them, can you also list all the changes to the individual smoke tests. Like yarn smokehouse is getting broken out into an explicit pwa smoke test, a bunch of stuff is getting moved into offline-expectations, but I believe you mentioned that was because you were going to get rid of one of the existing tests that already had a lot of overlap?

brendankenny · 2018-04-09T23:46:38Z

package.json

-    "smoke": "bash lighthouse-cli/test/smokehouse/run-all-tests.sh",
+
+    "smoke": "node lighthouse-cli/test/smokehouse/run-smoke.js",
+    "coverage": "istanbul cover -x \"**/third_party/**\" _mocha -- $(find */test -name '*-test.js') --reporter progress --report lcovonly",


rebase problem?

brendankenny · 2018-04-09T23:48:16Z

lighthouse-cli/test/smokehouse/smokehouse.js

  const results = runLighthouse(expected.initialUrl, configPath);
+  console.log(`Comparing results of '${expected.initialUrl}':`);


maybe console.log(`Auditing findings of '${expected.initialUrl}':`);?

brendankenny · 2018-04-09T23:49:03Z

lighthouse-cli/test/smokehouse/run-smoke.js

+    config: smokehouseDir + 'pwa-config.js',
+  },
+};
+


brendankenny · 2018-04-09T23:49:29Z

lighthouse-cli/test/smokehouse/run-smoke.js

+};
+
+
+function displayOutput(cp) {


brendankenny · 2018-04-09T23:49:42Z

lighthouse-cli/test/smokehouse/run-smoke.js

+}
+
+/**
+ * Update the report artifacts


old comment?

brendankenny · 2018-04-09T23:52:21Z

lighthouse-cli/test/smokehouse/run-smoke.js

+  serverForOffline.listen(10503, 'localhost');
+
+  const cmdPromises = [];
+  for (const [id, {expectations, config}] of Object.entries(smokes)) {


add way to specify how many to run at the same time? But it sounds like you may be planning that already

patrickhulce

lgtm % nits

patrickhulce · 2018-04-10T16:58:37Z

lighthouse-cli/test/smokehouse/run-smoke.js

+  for (const {id, expectations, config} of smokes) {
+    // If the machine is terribly slow, do them in succession, not parallel
+    if (process.env.APPVEYOR) {
+      await Promise.all(cmdPromises);


I'd have a slight preference to just move this farther down and await p for clarity

yeah, it's a little confusing at first how this would work since it's Promise.all and at the top of the loop, though it's nice doing it in one line and with the same array as the parallel case :)

patrickhulce · 2018-04-10T17:01:00Z

lighthouse-cli/test/smokehouse/run-smoke.js

+  const serialSmokeResults = await runSmokehouse(serialSmokes);
+  smokeResults.push(...serialSmokeResults);
+
+  await new Promise(res => server.close(res));


nit: this really throws me just because res is the canonical express response shorthand and then later you use it for result, resolve or done? :)

patrickhulce · 2018-04-10T17:01:43Z

lighthouse-cli/test/smokehouse/run-smoke.js

+  await new Promise(res => server.close(res));
+  await new Promise(res => serverForOffline.close(res));
+
+  const failingTests = smokeResults.filter(res => !!res.error);


nit: s/res/anything else :)

patrickhulce · 2018-04-10T17:10:12Z

lighthouse-cli/test/smokehouse/smokehouse.js

  const results = runLighthouse(expected.initialUrl, configPath, cli.debug);
+
+  console.log(`Auditing findings of '${expected.initialUrl}':`);


🤦‍♂️

maybe console.log(`Scoring reportCategory results of '${expected.initialUrl}':`); instead?

brendankenny · 2018-04-10T17:35:22Z

lighthouse-cli/test/smokehouse/pwa2-expectations.js

+    },
+  },
+
+  // Disabled due to flakiness of site.


should we reenable :)

brendankenny · 2018-04-10T17:37:30Z

lighthouse-cli/test/smokehouse/smokehouse.js

  const results = runLighthouse(expected.initialUrl, configPath, cli.debug);
+
+  console.log(`Auditing findings of '${expected.initialUrl}':`);


maybe console.log(`Scoring reportCategory results of '${expected.initialUrl}':`); instead?

brendankenny · 2018-04-10T17:41:45Z

lighthouse-cli/test/smokehouse/run-smoke.js

+
+/**
+ * Display smokehouse output from child process
+ * @param {{id: string, process?: NodeJS.Process, code?: number}} cp


s/cp/result

it would also be something like @param {{id: string, process: NodeJS.Process} || {id: string, error: Error & {stdout : NodeJS.WriteStream, stderr: NodeJS.WriteStream}}} result

but that might be getting a bit silly, especially with no type checking on this file :)

brendankenny · 2018-04-10T17:44:49Z

lighthouse-cli/test/smokehouse/run-smoke.js

+/**
+ * Run smokehouse in child processes for selected smoketests
+ * Display output from each as soon as they finish, but resolve function when ALL are complete
+ * @param {*} smokes


{id: string, expectations: string, config: string}

er, and perfSensitive?: boolean

brendankenny · 2018-04-10T17:49:04Z

lighthouse-cli/test/smokehouse/run-smoke.js

+  for (const {id, expectations, config} of smokes) {
+    // If the machine is terribly slow, do them in succession, not parallel
+    if (process.env.APPVEYOR) {
+      await Promise.all(cmdPromises);


yeah, it's a little confusing at first how this would work since it's Promise.all and at the top of the loop, though it's nice doing it in one line and with the same array as the parallel case :)

brendankenny · 2018-04-10T17:51:30Z

lighthouse-cli/test/smokehouse/run-smoke.js

+      `--config-path=${config}`,
+      `--expectations-path=${expectations}`,
+    ].join(' ');
+    const p = execAsync(cmd, {timeout: 5 * 60 * 1000, encoding: 'utf8'}).then(cp => {


maybe add a comment that the promise then/catch is on purpose so p resolves with process output no matter what?

brendankenny · 2018-04-10T18:10:31Z

package.json

@@ -23,7 +23,9 @@
    "build-viewer": "cd ./lighthouse-viewer && yarn build",
    "clean": "rimraf *.report.html *.report.dom.html *.report.json *.screenshots.html *.screenshots.json *.devtoolslog.json *.trace.json || true",
    "lint": "[ \"$CI\" = true ] && eslint --quiet -f codeframe . || eslint .",
-    "smoke": "bash lighthouse-cli/test/smokehouse/run-all-tests.sh",
+


these newlines won't survive a yarn add, fwiw

brendankenny · 2018-04-10T18:18:57Z

lighthouse-cli/test/smokehouse/run-smoke.js

+  } else {
+    smokes = SMOKETESTS.filter(test => argv.includes(test.id));
+    console.log(`Running ONLY smoketests for: ${smokes.map(t => t.id).join(' ')}\n`);
+  }


maybe add an error case that prints smokes ids?

brendankenny · 2018-04-10T18:19:45Z

lighthouse-cli/test/smokehouse/run-smoke.js

+  }
+
+  const parallelSmokes = SMOKETESTS.filter(t => !t.perfSensitive);
+  const serialSmokes = SMOKETESTS.filter(t => t.perfSensitive);


maybe I'm missing it, but aren't these still run in parallel?

brendankenny

LGTM! 🛤️🛤️🔀🛤️🛤️

brendankenny · 2018-04-10T23:33:58Z

lighthouse-cli/test/smokehouse/run-smoke.js

+/**
+ *
+ * @param {string[]} argv
+ * @return {Map<string, Array<SmoketestDfn>>}


string|undefined? or give a default batch name/symbol so it doesn't fall back to undefined

brendankenny · 2018-04-10T23:34:58Z

lighthouse-cli/test/smokehouse/run-smoke.js

+  }
+
+  // Split into serial batches that will run their tests concurrently
+  const batches = smokes.reduce((map, test) => {


is this still running the two perf batches in parallel?

currently there's a default batch (with like 7 smoketests) and a perf batch (with 2).

when perf batch goes, the 2 smoketests in there do run in parallel.

I was hoping that style was a little more clear with this "batch" framing.

brendankenny · 2018-04-10T23:37:30Z

lighthouse-cli/test/smokehouse/run-smoke.js

+async function runSmokehouse(smokes) {
+  const cmdPromises = [];
+  for (const {id, expectations, config} of smokes) {
+    console.log(`${purpleify(id)} smoketest starting…`);


for some reason, just for the perf batch, Travis does

not sure why :)

brendankenny · 2018-04-10T23:38:54Z

lighthouse-cli/test/smokehouse/run-smoke.js

+      const ret = {id: id, error: err};
+      displaySmokehouseOutput(ret);
+      return ret;
+    });


just to code golf, could also be

const p = execAsync(cmd, {timeout: 3 * 60 * 1000, encoding: 'utf8'}).then(cp => { return {id: id, process: cp}; }).catch(err => { return {id: id, error: err}; }).then(result => displaySmokehouseOutput(result));

(or the even shorter parenthesized version)

paulirish added 6 commits March 13, 2018 15:01

tests(smokehouse): run all run-tests.sh in parallel

3e4c0b8

smokehouse test run refactor

f7b52d5

disable 2 superfluous domsize smoketests.

ddb6c61

merge two smoketests evaluating the same page.

6468487

cleanup.

9774054

mprove logging

2711cc4

paulirish requested review from brendankenny and patrickhulce as code owners March 13, 2018 23:27

devtools-bot added the waiting4reviewer label Mar 13, 2018

brendankenny reviewed Mar 14, 2018

View reviewed changes

brendankenny added waiting4committer and removed waiting4reviewer labels Mar 14, 2018

devtools-bot added waiting4reviewer and removed waiting4committer labels Mar 14, 2018

paulirish mentioned this pull request Mar 17, 2018

tests(fixtures): introduce script to update the report fixtures #4793

Merged

paulirish added chillin ❄ and removed waiting4reviewer labels Mar 22, 2018

paulirish added 5 commits April 5, 2018 15:51

Merge branch 'master' into parallelizesmokehouse

67e3d1d

hello js smoketest runner.

4cb2e95

remove runtest.sh bs

d00198c

remove runtest.sh bs

21763d9

Merge branch 'master' into parallelizesmokehouse

6b3ae88

paulirish mentioned this pull request Apr 9, 2018

core(lantern): more flexible graph edge creation #4933

Merged

brendankenny requested changes Apr 9, 2018

View reviewed changes

paulirish added 3 commits April 9, 2018 17:32

cleanup.

80d3470

remove superfluous run from dbw smoketest.

ffa8983

cleanup + feedback

9ccedbb

paulirish force-pushed the parallelizesmokehouse branch from 703ff43 to 9ccedbb Compare April 10, 2018 00:53

paulirish added 2 commits April 9, 2018 20:58

Merge branch 'master' into parallelizesmokehouse

5eead99

cleanup + feedback

099237b

paulirish changed the title ~~tests(smokehouse): run all run-tests.sh in parallel (WIP)~~ tests(smokehouse): run smoketests in parallel Apr 10, 2018

paulirish added 2 commits April 9, 2018 18:38

Run some smoketests on their own, like perf or all on Appveyor.

6240a98

split pwa into pwa2 so they complete faster.

a499139

paulirish assigned brendankenny and patrickhulce Apr 10, 2018

patrickhulce approved these changes Apr 10, 2018

View reviewed changes

brendankenny removed the chillin ❄ label Apr 10, 2018

brendankenny requested changes Apr 10, 2018

View reviewed changes

brendankenny reviewed Apr 10, 2018

View reviewed changes

run smokehouse batches. feedback

a64b6eb

devtools-bot added the waiting4reviewer label Apr 10, 2018

paulirish added 2 commits April 10, 2018 15:50

re-enable shop.polymer smoketest

f47f562

shop.polymer smoketest: hacky workaround for #4898

f11021c

brendankenny approved these changes Apr 10, 2018

View reviewed changes

feedback.

93a6b94

paulirish merged commit 419b023 into master Apr 11, 2018

paulirish deleted the parallelizesmokehouse branch April 11, 2018 01:26

brendankenny mentioned this pull request Apr 13, 2018

tests(smoke): add smoke test code coverage #4967

Merged

paulirish removed the waiting4reviewer label Apr 30, 2018

		const results = runLighthouse(expected.initialUrl, configPath);
		console.log(`Comparing results of '${expected.initialUrl}':`);

		const results = runLighthouse(expected.initialUrl, configPath, cli.debug);

		console.log(`Auditing findings of '${expected.initialUrl}':`);

tests(smokehouse): run smoketests in parallel #4748

tests(smokehouse): run smoketests in parallel #4748

Conversation

paulirish commented Mar 13, 2018 • edited Loading

Changes

brendankenny left a comment

Choose a reason for hiding this comment

paulirish commented Mar 14, 2018

paulirish commented Mar 14, 2018

brendankenny commented Mar 15, 2018

paulirish commented Mar 15, 2018

wardpeet commented Mar 25, 2018

paulirish commented Mar 25, 2018

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

patrickhulce 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

brendankenny Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

paulirish commented Mar 13, 2018 •

edited

Loading

brendankenny Apr 10, 2018 •

edited

Loading