misc: smoketests reimagined #8962

patrickhulce · 2019-05-15T16:26:36Z

Summary
Per #8190, @brendankenny thinks we should move all our smoke testing to jest (😉) and I whole-heartedly agree!

This is an example of what it might look like to be completely jest-driven.

All our existing junk goes away and all we have is a single jest-test file smokes-test.js and our smoketest definitions *.smoke.js.

patrickhulce · 2019-05-15T16:27:20Z

lighthouse-cli/test/lantern.smoke.js

+module.exports = [
+  {
+    batch: 'performance',
+    url: 'http://localhost:10200/online-only.html',


config, url, batch, and assertions are all colocated, no more jumping around to 5 files! 🎉

patrickhulce · 2019-05-15T16:27:34Z

lighthouse-cli/test/lantern.smoke.js

+    url: 'http://localhost:10200/online-only.html',
+    config,
+    /** @param {() => Promise<Smokehouse.RunResult>} getResults */
+    describe(getResults) {


smoke assertions are just like writing a regular jest test

patrickhulce · 2019-05-15T16:28:00Z

lighthouse-cli/test/lantern.smoke.js

+      it('should compute the metric values using precomputedLanternData', async () => {
+        const {lhr} = await getResults();
+        expect(lhr.audits['first-contentful-paint'].numericValue).toBeGreaterThan(2000);
+        expect(lhr.audits['first-cpu-idle'].numericValue).toBeGreaterThan(2000);


hurray for arbitrary assertion complexity! :D

patrickhulce · 2019-05-15T16:29:31Z

lighthouse-cli/test/smokes-test.js

+
+  /** @type {Array<{label: string, definitions: any[]}>} */
+  const batches = []
+  for (const [batchName, definitions] of groupedByBatch.entries()) {


this does automatic smarts that we manually do currently for batches

batches are all run serially between, parallel within

performance batch means 1 run = 1 batch

everything else gets put into an automatic batch of MAX_PARALLEL_SMOKES

patrickhulce · 2019-05-15T16:29:50Z

lighthouse-cli/test/smokes-test.js

+      it(`should collect results for the ${batch.label} batch`, async () => {
+        await Promise.all(batch.definitions
+          .map(async definition => {
+            const results = await runLighthouse(definition.url, definition.config)


runLighthouse is the only existing code we would need to bring over and tweak to run async

connorjclark · 2019-05-15T17:48:13Z

Skipping jest tests should be easy in Smokerider, but moving from the data-driven approach to a jest-driven approach will make "augmenting" results way more difficult. Some augments are temporary, until the underlying issue is resolved. Others are necessary due to differences in environments (example, all URLs in Smokerider are https on GAE, because many web features we use in these tests require secure connection, either localhost or https).

I think if we continue with this approach, we should make the smoke tests pretty granular (so that skipping a failing test doesn't take other good assertions with it). And also be okay with less coverage in LR (or doing super hacky stuff like regexing jest code).

connorjclark · 2019-05-15T17:52:32Z

Outside of Smokerider considerations...

I like how it's all co-located. much better.

I don't like moving everything to jest. Some things are just too simple! Is the goal to get better messaging when a test fails (by grouping assertions with a jest test name), or to offer more flexibility?

What of a hybrid approach - keeping the current assertion data-model (ugh what is a good name for this) + allowing for more flexible jest tests?

brendankenny · 2019-05-15T19:05:19Z

Things I'd like to see preserved from current behavior:

calling the CLI version in a sub process so we get the full end-to-end experience test. I think we'd have to use --config-path in that case, though?
yarn smoke pwa, yarn smoke metrics lantern, etc. Jest file matching will be a step down though I can live with it as long as it's possible to filter like that
some of the dumber assertions I'm ok with doing an expect(lhr.audits['is-on-https'].details.items.length).toEqual(4);, but I personally prefer the big object that resembles the actual LHR approach to our smoke tests (not sure if others actually like it :).

I guess I was imagining something similar to the status quo and using toMatchObject to replace our custom differ (and toMatchObject allows using matchers in place of literal values, so we can do things like items: expect.arrayContaining([{statistic: 'Total DOM Elements', value: '34'}]) and not worry about array order, or warnings: [expect.stringMatching(/Unable to determine.*<a target="_blank">/)], etc).

Doing it that way would allow smokerider to still override specific fields, though that also might have to move to a separate file to keep that easy to do 😟

brendankenny · 2019-05-15T19:16:09Z

I don't like moving everything to jest. Some things are just too simple! Is the goal to get better messaging when a test fails (by grouping assertions with a jest test name), or to offer more flexibility?

👍 With jest getting more heavy handed ("opinionated!") in every release (e.g. AFAICT everything goes through babel before testing now, even if you don't need it, like us), I'd like to keep the door open for a hypothetical future exit that's as easy as the mocha->jest one was. I don't see any danger for that in this PR, but something to keep in mind :)

though that also might have to move to a separate file to keep that easy to do 😟

Another motivator that might have us seeing different approaches is the colocation thing :) It's never really bothered me to have separate expectations/config/test-runner files, though I do think if we keep them separate the test directories are in desperate need of reorganizing so that the files that go together clearly go together.

patrickhulce · 2019-05-15T19:23:21Z

Thanks for the feedback! And alright I hear ya I can dig preserving some of the data driven approach.

What about

interface RunDefinition {
  url: string
  config: LH.Config
   // for asserting like today but with `toMatchObject` and jest matchers like @brendankenny proposed
  assertions?: LHR
  // for asserting things that are impossible today and don't fit the mold
  describe?(getResults: () => Promise<{lhr: LHR, artifacts: LH.Artifacts}>)
}

Smokerider considerations

I legit forgot about smokerider before putting this up, oops 😳, so hopefully the assertions piece helps a bit with that? Definitely want to find something that can be workable

What of a hybrid approach - keeping the current assertion data-model (ugh what is a good name for this) + allowing for more flexible jest tests?

Definitely, that's how this all started anyway :)

I just want to be able to do some more assertive testing against artifacts with jest and unifying how they get run seems like a great opportunity to clean up both!

calling the CLI version in a sub process so we get the full end-to-end experience test. I think we'd have to use --config-path in that case, though?

yeah for sure we'd still do that here, just have runLighthouse write the config to a tmp path so it can still be defined inline

yarn smoke pwa

ya for sure this is easy to still keep we just filter the auto-discovery of smoke files

I personally prefer the big object that resembles the actual LHR approach to our smoke tests

see assertions proposal :)

connorjclark · 2019-05-20T20:04:56Z

lighthouse-cli/test/lantern.smoke.js

+const config = {
+  extends: 'lighthouse:full',
+  settings: {
+    onlyCategories: ['performanceormance'],


connorjclark · 2019-05-20T20:06:28Z

SGTM.

fyi most of the assertions you've initially converted to be jest fns would ideally be in the assertions property

patrickhulce · 2019-05-20T21:20:16Z

fyi most of the assertions you've initially converted to be jest fns would ideally be in the assertions property

Yeah I didn't update yet just waiting for the 👍 , but since you and brendan like the new plan. I'll do them now!! 🎉

patrickhulce · 2019-05-21T20:16:02Z

OK moving out of draft since it's mostly a real PR now.

0a92440

is the commit that makes it easiest to see what the required changes to runLighthouse were. If it looks good, I'll probably convert one more smoke that utilizes describe in this PR and call it a good stopping point for merge before continuing?

connorjclark · 2019-05-21T20:53:50Z

I'll pull into LR and see if Smokerider still works.

connorjclark · 2019-05-21T21:01:28Z

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

@@ -69,11 +69,6 @@ const SMOKE_TEST_DFNS = [{
  expectations: 'perf/expectations.js',
  config: 'perf/perf-config.js',
  batch: 'perf-metric',
-}, {


Currently, Smokerider uses smoke-test-dfns as a super easy way to get a full description of all the tests, modify expectations, runs each URL, and then uses {collateResults, report} on the results.

Gotta figure out how to do that with your *.smoke.js changes ...

I suppose I would import every *.smoke.js? Maybe we could have a function that does that and returns an array of everything?

Actually, don't bother with that, I'll do that on my side. Not sure if that'll be the best approach yet.

that's basically what https://github.com/GoogleChrome/lighthouse/pull/8962/files#diff-b7fef6a2e94d6698d40bf02b3fb67d49R17 is doing

happy to make this a shared function in a separate file if it would help (will need to modify it to process argv for the filtering logic later anyhow)

sure, let's do it

connorjclark

LGTM

patrickhulce · 2019-05-29T21:01:24Z

@brendankenny did you want final approval on this? I figured you would prefer me not implementing everything discussed and this was an adequate start :)

brendankenny · 2019-05-30T07:05:44Z

yes pls :)

patrickhulce · 2019-06-03T19:51:28Z

yes pls :)

I interpreted this to mean you would like to approve this before landing, correct?

connorjclark · 2019-07-01T23:39:54Z

wut's going on with dis

patrickhulce · 2019-07-25T20:14:17Z

I don't think this will ever see approval, so just closing.

misc: smoketests reimagined

78868c6

patrickhulce commented May 15, 2019

View reviewed changes

brendankenny mentioned this pull request May 20, 2019

tests: add artifact snapshot testing #8190

Closed

connorjclark reviewed May 20, 2019

View reviewed changes

patrickhulce added 4 commits May 20, 2019 18:48

smoketest updates

843d0ce

copy smokehouse.js for easier review diffs

122706b

tweak runLighthouse for dual usage

0a92440

make it actually work

1b6f4bc

vercel bot deployed to staging May 21, 2019 17:03 View deployment

lint

27a20b9

vercel bot deployed to staging May 21, 2019 17:04 View deployment

update for appveyor

2313ba7

patrickhulce marked this pull request as ready for review May 21, 2019 20:14

patrickhulce requested review from brendankenny and paulirish as code owners May 21, 2019 20:14

vercel bot deployed to staging May 21, 2019 20:14 View deployment

connorjclark reviewed May 21, 2019

View reviewed changes

connorjclark approved these changes May 21, 2019

View reviewed changes

Merge branch 'master' into jest_smokes

6da6799

vercel bot deployed to staging May 28, 2019 21:34 View deployment

Merge branch 'master' into jest_smokes

552762b

googlebot added the cla: yes label May 29, 2019

coverage support

e401b40

vercel bot deployed to staging May 29, 2019 21:14 View deployment

patrickhulce mentioned this pull request Jun 6, 2019

tests(smokehouse): always assert lhr.runtimeError #9130

Merged

connorjclark mentioned this pull request Jul 19, 2019

smokehouse: assertions based on other audits #9417

Open

patrickhulce closed this Jul 25, 2019

patrickhulce mentioned this pull request Aug 5, 2019

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

Merged

brendankenny mentioned this pull request Oct 14, 2019

tests: refactor smokehouse for extensibility #9843

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

misc: smoketests reimagined #8962

misc: smoketests reimagined #8962

patrickhulce commented May 15, 2019

patrickhulce May 15, 2019

patrickhulce May 15, 2019

patrickhulce May 15, 2019

patrickhulce May 15, 2019

patrickhulce May 15, 2019

connorjclark commented May 15, 2019

connorjclark commented May 15, 2019

brendankenny commented May 15, 2019

brendankenny commented May 15, 2019

patrickhulce commented May 15, 2019

connorjclark May 20, 2019

connorjclark commented May 20, 2019 •

edited

Loading

patrickhulce commented May 20, 2019

patrickhulce commented May 21, 2019

connorjclark commented May 21, 2019

connorjclark May 21, 2019 •

edited

Loading

connorjclark May 21, 2019 •

edited

Loading

connorjclark May 21, 2019

patrickhulce May 21, 2019

connorjclark May 21, 2019

connorjclark left a comment

patrickhulce commented May 29, 2019

brendankenny commented May 30, 2019

patrickhulce commented Jun 3, 2019

connorjclark commented Jul 1, 2019

patrickhulce commented Jul 25, 2019

misc: smoketests reimagined #8962

misc: smoketests reimagined #8962

Conversation

patrickhulce commented May 15, 2019

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 15, 2019

connorjclark commented May 15, 2019

brendankenny commented May 15, 2019

brendankenny commented May 15, 2019

patrickhulce commented May 15, 2019

Choose a reason for hiding this comment

connorjclark commented May 20, 2019 • edited Loading

patrickhulce commented May 20, 2019

patrickhulce commented May 21, 2019

connorjclark commented May 21, 2019

connorjclark May 21, 2019 • edited Loading

Choose a reason for hiding this comment

connorjclark May 21, 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

patrickhulce commented May 29, 2019

brendankenny commented May 30, 2019

patrickhulce commented Jun 3, 2019

connorjclark commented Jul 1, 2019

patrickhulce commented Jul 25, 2019

connorjclark commented May 20, 2019 •

edited

Loading

connorjclark May 21, 2019 •

edited

Loading

connorjclark May 21, 2019 •

edited

Loading