test(smokehouse): use static requires for test definitions #9501

connorjclark · 2019-08-02T22:19:44Z

I'm exploring creating a new bundle whose entry point is an interface for a Smoke test runner. This would eventually be used in a layout test in Chromium. It'd also bundle the test definitions, but to do that bundle.require is necessary to shim the expectations / configs loaded dynamically in smoke-test-dfns.js. See here for how the build script looks.

This is fine enough, but it made me think about how the smoke tests are defined. Doesn't seem to be much value in defining the paths and resolving them specially the way we do. I think doing it explicitly makes way more sense. Then no special require logic is necessary for bundling this code.

With this, some TypeScript gremlins were awakened. The types for most of the values in the expectations are wishy-washy, in that we accept RegExps anywhere and do strange stuff with arrays ({length: 1}). So I figured the values should be loose (any), and we should just verify that the keys make sense.

connorjclark · 2019-08-02T22:28:31Z

types/smokehouse.d.ts

@@ -19,17 +19,28 @@
    diff?: Difference | null;
  }

-  export type ExpectedLHR = Pick<LH.Result, 'audits' | 'finalUrl' | 'requestedUrl' | 'runtimeError'>
+  interface ExpectedLHR {
+    audits: Record<string, any>;


Is there a way to get all the audit ids? I tried something with import('../lighthouse-core/config/default-config') but couldn't get it to work.

why is it divorced from LH.Result at all?

Because we allow RegExps for any place a string is found, and {length:1} for arrays. Since the expectation files were changed to directly use this type now, it breaks if just LH.Result is used.

connorjclark · 2019-08-02T22:30:33Z

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

+ * @param {string} payloadPath
+ * @return {string}
+ */
+function resolveLocalOrProjectRoot(payloadPath) {


This actually still sticks arounds because the smoke runner needs a path to use with the LH CLI.

Instead, maybe the smoke runner could save a temporary file to disk, and pass that path? Does that sound better - temporary file creation vs this weird paths shenanigans?

could we leave it where it was then to avoid the unnecessary diff/blame churn?

Instead, maybe the smoke runner could save a temporary file to disk, and pass that path? Does that sound better - temporary file creation vs this weird paths shenanigans?

you know I support this :) this was one of my fav features of #8962

I didn't notice the file shenanigans you did there until now (I was just focused on the jest stuff), but yeah good to see this is supported by you too!

could we leave it where it was then to avoid the unnecessary diff/blame churn?

I am bad about deleting code when I think it's no longer needed, then adding it back when I realize I need it but putting it in a different place :(

connorjclark · 2019-08-02T22:31:11Z

Oh, you can test with this: npx browserify --s LH.SmokeTests lighthouse-cli/test/smokehouse/smoke-test-dfns.js -> paste in browser -> LH.SmokeTests

patrickhulce

Love the goal of smoketests for Chromium layout tests :)

This seems like a decent step in that direction though I'd be a bit bummed to see the structural change to smoketestdefns in a way that entrenches us further from #8962 (not saying we've hit that point yet, just throwing it out there as an internal measuring stick I'm using)

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

patrickhulce · 2019-08-05T15:38:44Z

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

+ * @param {string} payloadPath
+ * @return {string}
+ */
+function resolveLocalOrProjectRoot(payloadPath) {


could we leave it where it was then to avoid the unnecessary diff/blame churn?

Instead, maybe the smoke runner could save a temporary file to disk, and pass that path? Does that sound better - temporary file creation vs this weird paths shenanigans?

you know I support this :) this was one of my fav features of #8962

patrickhulce · 2019-08-05T15:44:06Z

lighthouse-cli/test/smokehouse/smokehouse.js

