tests(tags-blocking-first-paint): alternate stylesheets are not blocking #9248

connorjclark · 2019-06-21T03:46:08Z

While investigating #2065, I learned alternate stylesheets are never considered blocking. We do handle these correctly, but we had no test cases, so I added one.

Must be in the smoke test, as the unit test completely mocks out the code under test (it's mostly in a page function).

fyi: apparently, you must install an extension in Chrome to use alternate stylesheets. Firefox has it built in.

…t considered blocking

connorjclark · 2019-06-21T03:48:08Z

lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js

@@ -16,6 +16,9 @@ module.exports = [
      }, {
        id: 'wordpress',
      }],
+      TagsBlockingFirstPaint: {
+        length: 7,


should i be more explicit here?

[ { "tag": { "tagName": "LINK", "url": "http://localhost:10200/dobetterweb/dobetterweb/dbw_tester.css?delay=100", "href": "http://localhost:10200/dobetterweb/dobetterweb/dbw_tester.css?delay=100", "rel": "stylesheet", "media": "", "disabled": false, "mediaChanges": [] }, }, { "tag": { "tagName": "LINK", "url": "http://localhost:10200/dobetterweb/unknown404.css?delay=200", "href": "http://localhost:10200/dobetterweb/unknown404.css?delay=200", "rel": "stylesheet", "media": "", "disabled": false, "mediaChanges": [] }, }, { "tag": { "tagName": "LINK", "url": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200", "href": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200", "rel": "stylesheet", "media": "", "disabled": false, "mediaChanges": [] }, }, { "tag": { "tagName": "LINK", "url": "http://localhost:10200/dobetterweb/dbw_partial_a.html?delay=200", "href": "http://localhost:10200/dobetterweb/dbw_partial_a.html?delay=200", "rel": "import", "media": "", "disabled": false, "mediaChanges": [] }, }, { "tag": { "tagName": "LINK", "url": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped", "href": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped", "rel": "stylesheet", "media": "screen", "disabled": false, "mediaChanges": [ { "href": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped", "media": "not-matching", "matches": false }, { "href": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped", "media": "screen", "matches": true } ] }, }, { "tag": { "tagName": "SCRIPT", "url": "http://localhost:10200/dobetterweb/dbw_tester.js", "src": "http://localhost:10200/dobetterweb/dbw_tester.js", "mediaChanges": [] }, }, { "tag": { "tagName": "SCRIPT", "url": "http://localhost:10200/dobetterweb/fcp-delayer.js?delay=5000", "src": "http://localhost:10200/dobetterweb/fcp-delayer.js?delay=5000", "mediaChanges": [] }, } ]

I think we should include the URL at least otherwise debugging this is going to be a nightmare, are they in a predictable order? should we sort by URL or something?

we do have an existing assertion for the tags blocking first paint, in the render-blocking-resources audit, and that's probably a better one to sort (I'm kind of surprised it's not sorted by wastedMs already)

using that audit for the assertion is problematic b/c it also ignores resources that don't meet a minimum threshold.

yeah I agree with @connorjclark it's worth asserting these artifacts directly, and if we're asserting the length we should make it debuggable by asserting the URLs

Fair enough! I guess should say the audit(s) should also be updated, then, so we aren't neglecting the "to end" part of end-to-end testing :)

patrickhulce

always love more tests! ❤️

patrickhulce · 2019-06-21T13:38:19Z

lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js

@@ -16,6 +16,9 @@ module.exports = [
      }, {
        id: 'wordpress',
      }],
+      TagsBlockingFirstPaint: {
+        length: 7,


I think we should include the URL at least otherwise debugging this is going to be a nightmare, are they in a predictable order? should we sort by URL or something?

brendankenny

you must install an extension in Chrome to use alternate stylesheets. Firefox has it built in.

like, it won't load at all? Or there's no UI support? Or...?

brendankenny · 2019-06-21T23:00:23Z

lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js

@@ -16,6 +16,9 @@ module.exports = [
      }, {
        id: 'wordpress',
      }],
+      TagsBlockingFirstPaint: {
+        length: 7,


we do have an existing assertion for the tags blocking first paint, in the render-blocking-resources audit, and that's probably a better one to sort (I'm kind of surprised it's not sorted by wastedMs already)

connorjclark · 2019-06-23T21:53:50Z

like, it won't load at all? Or there's no UI support? Or...?

Oops, I forgot to share the surge I made: https://lh-alt-styles.surge.sh/

Chrome offers no UI. it still downloads the stylesheet. Firefox allows you to select which alternate to use (View -> Page Style).

connorjclark · 2019-06-24T17:01:24Z

lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js

+        },
+        undefined, // marks end of array
+      ],
+      // TagsBlockingFirstPaint: {


