tests(smokehouse): filter tests by url, print rerun command #9676

connorjclark · 2019-09-14T00:15:43Z

EDIT: just doing --only-urls in this PR.

[original message]

Two new flags to run a subset of smoke tests: --only-audits and --only-urls

cjamcl@cjamcl:~/code/lighthouse$ yarn smoke --help
yarn run v1.12.3
$ node lighthouse-cli/test/smokehouse/run-smoke.js --help
Options:
  --help         Show help                                                                                      [boolean]
  --only-audits  Filter for audit expectations to run                                                             [array]
  --only-urls    Filter for urls to run. Patterns accepted                                                        [array]

Examples:
  yarn smoke --only-audits network-requests                   Only run tests for the network-request audit
  yarn smoke --only-urls http://localhost:10200/preload.html  Only run tests for http://localhost:10200/preload.html

Also, if one of the batches fails, a command to rerun the failures is printed:

patrickhulce

neat! I'm suspecting this might have a few bad interactions though with some of the audits that are added just so we can assert something on artifacts and whatnot, but I don't have great ideas for that

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

lighthouse-cli/test/smokehouse/smokehouse.js

Co-Authored-By: Patrick Hulce <[email protected]>

connorjclark · 2019-09-16T19:54:37Z

neat! I'm suspecting this might have a few bad interactions though with some of the audits that are added just so we can assert something on artifacts and whatnot, but I don't have great ideas for that

good point. should be good now.

brendankenny

Is this serving a current need? Each smokehouse definition is pretty short on its own, typically is limited to audits that will individually trigger the same number of passes anyway, and I don't think I'm ever going to remember one of the URLs off the top of my head, if only for the port numbers alone :)

This seems more suited for a different definition setup, where each smoke test runs the full default config over each test page instead of each being pruned by hand (which may or may not be sustainable much longer, who knows).

connorjclark · 2019-09-16T20:24:54Z

Is this serving a current need?
being pruned by hand

Investigating any specific failure in the smoke tests requires pruning away what you don't want, which has become a step I really dislike.

and I don't think I'm ever going to remember one of the URLs off the top of my head

The bit that spits out the exact command to re-run the failures doesn't require remembering any URLs.

brendankenny · 2019-09-16T20:49:52Z

Investigating any specific failure in the smoke tests requires pruning away what you don't want, which has become a step I really dislike.

audit pruning doesn't seem like it would help, though? They're already silenced if they're passing.

If an expectations file has more than one url in it is annoying if the failure is in e.g. the last one, that's a good point (though we don't require URLs to be unique)

patrickhulce

not sure what the status of this effort is, but I'm fine either way. seems useful to me but I understand not churning smoke tests too 🤷‍♂

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

patrickhulce · 2019-09-23T21:40:30Z

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

      `--expectations-path=${expectations}`,
-    ].join(' ');
+    ];
+    if (argv.onlyAudits) commandParts.push(`--only-audits ${argv.onlyAudits.join(' ')}`);


should these be pushed separately? feels weird to double join with " "

paulirish · 2020-08-05T20:51:46Z

been a while but came here because i really want url filtering.

Investigating any specific failure in the smoke tests requires pruning away what you don't want, which has become a step I really dislike.

...

If an expectations file has more than one url in it is annoying if the failure is in e.g. the last one, that's a good point (though we don't require URLs to be unique)

yeah i do this all the time. i comment out the other URLs (and their expectations) in an expectation file. super annoying.

(I care less about only-audits and currently i dont think i'd really use it)

one thought: going in another direction, maybe each expectation file should only be for a single URL. we currently have 39 URLs we load. and they're defined across 16 files. (the 16 are selectable as 'pwa', 'dbw' 'pwa2', etc etc). does anyone feel like it should be 1 file per tested LH run?

connorjclark · 2021-01-13T00:42:01Z

I'm more open to "one url per expectation file" now, but there is the extra complication that some files (like seo/expectations.js) have a shared bit of code between each url declaration.

I suggest opening a new issue to talk more about it / schedule a talk during eng sync.

I will update this PR now.

patrickhulce

this is a nice intermediate step, thanks for fixing up :)

patrickhulce · 2021-01-13T03:16:38Z

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

  }

-  const exitCode = isPassing ? 0 : 1;
+  const failingUrls = testResults.testResults


should the variable be something like smokehouseResult? testResults.testResults twitches my nose a bit :)

patrickhulce · 2021-01-13T03:19:40Z

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

    console.log(usage);
  }

+  if (onlyUrls) {


interaction with invertMatch is a little confusing now, maybe warrants some documentation on the method and/or CLI help?

long-term fix might be a skipUrls parallel :)

brendankenny

This is relatively straightforward, but it's also treating url as an id, so every test has two IDs. This results in things like every smokehouse result being double keyed and being confusing which ID you need to look at to evaluate success (at the expectation level or the url level), the unclear interplay between --invert-match and --only-urls, etc.

It's a slightly more painful transition, but it does seem like @paulirish is right that making a change to 1 ID <=> 1 test solves all this more simply.

