Revamp of the offline.js gatherer #578

jeffposnick · 2016-08-10T18:08:26Z

This updates the logic in the offline.js gatherer. Instead of the XHR-based approach, which is prone to false-negatives for more complicated service workers, it goes through the steps needed to perform a full navigation to the target URL while the network is disconnected, and confirms that any successful responses were actually due to the service worker.

I've got a couple of inline comments that it would be great to get some feedback on, mainly due to my ignorance about Lighthouse's overall setup.

Closes #425

jeffposnick · 2016-08-10T18:14:19Z

lighthouse-core/gather/gatherers/offline.js

-            offlineResponseCode: -1
-          };
-        });
+    return driver.gotoURL(options.url, {waitForLoad: true})


So I think that this initial navigation to options.url is wasteful, since the config.json for this pass specifies "loadPage": true. That being said, when I used node lighthouse-cli <url> --audit-whitelist=works-offline to just run this one specific audit, I did end up needing to explicitly add in the initial navigation.

It may be because this is currently in a beforePass function. I don't think I could move this code to afterPass, since by the time that's called, tracingData is already set. Is there some other lifecycle function that I could use instead that would allow me to take advantage of "loadPage": true?

jeffposnick · 2016-08-10T18:23:57Z

(I'm looking into the Travis CI build failure.)

jeffposnick · 2016-08-11T11:27:25Z

Travis CI should be happy, and I tried to follow the model of using pre-recorded tracing data to drive the modified test cases. Happy to adjust the tests further based on any feedback.

brendankenny · 2016-08-12T00:39:06Z

Looking at the network record is definitely a much better way of doing this, but in general we really want to avoid doing navigation inside gatherers. Ideally this gather can just be beforePass() { Offline.goOffline() }, afterPass() { Offline.goOnline(); /* process network records... */ } with the navigation part in between correctly handled by the driver. Hopefully we can instead land any changes in lighthouse that are needed so this test can just use the usual navigation.

I see two issues with the current master:

when whitelisting an audit, only the required gathers are run. Before getting to the offline gatherer itself, the offline test requires a full load of the page while online to get the SW registered, assets loaded, etc, and we have no way of expressing this with our current audit/gatherer dependency resolution.

There are a few possible solutions here, but ultimately we want to make it clear that the offline test can't be in the first pass, the page has to have loaded before it runs, etc
networkRecords are overwritten for every pass that records them. Not a problem with the default config because only one pass records them in it, but as soon as something turns on network in another pass (like this PR does), each set of records will overwrite the last. networkRecords need to be updated to be stored per pass, like traces were in Trace buckets #531.

The second one will definitely have to be fixed before this will work, as it will break anything depending on network records from the first pass. Not sure what to do about the first one at this time, but I'd personally rather we (temporarily) break audit whitelisting for some gathers than start navigating in gatherers.

brendankenny · 2016-08-12T00:43:30Z

lighthouse-core/test/gather/gatherers/offline.js

-      assert.deepEqual(offlineGather.artifact, {offlineResponseCode: 200});
-    });
+  it('returns an artifact set to -1 when offline loading fails', () => {
+    offlineGather.afterPass({url: 'https://do-not-match.com'}, tracingData);


We should avoid testing with real network in the unit tests. The fixed smoke/end-to-end test in #518 should cover these two test cases. As far as unit testing goes, I'd say the only real thing that needs to be checked is any corner case(s) of url matching (trailing slash and...anything else?)

actually, if the gatherer doesn't do any navigation these would be good tests, just processing static network records :)

paulirish · 2016-08-12T02:20:38Z

networkRecords are overwritten for every pass that records them. Not a problem with the default config because only one pass records them in it, but as soon as something turns on network in another pass (like this PR does), each set of records will overwrite the last. networkRecords need to be updated to be stored per pass, like traces were....

Instead of flipping the network back on, I'd be in favor of leaving network listening after the first pass, and then in here, we'd work from the end of the network records to find a matching item.

paulirish · 2016-08-24T01:41:15Z

Brendan is landing some items that will enable us to take this PR in easier. Thanks for the patience!

brendankenny · 2016-08-29T22:05:34Z

@jeffposnick PTAL

Updated PR to take advantage of #610. Simpler and seems to support more sites than master (for instance, fixes #611).

There are some failure cases, like airhorner now reports that it doesn't work offline. I believe this is due to a flaw in gather-runner ordering. I'll make a separate PR for that.

brendankenny · 2016-08-29T22:06:59Z

also breaks under the smoketest. I'll take a look at that too :) May be the same issue as airhorner

brendankenny · 2016-08-30T01:31:58Z

updated, should be good to go.

paulirish · 2016-08-30T16:34:46Z

lighthouse-core/gather/gatherers/offline.js

  }

  static goOnline(driver) {
    return driver.sendCommand('Network.emulateNetworkConditions', Offline.config({offline: false}));


lets move these to the driver

jeffposnick · 2016-08-30T17:10:40Z

Your version of the code seems to give the correct results, and thanks for rewriting it using a cleaner approach!

I can still reliably trigger TypeError: this.networkManager._dispatcher.resourceChangedPriority is not a function when I run the CLI against my test site (https://ifixit-pwa.appspot.com), which I tried to address in one of my previous commits.

But given that I can reproduce the same error against the current master branch, I'll file an issue for it and it can be dealt with independent of this code change.

jeffposnick · 2016-08-30T17:18:09Z

(I filed #621 to track the priority-related issues I'm seeing.)

brendankenny · 2016-08-30T17:31:38Z

I can still reliably trigger TypeError: this.networkManager._dispatcher.resourceChangedPriority is not a function when I run the CLI against my test site (https://ifixit-pwa.appspot.com), which I tried to address in one of my previous commits.

really weird. I was hoping that doing it this way would avoid any WebInspector initialization timing issues. I'm not seeing the error at all, unfortunately, but maybe I should do a fresh npm install and see if I'm lagging in deps or something

jeffposnick · 2016-08-30T17:42:48Z

After investigating more, @brendankenny, the errors I'm seeing don't appear to be related to the code in this PR. So they can be investigated independently.

brendankenny · 2016-08-31T01:37:01Z

lets move these to the driver

done, though it's a little ugly with the options object. Happy to listen to other ideas

paulirish · 2016-08-31T05:09:38Z

lighthouse-core/gather/gatherers/offline.js

+  afterPass(options, tracingData) {
+    const navigationRecord = tracingData.networkRecords.filter(record => {
+      // If options.url is just an origin without a path, Chrome will implicitly
+      // add in a path of '/'.


I actually took care of this part a bit ago: https://github.com/GoogleChrome/lighthouse/blob/ee92529ba/lighthouse-core/runner.js#L50-L51

can be simplified to just ...

return record._url === options.url && record._fetchedViaServiceWorker;

good point. also updated to use the non-underscored getter versions of these properties

paulirish · 2016-08-31T05:11:55Z

works for me. lgtm

brendankenny · 2016-08-31T23:32:22Z

Thanks @jeffposnick! Sorry this took so long.

jeffposnick · 2016-09-01T00:50:55Z

Thanks for taking it over, @brendankenny!

jeffposnick reviewed Aug 10, 2016
View reviewed changes

paulirish mentioned this pull request Aug 11, 2016

Keep track of the document URL post-redirects #582

Merged

brendankenny reviewed Aug 12, 2016
View reviewed changes

brendankenny mentioned this pull request Aug 24, 2016

support collecting network records per pass #610

Merged

brendankenny mentioned this pull request Aug 24, 2016

Works offline audit failure case #611

Closed

brendankenny force-pushed the works-offline branch from 2138876 to bde0b39 Compare August 29, 2016 21:02

brendankenny mentioned this pull request Aug 29, 2016

about:blank navigation moved to before gatherer.beforeClass() #619

Merged

jeffposnick added 2 commits August 29, 2016 18:14

Revamp of the offline.js gatherer

2b39d27

Attempt to fix the tests

34509b1

brendankenny force-pushed the works-offline branch from 2e209ea to fa6e524 Compare August 30, 2016 01:27

paulirish reviewed Aug 30, 2016
View reviewed changes

paulirish reviewed Aug 31, 2016
View reviewed changes

paulirish added the lgtm label Aug 31, 2016

brendankenny force-pushed the works-offline branch from 459dae7 to c06b2f3 Compare August 31, 2016 22:33

update offline gatherer to use network recording changes

db69edd

brendankenny force-pushed the works-offline branch from c06b2f3 to ceb5f36 Compare August 31, 2016 23:24

add goOffline/goOnline methods to driver

ceb5f36

brendankenny merged commit 50890ce into master Aug 31, 2016

brendankenny deleted the works-offline branch August 31, 2016 23:30

jeffposnick mentioned this pull request Feb 6, 2017

Audit: does SW use offline-first caching? (or use s-w-r?) #1646

Open

jeffposnick mentioned this pull request Jul 14, 2017

Audit claims start_url of /?utm_ is not cached when navigateFallback exists #2688

Closed

brendankenny mentioned this pull request Feb 24, 2021

core(driver): remove unused goOffline/goOnline methods #12135

Merged

Revamp of the offline.js gatherer #578

Revamp of the offline.js gatherer #578

Uh oh!

Conversation

jeffposnick commented Aug 10, 2016

Uh oh!

jeffposnick Aug 10, 2016

Choose a reason for hiding this comment

Uh oh!

jeffposnick commented Aug 10, 2016

Uh oh!

jeffposnick commented Aug 11, 2016

Uh oh!

brendankenny commented Aug 12, 2016

Uh oh!

brendankenny Aug 12, 2016

Choose a reason for hiding this comment

Uh oh!

brendankenny Aug 12, 2016

Choose a reason for hiding this comment

Uh oh!

paulirish commented Aug 12, 2016

Uh oh!

paulirish commented Aug 24, 2016

Uh oh!

brendankenny commented Aug 29, 2016

Uh oh!

brendankenny commented Aug 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brendankenny commented Aug 30, 2016

Uh oh!

paulirish Aug 30, 2016

Choose a reason for hiding this comment

Uh oh!

jeffposnick commented Aug 30, 2016

Uh oh!

jeffposnick commented Aug 30, 2016

Uh oh!

brendankenny commented Aug 30, 2016

Uh oh!

jeffposnick commented Aug 30, 2016

Uh oh!

brendankenny commented Aug 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulirish Aug 31, 2016

Choose a reason for hiding this comment

Uh oh!

brendankenny Aug 31, 2016

Choose a reason for hiding this comment

Uh oh!

paulirish commented Aug 31, 2016

Uh oh!

brendankenny commented Aug 31, 2016

Uh oh!

jeffposnick commented Sep 1, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brendankenny commented Aug 29, 2016 •

edited

Loading

brendankenny commented Aug 31, 2016 •

edited

Loading