tests: add smokehouse to bin for downstream use #12446

brendankenny · 2021-05-05T19:58:30Z

follow up to #12415 and #12325

Relatively unimportant improvement given the lack of users of this, but makes the experience of running smoke tests for downstream projects (publisher-ads only?) a lot better. This lets us do our release testing of a pristine packed lighthouse more cleanly too, though (which also acts as a test that we're not breaking testing for downstream projects...).

The changes end up a little tangled to accomplish this, but starting at the entry point:

expose a smokehouse bin script so downstream doesn't have to reach in to find it. Packages with lighthouse as a dep can run it with yarn smokehouse or npx --no-install smokehouse
since this becomes part of our public interface, move lodash.clonedeep to deps
since static-server can only serve the core smoke tests by design, dynamically require and start it only if running the core tests (and officially add mime-types (used by static-server) to our devDeps)
move release/test.sh to use this entry point with the pristine checkout's static-server and tests
make takeNetworkRequestUrls optional after all (see original discussion to make it required). For users of smokehouse-bin that aren't using the core tests (like publisher-ads and us during release time), they can't pass in that function (since it's a CLI run), and it turns out we're not interested in implementing network request collection for smokerider at the current time anyways :)
since we have two bits to pipe through for deciding whether or not to assert networkRequests now, and smokehouse-bin is the only place that really knows about everything going on, move the pruning of those out to smokehouse-bin so the assertions on expectations can go back to being a little simpler. I went back and forth about the location of this last one...happy to discuss.

brendankenny · 2021-05-05T20:00:46Z

.npmignore

-lighthouse-cli/test/smokehouse/test-definitions
+lighthouse-cli/test/*
+!lighthouse-cli/test/smokehouse/
+lighthouse-cli/test/smokehouse/test-definitions/


almost the exact same thing, just a little more explicitly what the comment describes, and a little more future proof from accidentally including things added to lighthouse-cli/test/ in the future. The only difference today is that lighthouse-cli/test/static-server-test.js is no longer in our published package (a whole 1.5 KiB savings :)

brendankenny · 2021-05-05T20:07:28Z

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

+  const clonedDefns = cloneDeep(testDefns);
+  for (const {id, expectations, runSerially} of clonedDefns) {
+    for (const expectation of expectations) {
+      if (!runSerially && expectation.networkRequests) {


In the current version, if a test is runSerially: false but has a networkRequests expectation, only a warning is printed when run locally, but it's a fatal error in CI. I believe this is due to my suggestions, since originally I was thinking we always run smoke serially in CI with -j=1, so we'll always get the test one way or another.

I eventually came around to @adamraine's thinking that we should really be looking at only the individual tests's runSerially value so that there's no surprises depending on if running locally or in CI (and if running with -j=1 or not), so this is just making that concrete. No one should add networkRequests expectations unless they're assertable in all environments :)

brendankenny · 2021-05-05T20:08:31Z

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

+  let server;
+  let serverForOffline;
+  let takeNetworkRequestUrls = undefined;
+
  try {


this try/finally makes everything pretty ugly. Happy to take 🏌️ suggestions :)

brendankenny · 2021-05-05T20:09:39Z

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

+      ({server, serverForOffline} = require('../../fixtures/static-server.js'));
+      server.listen(10200, 'localhost');
+      serverForOffline.listen(10503, 'localhost');
+      takeNetworkRequestUrls = server.takeRequestUrls.bind(server);


takeNetworkRequestUrls = () => server.takeRequestUrls() unfortunately makes tsc lose the server type from the closure + the conditional setting it to undefined in the other branch. This is good enough.

brendankenny · 2021-05-05T20:11:10Z

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

@@ -54,7 +54,7 @@ async function internalRun(url, tmpPath, configJson, isDebug) {
  const artifactsDirectory = `${tmpPath}/artifacts/`;

  const args = [
-    'lighthouse-cli/index.js',
+    `${__dirname}/../../../index.js`, // 'lighthouse-cli/index.js'


needs to be absolute so it works from inside node_modules/lighthouse/, otherwise it tries to run it from the root of the parent project (this is why publisher ads had to change the cwd for their tests)

brendankenny · 2021-05-05T20:13:34Z

lighthouse-core/scripts/release/test.sh

 npm explore lighthouse -- npm run fast -- http://example.com

+# Start up pristine's static-server (and kill it on exit).
+node "$LH_PRISTINE_ROOT/lighthouse-cli/test/fixtures/static-server.js" &
+trap "trap - SIGTERM && kill -- -$$" SIGINT SIGTERM EXIT


this is the only way I could get it to kill static-server since it's launching another process of its own (but it's not a child of it? I have no idea but it just wouldn't die) so e.g. saving the node static-server pid and then killing that wasn't sufficient. Happy to take suggestions from people who know much better what they're doing here, though :)

this is the strongest argument I've seen yet for why we should just have an option allow the static server with --fixture-path :)

--experimental-super-dangerous-server-dir if we really don't want others to rely on its query params?

this is the strongest argument I've seen yet for why we should just have an option allow the static server with --fixture-path :)

well let's not go too wild based just on me not understanding how signals in process trees work :P I was hoping to nerd snipe someone into it, but @paulirish's on vacation and you and @connorjclark didn't take the bait :)

I moved up the static-server start to before the directory change, so at least that part can be the much cleaner yarn static-server &.

...that is an option though. Are we going to care much about this line after it lands until we next look at it in like 18 months from now, though? OTOH, static-server is definitely ready for a cleanup pass regardless of this (fs.exists has been deprecated since node 1.0!), and it might not be so bad to add at the same time.

not understanding how signals in process trees work

Haha, you're not alone here that's why I thought it was a strong argument to avoid dealing with them for all our sakes :)

I didn't jump in because I didn't think I understood the problem. I didn't see where static-server starts child processes, so I was surprised that this would be a thing.

yarn static-server & kill $!

worked just fine locally for me, so couldn't figure out where I could be sniped 🤷

I believe you have to give it a second to start up, so stick a sleep 5 or something in between

Yeah, I was trying something similar and see the same behavior with that script, two zombie node processes (I swear it was one before, but definitely two now, I assume one for each http.Server).

But looking into the process tree now, I think the problem might be that I'm using volta for managing node versions and the issue might be volta-cli/volta#36, which is annoying I didn't think of that long before :)

Maybe I'll leave the overkill kill for now but also leave a comment on it?

(the last comment in that issue is what makes it seem most likely, because ctrl-c does kill the node processes even without a trap)

Ah, ok. Yeah I'd vote regular kill then 👍

maybe detect a volta installation and conditionally do the overkill / normal kill? effectively the same behavior but more self documenting.

Well it's overkill for anyone not using volta, but it's not like it's doing anything bad, everything should still work as expected. It's just killing its own process group so all child processes are taken out instead of a child process + any of its children. My understanding is the first part is to prevent trap recursion in recent bash.

connorjclark · 2021-05-05T20:11:16Z

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


-const coreTestDefnsPath = `${__dirname}/../test-definitions/core-tests.js`;
+const coreTestDefnsPath = path.join(__dirname, '../test-definitions/core-tests.js');


I'm confused, this change seems out of character for you :)

I'm confused, this change seems out of character for you :)

I actually laughed out loud when I saw you wrote that line in #12415 and I was very sorry to change it :D

It's the testDefnPath === coreTestDefnsPath comparison that trips it up, since it ends up being lighthouse-cli/test/smokehouse/test-definitions/core-tests.js compared to lighthouse-cli/test/smokehouse/frontends/../test-definitions/core-tests.js, so it needs to be normalized, and this was the easiest way 😿

Patrick adds another example to the (admittedly short) "do you know when it will matter?" list 😉

connorjclark · 2021-05-05T20:12:45Z

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

-    serverForOffline.listen(10503, 'localhost');
-    isPassing = (await runSmokehouse(testDefns, options)).success;
+    // If running the core tests, spin up the test server.
+    if (testDefnPath === coreTestDefnsPath) {


maybe we should expose the static server but make the folder that is served configurable? altho we'd be making the static server query params and such part of public interface, maybe not worth it

maybe we should expose the static server but make the folder that is served configurable? altho we'd be making the static server query params and such part of public interface, maybe not worth it

yeah, looking at the publisher-ads script just running their own server convinced me it wasn't worth it. I think most people would never need all the many options we have in there, so using a third-party one seems like an easier path for most.

connorjclark · 2021-05-05T20:17:18Z

cc @jburger424

patrickhulce

drive by :)

neat!

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

patrickhulce · 2021-05-05T21:16:57Z

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

-    isPassing = (await runSmokehouse(testDefns, options)).success;
+    // If running the core tests, spin up the test server.
+    if (testDefnPath === coreTestDefnsPath) {
+      ({server, serverForOffline} = require('../../fixtures/static-server.js'));


I think this is the first occurrence we've ever had of declarationless destructuring! 🎉

oh you can do this?! Just need parentheses... nice.

patrickhulce · 2021-05-05T21:22:42Z

lighthouse-core/scripts/release/test.sh

 npm explore lighthouse -- npm run fast -- http://example.com

+# Start up pristine's static-server (and kill it on exit).
+node "$LH_PRISTINE_ROOT/lighthouse-cli/test/fixtures/static-server.js" &
+trap "trap - SIGTERM && kill -- -$$" SIGINT SIGTERM EXIT


this is the strongest argument I've seen yet for why we should just have an option allow the static server with --fixture-path :)

--experimental-super-dangerous-server-dir if we really don't want others to rely on its query params?

adamraine

LGTM

tests: add smokehouse to bin for downstream use

a13c839

brendankenny requested a review from adamraine May 5, 2021 19:58

brendankenny requested a review from a team as a code owner May 5, 2021 19:58

google-cla bot added the cla: yes label May 5, 2021

devtools-bot assigned adamraine May 5, 2021

devtools-bot added the waiting4reviewer label May 5, 2021

brendankenny commented May 5, 2021

View reviewed changes

connorjclark reviewed May 5, 2021

View reviewed changes

patrickhulce reviewed May 5, 2021

View reviewed changes

msg and cleaner static-server

a746b01

vercel bot deployed to Preview May 5, 2021 21:54 View deployment

adamraine approved these changes May 6, 2021

View reviewed changes

add comment

790068a

vercel bot deployed to Preview May 10, 2021 16:18 View deployment

brendankenny added the land-when-ci-is-green label May 10, 2021

devtools-bot merged commit 17e9a13 into master May 10, 2021

devtools-bot deleted the smokehousebin branch May 10, 2021 22:00

devtools-bot removed the land-when-ci-is-green label May 10, 2021

paulirish pushed a commit that referenced this pull request May 13, 2021

tests: add smokehouse to bin for downstream use (#12446)

247960f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: add smokehouse to bin for downstream use #12446

tests: add smokehouse to bin for downstream use #12446

brendankenny commented May 5, 2021 •

edited

Loading

brendankenny May 5, 2021

brendankenny May 5, 2021

brendankenny May 5, 2021

brendankenny May 5, 2021

brendankenny May 5, 2021

brendankenny May 5, 2021

patrickhulce May 5, 2021

brendankenny May 5, 2021

patrickhulce May 6, 2021

brendankenny May 6, 2021 •

edited

Loading

brendankenny May 6, 2021 •

edited

Loading

brendankenny May 6, 2021

patrickhulce May 6, 2021

connorjclark May 6, 2021 •

edited

Loading

brendankenny May 10, 2021 •

edited

Loading

connorjclark May 5, 2021

brendankenny May 5, 2021

patrickhulce May 5, 2021

connorjclark May 5, 2021

brendankenny May 5, 2021

connorjclark commented May 5, 2021

patrickhulce left a comment

patrickhulce May 5, 2021

connorjclark May 5, 2021

patrickhulce May 5, 2021

adamraine left a comment


		const coreTestDefnsPath = `${__dirname}/../test-definitions/core-tests.js`;
		const coreTestDefnsPath = path.join(__dirname, '../test-definitions/core-tests.js');

tests: add smokehouse to bin for downstream use #12446

tests: add smokehouse to bin for downstream use #12446

Conversation

brendankenny commented May 5, 2021 • 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

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 May 6, 2021 • edited Loading

Choose a reason for hiding this comment

brendankenny May 6, 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 May 6, 2021 • edited Loading

Choose a reason for hiding this comment

brendankenny May 10, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented May 5, 2021

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

adamraine left a comment

Choose a reason for hiding this comment

brendankenny commented May 5, 2021 •

edited

Loading

brendankenny May 6, 2021 •

edited

Loading

brendankenny May 6, 2021 •

edited

Loading

connorjclark May 6, 2021 •

edited

Loading

brendankenny May 10, 2021 •

edited

Loading