brendankenny · 2021-01-13T04:06:45Z

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

+    smokes = smokes.filter(smoke => {
+      smoke.expectations = smoke.expectations.filter(expectation => {
+        const url = expectation.lhr.requestedUrl;
+        const shouldRunUrl = onlyUrls.some(pattern => new RegExp(pattern).test(url));


new RegExp(pattern)

it seems like this would be a problem with some of the gnarlier URLs when c/p verbatim

new RegExp(pattern)

it seems like this would be a problem with some of the gnarlier URLs when c/p verbatim

actually they look mostly ok, it's just the test urls with querystrings that break it, since ? won't match itself in new RegExp(urlWithQuery).test(urlWithQuery)

(are there other metacharacters that aren't usually url encoded that could make their way into one of our test URLs?)

I asked my friend SO for help.

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

brendankenny · 2021-01-13T04:17:21Z

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

+      'node',
+      path.relative(process.cwd(), __filename),
+      // This is the good bit.
+      `--only-urls ${failingUrls.join(' ')}`,


ampersands, at least, are an issue for me without quoting

fixed with a child_process hack. too bad it doesn't expose a function "give me formatted command".

lighthouse-cli/test/smokehouse/smokehouse.js

connorjclark · 2021-08-16T21:57:08Z

this is mostly replaced by the each-url-a-test refactor, but I would like to close and follow up with a small change that does the "print a command to rerun failures" thing this PR did.

connorjclark added 3 commits September 13, 2019 16:15

add filter flags

b268f8a

update

986828b

help

1c3179d

connorjclark requested review from brendankenny and patrickhulce as code owners September 14, 2019 00:15

googlebot added the cla: yes label Sep 14, 2019

vercel bot deployed to staging September 14, 2019 00:15 View deployment

connorjclark changed the title ~~tests(smokehouse): filter which tests runs~~ tests(smokehouse): filter tests Sep 14, 2019

log error

14ff24d

vercel bot deployed to staging September 14, 2019 00:25 View deployment

patrickhulce reviewed Sep 14, 2019

View reviewed changes

lighthouse-cli/test/smokehouse/run-smoke.js Outdated Show resolved Hide resolved

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

typo

cd2c31f

vercel bot temporarily deployed to staging September 16, 2019 19:33 Inactive

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

2f205b7

Co-Authored-By: Patrick Hulce <[email protected]>

vercel bot deployed to staging September 16, 2019 19:34 View deployment

account for artifacts

efe5727

vercel bot deployed to staging September 16, 2019 19:54 View deployment

brendankenny reviewed Sep 16, 2019

View reviewed changes

patrickhulce reviewed Sep 23, 2019

View reviewed changes

pr

dd49651

vercel bot temporarily deployed to staging September 23, 2019 22:32 Inactive

fix comment

2664138

vercel bot deployed to staging September 23, 2019 22:33 View deployment

connorjclark mentioned this pull request Sep 25, 2019

tests(smokehouse): snapshot stderr #9733

Closed

Merge remote-tracking branch 'origin/master' into smoke-cli

977dbb7

vercel bot deployed to staging October 8, 2019 21:35 View deployment

paulirish added the waiting4committer label Sep 28, 2020

paulirish mentioned this pull request Jan 13, 2021

tests(smoke): add category to run some perf tests in parallel #11932

Merged

paulirish mentioned this pull request Jan 13, 2021

smoketests proposal: one test file per url #11950

Closed

Merge remote-tracking branch 'origin/master' into smoke-cli

7645f36

connorjclark requested a review from a team as a code owner January 13, 2021 01:14

vercel bot deployed to Preview January 13, 2021 01:14 View deployment

connorjclark changed the title ~~tests(smokehouse): filter tests~~ tests(smokehouse): filter tests by url Jan 13, 2021

connorjclark changed the title ~~tests(smokehouse): filter tests by url~~ tests(smokehouse): filter tests by url, print rerun command Jan 13, 2021

connorjclark added waiting4reviewer and removed waiting4committer labels Jan 13, 2021

patrickhulce approved these changes Jan 13, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Jan 13, 2021

brendankenny reviewed Jan 13, 2021

View reviewed changes

connorjclark added 2 commits January 13, 2021 13:16

update

f52156a

pr

f471265

vercel bot deployed to Preview January 13, 2021 19:20 View deployment

connorjclark added waiting4reviewer and removed waiting4committer labels Aug 16, 2021

connorjclark mentioned this pull request Aug 17, 2021

tests(smoke): print command to rerun failures #12924

Merged

connorjclark closed this Aug 17, 2021

tests(smokehouse): filter tests by url, print rerun command #9676

tests(smokehouse): filter tests by url, print rerun command #9676

Uh oh!

Conversation

connorjclark commented Sep 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

connorjclark commented Sep 16, 2019

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

connorjclark commented Sep 16, 2019

Uh oh!

brendankenny commented Sep 16, 2019

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish commented Aug 5, 2020

Uh oh!

connorjclark commented Jan 13, 2021

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

connorjclark commented Aug 16, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

connorjclark commented Sep 14, 2019 •

edited

Loading