@@ -129,25 +110,27 @@ function runLighthouse(url, configPath, isDebug) {
 const cli = yargs
  .help('help')
  .describe({
-    'config-path': 'The path to the config JSON file',
-    'expectations-path': 'The path to the expected audit results file',
+    'smoke-id': 'The id of the smoke test to run',


honestly at this point I don't see a reason we couldn't send the entire config/expectations as a stringified argument, this halfway point where you have to resolve everything ahead of time and then it still has to know how to lookup the definitions feels weird

agreed ... So it's:

revert back to config/expectation paths and use tmp files as mentioned before

pass objects via CLI args

Is this smokehouse.js script only meant to be used programmatically via run-smoke.js? I'm not sure if others use it directly. If not, then we can change it without breaking anyone's workflow and do option 2.

Oh, smokehouse.js still needs a file path for the config, since it calls lighthouse-cli/index.js --config-path=.... Seems like we should just stick to paths since we can't change that.

But to further complicate things, writing the expectations to a temporary file means replacing/reviving RegExps.

OK, b/c of all the above I think this should just remain "smoke-id". Can still get rid of the config path/value thing though.

patrickhulce · 2019-08-05T15:44:46Z

types/smokehouse.d.ts

@@ -19,17 +19,28 @@
    diff?: Difference | null;
  }

-  export type ExpectedLHR = Pick<LH.Result, 'audits' | 'finalUrl' | 'requestedUrl' | 'runtimeError'>
+  interface ExpectedLHR {
+    audits: Record<string, any>;


why is it divorced from LH.Result at all?

patrickhulce · 2019-08-05T15:50:40Z

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

  id: 'a11y',
-  config: smokehouseDir + 'a11y/a11y-config.js',
-  expectations: 'a11y/expectations.js',
+  expectations: require('./a11y/expectations.js'),


static require paths for the smoke tests I'm all for, but the new {path, value} structure of config seems unnecessarily confusing. would it be possible to move to the model in #8962 of colocating the config/expectations and staticly require'ing in that file? IIUC that should alleviate the need for path here since we move to the transient config files :)

Let's drop the path/value thing in favor of writing temporary files for the config when passing to the smokehouse runner. That interface was the only reason path is needed.

Colocating the expectation/config seems like a bigger change to me and not one that's needed for static resolving to work.

connorjclark · 2019-08-05T16:09:25Z

FYI after a few nights of letting this settle in my mind, I'll say that the shimming workaround mentioned actually doesn't seem too bad to me anymore. So I wouldn't be against reverting most of these changes (and just keeping the type changes, which I believe is an improvement).

Will address @patrickhulce's comments now.

brendankenny

can you give a sketch of the architecture here now and how it'll be used by these two different clients? Smokehouse keeps sprouting new layers while it seems like we could be doing some simplification here and move incrementally closer to #8962

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

types/smokehouse.d.ts

connorjclark · 2019-08-06T17:00:19Z

Smokehouse keeps sprouting new layers while it seems like we could be doing some simplification here and move incrementally closer to #8962

I think this does some slight simplification (the resolving thing, although rather self-contained, was weird to reason about), but I hope there's more we can do.

can you give a sketch of the architecture here now and how it'll be used by these two different clients?

I'll list out how I see all three of these clients being used (including LR).

First, there's this smoke test runner interface that I linked to in the OP:

await Lighthouse.Firehouse.runSmokes({
  runLighthouse: async (url, config) => {
    const {lhr, artifacts} = await doSomething(url, config);
    return {lhr, artifacts};
  },
  skip: (test, expectation) => {
    // return string (an explanation) or `false`
  },
  modify(test, expectation) {
    // ...
  },
}

This iterates over every smoke test definition (in series), using the given function to run lighthouse in an environment-specific way, and using the other parameters to skip or modify smoke tests to match the environment. It also logs the results. This is what the current LR smoke tests do, and I found myself needing the same thing when running the smoke tests in DevTools.

Lightrider - currently does the above itself, requiring smoke-test-dfns.js and smokehouse-report.js. With the above interface, most of that goes away, it just requires firehouse.js (or whatever its named) and configures the options. These tests run in Node, so no concern re: browserifying. The fixture server is not needed locally, but is instead hosted in GAE.
DevTools - Run as a layout test, which means in a browser, so firehouse.js and its dependencies should be browser-friendly. Requires the fixture server to run locally. You can see the WIP layout test here.
lighthouse-cli - the normal way. This would not use the above smoke test runner, as it does some stuff that can't be generalized to fit that interface (the batching). It would continue to use smoke-test-dfns.js and smokehouse-report.js directly.

The Lightrider smoke tests are not fully-automated. They are meant to be run during every new roll while in staging, to notice any feature regressions due to the version of WRS that LR uses, or some other unexpected environment difference. Making it an automated test is possible with some additional work.

The DevTools smoke tests will also not be fully-automated at first. That will require running the fixture server during the layout tests, which either means figuring out how to run a node server in that test environment, or reimplementing it with the existing layout test runner features (or a python server, I guess). I see that as something that can be added later - I think using it as a manually-run test (just like Lightrider) is enough for a first pass. So the layout test won't actually run in the Chromium CI at first, but it will still leverage the test runner scripts that can be used locally.

…require

patrickhulce

With #8962 basically dead, I think this is a good step in the new, right direction of smoketests everywhere 👍

LGTM

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

…require

test(smokehouse): use static requires for test definitions

757e417

connorjclark requested review from brendankenny and patrickhulce as code owners August 2, 2019 22:19

googlebot added the cla: yes label Aug 2, 2019

assert

5924911

vercel bot deployed to staging August 2, 2019 22:24 View deployment

connorjclark commented Aug 2, 2019

View reviewed changes

lint

8acbcb1

vercel bot deployed to staging August 2, 2019 22:32 View deployment

patrickhulce reviewed Aug 5, 2019

View reviewed changes

tmp config path

a1bc19e

vercel bot deployed to staging August 5, 2019 16:45 View deployment

brendankenny reviewed Aug 6, 2019

View reviewed changes

lighthouse-cli/test/smokehouse/seo/expectations.js Outdated Show resolved Hide resolved

types/smokehouse.d.ts Outdated Show resolved Hide resolved

This was referenced Aug 27, 2019

Lighthouse 5.2.0 mistakes browser window width for device width in mobile audit #9476

Closed

misc(build): minify bundles with terser #9605

Merged

Merge remote-tracking branch 'origin/master' into smoke-tests-static-…

3756200

…require

vercel bot deployed to staging September 12, 2019 18:51 View deployment

testdfn

3236265

vercel bot temporarily deployed to staging October 1, 2019 06:05 Inactive

Merge remote-tracking branch 'origin/master' into smoke-tests-static-…

10cca97

…require

vercel bot deployed to staging October 1, 2019 06:06 View deployment

patrickhulce approved these changes Oct 1, 2019

View reviewed changes

lighthouse-cli/test/smokehouse/seo/expectations.js Outdated Show resolved Hide resolved

rm URLSearchParams stuff

f0f918e

vercel bot deployed to staging October 1, 2019 20:16 View deployment

connorjclark added 2 commits October 1, 2019 13:17

add type decl to new expectation files

20dd239

Merge remote-tracking branch 'origin/master' into smoke-tests-static-…

fbeae33

…require

vercel bot deployed to staging October 1, 2019 20:18 View deployment

lint

a83133f

vercel bot deployed to staging October 2, 2019 00:04 View deployment

connorjclark merged commit 93769d1 into master Oct 2, 2019

connorjclark deleted the smoke-tests-static-require branch October 2, 2019 18:08

connorjclark mentioned this pull request Oct 5, 2019

tests(build): use firehouse smoke test runner to test bundle #9791

Merged

paulirish pushed a commit that referenced this pull request Nov 6, 2019

test(smokehouse): use static requires for test definitions (#9501)

b171225

snyk-bot mentioned this pull request Mar 21, 2020

[Snyk] Upgrade lighthouse from 5.1.0 to 5.6.0 godaddy/lighthouse4u#13

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(smokehouse): use static requires for test definitions #9501

test(smokehouse): use static requires for test definitions #9501

connorjclark commented Aug 2, 2019 •

edited

Loading

connorjclark Aug 2, 2019

patrickhulce Aug 5, 2019

connorjclark Aug 5, 2019

connorjclark Aug 2, 2019

patrickhulce Aug 5, 2019

connorjclark Aug 5, 2019

connorjclark commented Aug 2, 2019

patrickhulce left a comment

patrickhulce Aug 5, 2019

patrickhulce Aug 5, 2019

connorjclark Aug 5, 2019

connorjclark Aug 5, 2019

connorjclark Aug 5, 2019

connorjclark Aug 5, 2019 •

edited

Loading

patrickhulce Aug 5, 2019

patrickhulce Aug 5, 2019

connorjclark Aug 5, 2019

connorjclark commented Aug 5, 2019

brendankenny left a comment

connorjclark commented Aug 6, 2019

patrickhulce left a comment

test(smokehouse): use static requires for test definitions #9501

test(smokehouse): use static requires for test definitions #9501

Conversation

connorjclark commented Aug 2, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Aug 2, 2019

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

connorjclark Aug 5, 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 commented Aug 5, 2019

brendankenny left a comment

Choose a reason for hiding this comment

connorjclark commented Aug 6, 2019

patrickhulce left a comment

Choose a reason for hiding this comment

connorjclark commented Aug 2, 2019 •

edited

Loading

connorjclark Aug 5, 2019 •

edited

Loading