core(smoke): support a per-runner test exclusion list #14316

alexnj · 2022-08-24T20:31:04Z

Fixes #14127.

cli/test/smokehouse/config/config.js

connorjclark

This will be very useful!

cli/test/smokehouse/config/config.js

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

brendankenny · 2022-08-26T16:33:20Z

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

@@ -193,8 +194,12 @@ async function begin() {
  const requestedTestIds = argv._;
  const {default: rawTestDefns} = await import(url.pathToFileURL(testDefnPath).href);
  const allTestDefns = updateTestDefnFormat(rawTestDefns);
+  const excludedTests = new Set(exclusions[argv.runner] || []);
+  const filteredTestDefns = !excludedTests.size ? allTestDefns :
+    allTestDefns.filter(test => !excludedTests.has(test.id));


I think you're actually going to have to pass this into getDefinitionsToRun since if invertMatch is true this is actually going to add the excluded tests to the list of tests to run

In #14127 (comment) I suggested adding it to just this branch in getDefinitionsToRun

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

Lines 52 to 55 in ec4b36b

if (requestedIds.length === 0 && !invertMatch) {

smokes = [...allTestDefns];

console.log('Running ALL smoketests. Equivalent to:');

console.log(usage);

still seems reasonable so that you could still run yarn smoke --runner devtools redirects-scripts manually if the need ever arose.

If we wanted to support exclusions with invertMatch and the explicit case that gets a lot tricker, so maybe just punt on exclusions+invertMatch for when/if we actually need that?

What would be a good use case for invertMatch now? Previously it was being used to exclude tests.

@brendankenny getDefinitionsToRun runs off the supplied list, which is already filtered. So the scenario of inversion adding excluded list wouldn't happen. The other case though, which is wanting to run an excluded test manually wouldn't work, and will require removal of exclusion from config.

getDefinitionsToRun runs off the supplied list, which is already filtered

whoops, yes, you're totally right

The other case though, which is wanting to run an excluded test manually wouldn't work, and will require removal of exclusion from config.

I guess I thought we were doing this from #14127 (comment), instead of having an explicit --ignore-exclusions flag. If someone is e.g. trying to get byte-efficiency working in devtools, running yarn smoke --runner devtools byte-efficiency simply won't run anything. It's not the end of the world to remember to go remove it from the exclusions list at that point, and I guess we can also just see how it goes in practice, but it also seems like it wouldn't be too hard to support?

What would be a good use case for invertMatch now? Previously it was being used to exclude tests.

Probably not much. It was originally added for a weaker and much more awkward version of what sharding does now. We can survey if anyone else wants it (and double check if LR is using it/needs it for now), but otherwise removing (in some future PR) SG

brendankenny · 2022-08-26T16:34:34Z

cli/test/smokehouse/config/config.js

+    // Disabled because Chrome will follow the redirect first, and Lighthouse will
+    // only ever see/run the final URL.


ah, this is so much better now having these comments right by each group of tests being excluded

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

brendankenny

two last comments but your call on both.

LGTM! Really happy to have all these in one accessible place

brendankenny · 2022-08-29T16:10:38Z

cli/test/smokehouse/config/exclusions.js

+    // Disabled because these tests use settings that cannot be fully configured in
+    // DevTools (e.g. throttling method "provided").
+    'metrics-tricky-tti', 'metrics-tricky-tti-late-fcp', 'screenshot',
+    // Disabled because of differences that need further investigation
+    'byte-efficiency', 'byte-gzip', 'perf-preload',


@connorjclark should we open smoke backlog bugs for these? Both the explicitly needs investigation and if we can switch the metrics off of provided

brendankenny · 2022-08-29T16:11:39Z

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

@@ -193,8 +194,12 @@ async function begin() {
  const requestedTestIds = argv._;
  const {default: rawTestDefns} = await import(url.pathToFileURL(testDefnPath).href);
  const allTestDefns = updateTestDefnFormat(rawTestDefns);
+  const excludedTests = new Set(exclusions[argv.runner] || []);
+  const filteredTestDefns = !excludedTests.size ? allTestDefns :
+    allTestDefns.filter(test => !excludedTests.has(test.id));


getDefinitionsToRun runs off the supplied list, which is already filtered

whoops, yes, you're totally right

The other case though, which is wanting to run an excluded test manually wouldn't work, and will require removal of exclusion from config.

I guess I thought we were doing this from #14127 (comment), instead of having an explicit --ignore-exclusions flag. If someone is e.g. trying to get byte-efficiency working in devtools, running yarn smoke --runner devtools byte-efficiency simply won't run anything. It's not the end of the world to remember to go remove it from the exclusions list at that point, and I guess we can also just see how it goes in practice, but it also seems like it wouldn't be too hard to support?

brendankenny · 2022-08-29T16:18:45Z

cli/test/smokehouse/config/exclusions.js

@@ -0,0 +1,31 @@
+/**


nit: maybe rename the directory too to not overload config?

It's not the end of the world to remember to go remove it from the exclusions list at that point

I added a console line that lists excluded tests when a requested test is mismatched. So in this scenario, it would print:

Smoketests not found for: byte-efficiency Check test exclusions (byte-efficiency)

This should help reminding the dev running smoke why a test can't be found. If this and the config edit in practice turns out inconvenient, we can add the --ignore-exclusions that @connorjclark initially suggested.

brendankenny · 2022-08-29T16:20:34Z

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

@@ -193,8 +194,12 @@ async function begin() {
  const requestedTestIds = argv._;
  const {default: rawTestDefns} = await import(url.pathToFileURL(testDefnPath).href);
  const allTestDefns = updateTestDefnFormat(rawTestDefns);
+  const excludedTests = new Set(exclusions[argv.runner] || []);
+  const filteredTestDefns = !excludedTests.size ? allTestDefns :
+    allTestDefns.filter(test => !excludedTests.has(test.id));


What would be a good use case for invertMatch now? Previously it was being used to exclude tests.

Probably not much. It was originally added for a weaker and much more awkward version of what sharding does now. We can survey if anyone else wants it (and double check if LR is using it/needs it for now), but otherwise removing (in some future PR) SG

core(smoke): support a per-runner test exclusion list

f0a8113

alexnj requested review from brendankenny and connorjclark August 24, 2022 20:31

alexnj requested a review from a team as a code owner August 24, 2022 20:31

vercel bot deployed to Preview August 24, 2022 20:32 View deployment

connorjclark reviewed Aug 24, 2022

View reviewed changes

cli/test/smokehouse/config/config.js Outdated Show resolved Hide resolved

connorjclark reviewed Aug 24, 2022

View reviewed changes

cli/test/smokehouse/config/config.js Outdated Show resolved Hide resolved

devtools-bot assigned brendankenny Aug 24, 2022

devtools-bot added the waiting4reviewer label Aug 24, 2022

license and exclusions from devtools.yaml

df3963c

vercel bot deployed to Preview August 24, 2022 22:11 View deployment

ocd trigger

ce8b60f

vercel bot deployed to Preview August 24, 2022 22:20 View deployment

Merge branch 'master' into smoke-exclusion

7a6447c

vercel bot deployed to Preview August 24, 2022 22:22 View deployment

Move redirects together

643aa2a

vercel bot deployed to Preview August 24, 2022 22:26 View deployment

Merge remote-tracking branch 'origin' into smoke-exclusion

02b1950

vercel bot deployed to Preview August 24, 2022 22:28 View deployment

Merge branch 'master' into smoke-exclusion

6eea127

vercel bot deployed to Preview August 24, 2022 22:49 View deployment

connorjclark reviewed Aug 25, 2022

View reviewed changes

cli/test/smokehouse/config/config.js Outdated Show resolved Hide resolved

connorjclark approved these changes Aug 25, 2022

View reviewed changes

Moved the comment to jsDoc.

73b3387

vercel bot deployed to Preview August 25, 2022 22:42 View deployment

Merge branch 'master' into smoke-exclusion

351a5fd

vercel bot deployed to Preview August 25, 2022 22:44 View deployment

alexnj removed the waiting4reviewer label Aug 25, 2022

brendankenny reviewed Aug 26, 2022

View reviewed changes

Update cli/test/smokehouse/config/config.js

645b7b9

Co-authored-by: Brendan Kenny <bckenny@gmail.com>

vercel bot deployed to Preview August 26, 2022 17:36 View deployment

Review comments.

aeeff90

vercel bot deployed to Preview August 26, 2022 22:09 View deployment

Removed the accidental inclusion of a11y

dfe873e

vercel bot deployed to Preview August 26, 2022 22:13 View deployment

Merge branch 'master' into smoke-exclusion

6f988ba

vercel bot deployed to Preview August 26, 2022 22:16 View deployment

brendankenny approved these changes Aug 29, 2022

View reviewed changes

alexnj merged commit 650d4ff into GoogleChrome:master Aug 29, 2022

alexnj mentioned this pull request Aug 29, 2022

smoke: retire --invert-match cli option #14328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(smoke): support a per-runner test exclusion list #14316

core(smoke): support a per-runner test exclusion list #14316

alexnj commented Aug 24, 2022

connorjclark left a comment

brendankenny Aug 26, 2022

brendankenny Aug 26, 2022

alexnj Aug 26, 2022

alexnj Aug 26, 2022 •

edited

Loading

brendankenny Aug 29, 2022

brendankenny Aug 29, 2022 •

edited

Loading

brendankenny Aug 26, 2022

brendankenny left a comment

brendankenny Aug 29, 2022

brendankenny Aug 29, 2022

brendankenny Aug 29, 2022

alexnj Aug 29, 2022

brendankenny Aug 29, 2022 •

edited

Loading

	if (requestedIds.length === 0 && !invertMatch) {
	smokes = [...allTestDefns];
	console.log('Running ALL smoketests. Equivalent to:');
	console.log(usage);

		// Disabled because Chrome will follow the redirect first, and Lighthouse will
		// only ever see/run the final URL.

core(smoke): support a per-runner test exclusion list #14316

core(smoke): support a per-runner test exclusion list #14316

Conversation

alexnj commented Aug 24, 2022

connorjclark 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

alexnj Aug 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny Aug 29, 2022 • edited Loading

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 Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

alexnj Aug 26, 2022 •

edited

Loading

brendankenny Aug 29, 2022 •

edited

Loading

brendankenny Aug 29, 2022 •

edited

Loading