which one?

I very much dislike both, but I dislike the first one less, even though it would technically pass something like [Object,Object,Object,undefined,Object,Object] ...

Was considering doing something like [..., Symbol('end')] instead of [..., undefined] and adding a customer checker in the assertions, but that would be too far for just this one thing.

option c: we could start asserting the array length in our smokes :)

I'm fine with the first one though, definitely not a huge fan of option 2

i like c. I can follow up after this PR.

cool beans want to remove the dead weight ones for now then? :)

lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js

patrickhulce

LGTM

brendankenny · 2019-06-27T20:41:06Z

lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js

@@ -16,6 +16,64 @@ module.exports = [
      }, {
        id: 'wordpress',
      }],
+      TagsBlockingFirstPaint: [


the user-observable behavior here is in the render-blocking-resources audit (though TagsBlockingFirstPaint will enter our PublicGathererArtifacts someday).

There is a balance to strike when testing internal state when it's difficult to test otherwise, so I understand the argument for capturing it here, but if we accept that and that this data is important to assert, then we should assert it in the audit as well.

would you prefer I change the alternate request to a large stylesheet so we can assert in the audit too?

would you prefer I change the alternate request to a large stylesheet so we can assert in the audit too?

that might make the alternate stylesheet test more realistic, but not being included there is also a signal that the audit is working as expected

sorry, but what's the requested change here?

sorry, but what's the requested change here?

if whatever items you don't want to see regress (e.g. the urls, etc) are important enough to add to a smoke test, they should be asserted where they are actually observable by a user? The artifact can be asserted all day but that doesn't matter if the audit is mismatched to it.

talked offline - didn't realize his suggestions are independent of the specific alternate-stylesheet I added. should expand the render-blocking-resources audit expectation to be more than "detail.items is array of 7 items".

yeah, sorry, we were having two different conversations :)

connorjclark · 2019-07-10T22:14:54Z

FYI the reason I want this test is to avoid regressions from any action taken on #2065

brendankenny

LGTM

…nate-testcase

tests(tags-blocking-first-paint): ensure alternate stylesheets are no…

65014f3

…t considered blocking

connorjclark requested review from brendankenny and patrickhulce as code owners June 21, 2019 03:46

googlebot added the cla: yes label Jun 21, 2019

connorjclark commented Jun 21, 2019

View reviewed changes

patrickhulce reviewed Jun 21, 2019

View reviewed changes

brendankenny reviewed Jun 21, 2019

View reviewed changes

expand artifact expectation

332251e

vercel bot deployed to staging June 24, 2019 16:59 View deployment

connorjclark commented Jun 24, 2019

View reviewed changes

lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js Show resolved Hide resolved

connorjclark changed the title ~~tests(tags-blocking-first-paint): ensure alternate stylesheets are not considered blocking~~ tests(tags-blocking-first-paint): alternate stylesheets are not blocking Jun 24, 2019

remove alt expectation

338f238

vercel bot deployed to staging June 27, 2019 03:25 View deployment

connorjclark mentioned this pull request Jun 27, 2019

tests(smokehouse): assert on expected array length #9292

Merged

patrickhulce approved these changes Jun 27, 2019

View reviewed changes

brendankenny reviewed Jun 27, 2019

View reviewed changes

more deets

6a66343

vercel bot deployed to staging July 10, 2019 22:36 View deployment

brendankenny approved these changes Jul 10, 2019

View reviewed changes

lint

66a48ee

vercel bot deployed to staging July 11, 2019 01:53 View deployment

remove end mark

bce8593

vercel bot temporarily deployed to staging July 11, 2019 02:05 Inactive

Merge remote-tracking branch 'origin/master' into tags-blocking-alter…

00f60f8

…nate-testcase

vercel bot deployed to staging July 11, 2019 02:06 View deployment

connorjclark merged commit a36d263 into master Jul 11, 2019

paulirish deleted the tags-blocking-alternate-testcase branch July 11, 2019 23:07

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

tests(tags-blocking-first-paint): alternate stylesheets are not blocking #9248

tests(tags-blocking-first-paint): alternate stylesheets are not blocking #9248

tests(tags-blocking-first-paint): alternate stylesheets are not blocking #9248

tests(tags-blocking-first-paint): alternate stylesheets are not blocking #9248

Conversation

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment