tests(smoke): convert core tests to single-expectations format #12819

brendankenny · 2021-07-21T23:43:25Z

part 2/2 of #11950. PR currently based on top of #12818

This splits up our core tests so there's a separate testDefn and ID for every smoke expectation. The splitting up is done here and tentative IDs are chosen, but I wanted some bike shedding input before I tried to put everything in the same format.

For reviewers:

this is much, much easier to review with ?w=1. Lots of indentation changes.
Most important file is core-tests.js, as that's where the test IDs are set.
(I'll be honest: there are a lot of these, and about halfway through this I stopped trying very hard with the names if the tests weren't super obvious how they were different from each other. I'm looking at you redirects and csp :P Please suggest better IDs than I have here)
expectations are functionally all the same, but feel free to pick a smoke file you care about and double check it's still good and offer opinions on the changes in them.

For bikeshedders:

IDs!
smoketests proposal: one test file per url #11950 suggested a file per expectation, but in some cases there are shared properties between expectations, and multiple named exports could maybe work ok. Do we want to go all the way and still do a file per expectation (and import shared stuff), or have a mixture of single and multiples, or...? Splitting into different files should be easy now, so I'm happy to go either way.
We have grouped IDs where e.g. the old errors becomes errors-infinite-loop, errors-expired-ssl etc. Handy for running with just yarn smoke errors, but:
- do we want to formalize a category-specific naming format?
- are one-off tests ok? e.g. there's only one oopif tests, so it's still just called oopif. Is that ok or should we preemptively call it oopif-requests just in case one day we have oopif-some-other-thing?

brendankenny · 2021-07-21T23:53:00Z

lighthouse-cli/test/smokehouse/test-definitions/issues/mixed-content.js

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


moved from dbw-expectations.js as dbw is kind of its own thing and issues are worthy of their own smoke tests

patrickhulce

names mostly seem fine to me! 🎉

thanks for trudging through this super mechanical drudgery :D

patrickhulce · 2021-07-22T14:17:18Z

lighthouse-cli/test/smokehouse/test-definitions/lantern/lantern-expectations.js

+/**
+ * @type {Smokehouse.ExpectedRunnerResult}
+ */
+const ricShort = {


idleCallbackShort?

ric lowercase is harder to remember what it is than rIC, probably should've done that with the ric-shim filename originally 😊

patrickhulce · 2021-07-22T14:17:28Z

lighthouse-cli/test/smokehouse/test-definitions/lantern/lantern-expectations.js

+/**
+ * @type {Smokehouse.ExpectedRunnerResult}
+ */
+const ricLong = {


patrickhulce · 2021-07-22T14:21:09Z

lighthouse-cli/test/smokehouse/test-definitions/redirects/expectations.js

+// TODO: what's the describable difference between this and the above? Also, better names.
+/**
+ * @type {Smokehouse.ExpectedRunnerResult}
+ * Expected Lighthouse audit values for a site with a client-side redirect (2s + 5s),
+ * paints at 2s, then a server-side redirect (1s).
+ */
+const clientPaintServer2 = {


Suggested change

// TODO: what's the describable difference between this and the above? Also, better names.

/**

* @type {Smokehouse.ExpectedRunnerResult}

* Expected Lighthouse audit values for a site with a client-side redirect (2s + 5s),

* paints at 2s, then a server-side redirect (1s).

*/

const clientPaintServer2 = {

/**

* @type {Smokehouse.ExpectedRunnerResult}

* Expected Lighthouse audit values for a site with a client-side redirect (2s + 5s),

* paints at 2s, then a server-side redirect (1s), followed by a history.pushState to an unrelated URL.

*/

const historyPushState = {

NOTE: will require a few other historyPushState renames as well

patrickhulce · 2021-07-22T14:22:40Z

lighthouse-cli/test/smokehouse/test-definitions/core-tests.js

  config: require('./seo/seo-config.js'),
 }, {
-  id: 'offline',
-  expectations: require('./offline-local/offline-expectations.js'),
+  id: 'seo-tap-target',


Suggested change

id: 'seo-tap-target',

id: 'seo-tap-targets',

patrickhulce · 2021-07-22T14:22:54Z

lighthouse-cli/test/smokehouse/test-definitions/core-tests.js

+  expectations: redirects.singleClient,
+  config: require('./redirects/redirects-config.js'),
+}, {
+  id: 'redirects-client-paint-server-2',


Suggested change

id: 'redirects-client-paint-server-2',

id: 'redirects-history-pushstate',

paulirish · 2021-07-22T21:47:15Z

yay death to pwa2 and pwa3! :)

Do we want to go all the way and still do a file per expectation (and import shared stuff)

yeah. i think doing that allows us to fix the file/folder structure of test-definitions which i'm really excited about. i think it makes sense as a follow up PR; the scope of this one is great.

do we want to formalize a category-specific naming format?

eh, nah.

are one-off tests ok? e.g. there's only one oopif tests, so it's still just called oopif. Is that ok or should we preemptively call it oopif-requests just in case one day we have oopif-some-other-thing?

yeah lets leave it alone. shorter is better, so lets only elongate when we have to.

i'm happy with these slugs (except for csp-base which should be csp-basics or something)

woo!

brendankenny · 2021-07-22T22:00:12Z

SG. We'll go with these (except for the CSP ones, @adamraine is about to take a look) but we have a second chance to discuss them in the next PR.

adamraine · 2021-07-22T21:58:32Z

lighthouse-cli/test/smokehouse/test-definitions/csp/csp-expectations.js

-      requestedUrl: 'http://localhost:10200/csp.html',
-      finalUrl: 'http://localhost:10200/csp.html',
-      audits: {},
+const base = {


Suggested change

const base = {

const noCsp = {

Alternative: allowAll

adamraine · 2021-07-22T21:59:16Z

lighthouse-cli/test/smokehouse/test-definitions/csp/csp-expectations.js

+/**
+ * @type {Smokehouse.ExpectedRunnerResult}
+ */
+const exceptInline = {


Suggested change

const exceptInline = {

const blockAllM91 = {

This CSP blocks everything, with a special exception for one inline script that has a source map.

We will probably remove this once M91 hits stable, so I would add the version suffix to this one.

adamraine · 2021-07-22T22:00:45Z

lighthouse-cli/test/smokehouse/test-definitions/csp/csp-expectations.js

+/**
+ * @type {Smokehouse.ExpectedRunnerResult}
+ */
+const exceptInlineM92 = {


Suggested change

const exceptInlineM92 = {

const blockAll = {

brendankenny · 2021-07-22T22:06:34Z

looks great. Thanks!

connorjclark · 2021-07-22T23:30:58Z

lighthouse-cli/test/smokehouse/test-definitions/core-tests.js

+  id: 'issues-mixed-content',
+  expectations: require('./issues/mixed-content.js'),
+}, {
+  id: 'redirects-single-server',


how about going one step further and moving this entire object to what expectations.js exports? there is overhead in separating "this is what the test does" and "this is the id we give it and the config it uses"

then core-tests.js is just bringing it all together as an array.

(separate PR would be best)

Yeah, it's a little silly to have filenames for separate files and then IDs for them in a different file. That'll be in the above-mentioned (but undescribed, sorry) next PR.

brendankenny requested a review from a team as a code owner July 21, 2021 23:43

brendankenny requested review from patrickhulce and removed request for a team July 21, 2021 23:43

google-cla bot added the cla: yes label Jul 21, 2021

devtools-bot assigned patrickhulce Jul 21, 2021

devtools-bot added the waiting4reviewer label Jul 21, 2021

brendankenny commented Jul 21, 2021

View reviewed changes

patrickhulce approved these changes Jul 22, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Jul 22, 2021

Base automatically changed from single-smoke to master July 22, 2021 21:45

tests(smoke): convert core tests to single-expectations format

4de7358

brendankenny force-pushed the smoke-core-flattening branch from 7cad115 to 4de7358 Compare July 22, 2021 21:50

vercel bot deployed to Preview July 22, 2021 21:50 View deployment

feedback

1cf9d39

vercel bot deployed to Preview July 22, 2021 21:59 View deployment

adamraine reviewed Jul 22, 2021

View reviewed changes

csp IDs

53df9a1

vercel bot deployed to Preview July 22, 2021 22:08 View deployment

stdout/stderr ordering

9c3886b

vercel bot deployed to Preview July 22, 2021 22:31 View deployment

connorjclark reviewed Jul 22, 2021

View reviewed changes

brendankenny merged commit 43b11e0 into master Jul 23, 2021

brendankenny deleted the smoke-core-flattening branch July 23, 2021 00:43

brendankenny mentioned this pull request Sep 13, 2021

tests(smokehouse): convert to ES modules #13046

Merged

brendankenny mentioned this pull request Dec 3, 2021

tests: split smoke tests into one test per file #13461

Merged

brendankenny mentioned this pull request Aug 23, 2022

smoketests proposal: one test file per url #11950

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests(smoke): convert core tests to single-expectations format #12819

tests(smoke): convert core tests to single-expectations format #12819

brendankenny commented Jul 21, 2021 •

edited

Loading

brendankenny Jul 21, 2021

patrickhulce left a comment

patrickhulce Jul 22, 2021

patrickhulce Jul 22, 2021

patrickhulce Jul 22, 2021

patrickhulce Jul 22, 2021

patrickhulce Jul 22, 2021

patrickhulce Jul 22, 2021

paulirish commented Jul 22, 2021

brendankenny commented Jul 22, 2021

adamraine Jul 22, 2021

adamraine Jul 22, 2021

adamraine Jul 22, 2021

adamraine Jul 22, 2021

brendankenny commented Jul 22, 2021

connorjclark Jul 22, 2021 •

edited

Loading

brendankenny Jul 23, 2021

	id: 'redirects-client-paint-server-2',
	id: 'redirects-history-pushstate',

tests(smoke): convert core tests to single-expectations format #12819

tests(smoke): convert core tests to single-expectations format #12819

Conversation

brendankenny commented Jul 21, 2021 • edited Loading

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

paulirish commented Jul 22, 2021

brendankenny commented Jul 22, 2021

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 commented Jul 22, 2021

connorjclark Jul 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Jul 21, 2021 •

edited

Loading

connorjclark Jul 22, 2021 •

edited

Loading