Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(start-url): stay offline for entirety of offlinePass #9451

Merged
merged 8 commits into from
Aug 15, 2019
Merged

core(start-url): stay offline for entirety of offlinePass #9451

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b706bab
core(start-url): fetch start url while offline
patrickhulce Jul 24, 2019
d96b796
lint
patrickhulce Jul 25, 2019
828e71b
Merge branch 'master' into offline_for_start_url
patrickhulce Jul 25, 2019
c450ce3
add comment
patrickhulce Jul 25, 2019
1d40c11
wording tweaks
patrickhulce Aug 14, 2019
7711ef2
Merge branch 'master' into offline_for_start_url
patrickhulce Aug 14, 2019
6820ccf
feedback
patrickhulce Aug 14, 2019
b3a1825
lint
patrickhulce Aug 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions lighthouse-core/gather/gatherers/offline.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,14 +21,13 @@ class Offline extends Gatherer {
* @param {LH.Gatherer.LoadData} loadData
* @return {Promise<LH.Artifacts['Offline']>}
*/
afterPass(passContext, loadData) {
async afterPass(passContext, loadData) {
const navigationRecord = loadData.networkRecords.filter(record => {
return URL.equalWithExcludedFragments(record.url, passContext.url) &&
record.fetchedViaServiceWorker;
}).pop(); // Take the last record that matches.

return passContext.driver.goOnline(passContext)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment around L16 that the goOnline now happens in start-url which sequentially follows this gatherer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually it seems like the goOffline in L16 isnt needed anymore since starturl does it on its own. right

Copy link
Collaborator Author

@patrickhulce patrickhulce Jul 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a comment around L16 that the goOnline now happens in start-url which sequentially follows this gatherer?

well the goOnline that cleans this up actually happens in gather-runner that was the problem. each afterPass happens while online and each gatherer has to handle its own online/offline cycle. but yes comment is great idea 👍

actually it seems like the goOffline in L16 isnt needed anymore since starturl does it on its own. right

it's still needed to setup the offline for the load of the page, we do the offline check for both the page and the start url separately

.then(_ => navigationRecord ? navigationRecord.statusCode : -1);
return navigationRecord ? navigationRecord.statusCode : -1;
}
}

Expand Down
24 changes: 20 additions & 4 deletions lighthouse-core/gather/gatherers/start-url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,36 @@ const Gatherer = require('./gatherer.js');

class StartUrl extends Gatherer {
/**
* Grab the manifest, extract it's start_url, attempt to `fetch()` it while offline
* Go offline, assess the start url, go back online.
* @param {LH.Gatherer.PassContext} passContext
* @return {Promise<LH.Artifacts['StartUrl']>}
*/
async afterPass(passContext) {
// Offline pass is not actually offline in `afterPass`,
// each gatherer must set and cleanup its own offline state.
await passContext.driver.goOffline();
const result = await this._determineStartUrlAvailability(passContext);
await passContext.driver.goOnline(passContext);

return result;
}
/**
* Grab the manifest, extract it's start_url, attempt to `fetch()` it while offline
* @param {LH.Gatherer.PassContext} passContext
* @return {Promise<LH.Artifacts['StartUrl']>}
*/
async _determineStartUrlAvailability(passContext) {
const manifest = passContext.baseArtifacts.WebAppManifest;
const startUrlInfo = this._readManifestStartUrl(manifest);
if (startUrlInfo.isReadFailure) {
return {statusCode: -1, explanation: startUrlInfo.reason};
}

return this._attemptStartURLFetch(passContext.driver, startUrlInfo.startUrl).catch(() => {
try {
return await this._attemptStartURLFetch(passContext.driver, startUrlInfo.startUrl)
} catch (err) {
return {statusCode: -1, explanation: 'Unable to fetch start URL via service worker.'};
});
}
}

/**
Expand Down Expand Up @@ -77,7 +93,7 @@ class StartUrl extends Gatherer {
if (!response.fromServiceWorker) {
return resolve({
statusCode: -1,
explanation: 'Unable to fetch start URL via service worker.',
explanation: 'Fetched start URL but not via service worker.',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like this message is more confusing than the original (should this audit care about that it's fetchable like that?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now the message is indistinguishable from not being able to fetch it at all which is a problem IMO. The fact that it fetched but was falling back to disk cache or something is worth flagging. Totally up for whatever wording others feel captures this goal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The start URL did respond, but not via a service worker

mostly trying to align with the text of the audit.. (where we dont talk about fetching, etc)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact that it fetched but was falling back to disk cache or something is worth flagging

I don't know if everyone knows the priority order here, though. If I'm looking at the start_url audit LH tells me

The start URL did respond, but not via a service worker

my first question would be "well why didn't LH try fetching via the SW? Do I need to force it somehow?"

It seems like the only important information is that LH tried to fetch it and the SW didn't return it. Just because it was in the disk cache during the LH run doesn't mean it will be for returning users, and isn't that the case we're testing for?

Alternatively, should we just call out the fromDiskCache case with a separate explanation?

});
}
// Successful SW-served fetch of the start_URL
Expand Down
263 changes: 142 additions & 121 deletions lighthouse-core/test/gather/gatherers/start-url-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,168 +7,189 @@

/* eslint-env jest */

jest.useFakeTimers();

const StartUrlGatherer = require('../../../gather/gatherers/start-url.js');
const parseManifest = require('../../../lib/manifest-parser.js');
const assert = require('assert');

const mockDriver = {
goOffline() {
return Promise.resolve();
},
goOnline() {
return Promise.resolve();
},
off() {},
};

const wrapSendCommand = (mockDriver, url, status = 200, fromServiceWorker = false) => {
mockDriver = Object.assign({}, mockDriver);
mockDriver.evaluateAsync = () => Promise.resolve();
mockDriver.on = (name, cb) => {
cb({response: {status, url, fromServiceWorker}});
};

return mockDriver;
};

describe('Start-url gatherer', () => {
let baseArtifacts;

describe('StartUrl Gatherer', () => {
let mockDriver;
let gatherer;

function createArtifactsWithURL(url) {
return {WebAppManifest: {value: {start_url: {value: url}}}};
}

function unimplemented() {
throw new Error('Unimplemented');
}

beforeEach(() => {
jest.useFakeTimers();
baseArtifacts = {WebAppManifest: null};
gatherer = new StartUrlGatherer();
mockDriver = {
goOffline: unimplemented,
goOnline: unimplemented,
evaluateAsync: unimplemented,
on: unimplemented,
off: unimplemented,
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

moving off bespoke mocks is good, but I have to say this is a nice model for this file :)

});

afterEach(() => {
jest.advanceTimersByTime(5000);
});

it('returns an artifact set to -1 when offline loading fails', () => {
const startUrlGatherer = new StartUrlGatherer();
const startUrlGathererWithQueryString = new StartUrlGatherer();
const startUrlGathererWithResponseNotFromSW = new StartUrlGatherer();
const throwOnEvaluate = (mockDriver) => {
mockDriver.on = () => {};
mockDriver.evaluateAsync = () => {
throw new Error({
TypeError: 'Failed to fetch',
__failedInBrowser: true,
name: 'TypeError',
message: 'Failed to fetch',
});
};

return mockDriver;
};
it('returns a explanation when manifest cannot be found', async () => {
mockDriver.goOffline = jest.fn();
mockDriver.goOnline = jest.fn();

const passContext = {
url: 'https://do-not-match.com/',
baseArtifacts,
driver: throwOnEvaluate(wrapSendCommand(mockDriver, 'https://do-not-match.com/', -1)),
};
const passContextWithFragment = {
baseArtifacts,
url: 'https://ifixit-pwa.appspot.com/?history',
driver: throwOnEvaluate(wrapSendCommand(mockDriver, 'https://ifixit-pwa.appspot.com/?history', -1)),
};
const passContextWithResponseNotFromSW = {
url: 'https://do-not-match.com/',
baseArtifacts: createArtifactsWithURL('https://do-not-match.com/'),
driver: wrapSendCommand(mockDriver, 'https://do-not-match.com/', 200),
baseArtifacts: {WebAppManifest: null},
driver: mockDriver,
};

return Promise.all([
startUrlGatherer.afterPass(passContext),
startUrlGathererWithQueryString.afterPass(passContextWithFragment),
startUrlGathererWithResponseNotFromSW.afterPass(passContextWithResponseNotFromSW),
]).then(([artifact, artifactWithQueryString, artifactWithResponseNotFromSW]) => {
assert.equal(artifact.statusCode, -1);
assert.ok(artifact.explanation, 'did not set debug string');
assert.equal(artifactWithQueryString.statusCode, -1);
assert.ok(artifactWithQueryString.explanation, 'did not set debug string');
assert.equal(artifactWithResponseNotFromSW.statusCode, -1);
assert.equal(artifactWithResponseNotFromSW.explanation,
'Unable to fetch start URL via service worker.');
const result = await gatherer.afterPass(passContext);
expect(result).toEqual({
statusCode: -1,
explanation: 'No usable web app manifest found on page.',
});
});

it('returns an artifact set to 200 when offline loading from service worker succeeds', () => {
const startUrlGatherer = new StartUrlGatherer();
const startUrlGathererWithFragment = new StartUrlGatherer();
it('returns a explanation when manifest cannot be parsed', async () => {
mockDriver.goOffline = jest.fn();
mockDriver.goOnline = jest.fn();

const passContext = {
url: 'https://ifixit-pwa.appspot.com/',
baseArtifacts: createArtifactsWithURL('https://ifixit-pwa.appspot.com/'),
driver: wrapSendCommand(mockDriver, 'https://ifixit-pwa.appspot.com/', 200, true),
baseArtifacts: {
WebAppManifest: parseManifest(
'this is invalid',
'https://example.com/manifest.json',
'https://example.com/'
)
},
driver: mockDriver,
};
const passContextWithFragment = {
url: 'https://ifixit-pwa.appspot.com/#/history',
baseArtifacts: createArtifactsWithURL('https://ifixit-pwa.appspot.com/#/history'),
driver: wrapSendCommand(mockDriver, 'https://ifixit-pwa.appspot.com/#/history', 200, true),

const result = await gatherer.afterPass(passContext);
expect(mockDriver.goOffline).toHaveBeenCalled();
expect(mockDriver.goOnline).toHaveBeenCalled();
expect(result).toEqual({
statusCode: -1,
explanation: `Error fetching web app manifest: ERROR: file isn't valid JSON: ` +
`SyntaxError: Unexpected token h in JSON at position 1.`,
});
});

it('sets the status code to -1 when navigation fails', async () => {
mockDriver.goOffline = jest.fn();
mockDriver.goOnline = jest.fn();
mockDriver.evaluateAsync = jest.fn().mockRejectedValue(new Error('Fetch failed'));
mockDriver.on = jest.fn();
mockDriver.off = jest.fn();

const passContext = {
baseArtifacts: createArtifactsWithURL('https://example.com/'),
driver: mockDriver,
};

return Promise.all([
startUrlGatherer.afterPass(passContext),
startUrlGathererWithFragment.afterPass(passContextWithFragment),
]).then(([artifact, artifactWithFragment]) => {
assert.equal(artifact.statusCode, 200);
assert.equal(artifactWithFragment.statusCode, 200);
const result = await gatherer.afterPass(passContext);
expect(mockDriver.goOffline).toHaveBeenCalled();
expect(mockDriver.goOnline).toHaveBeenCalled();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about an extra level of checking for these that it was offline when evaluateAsync was called, not just that off and on were both called?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it as a separate test but not on every single one :) done!

expect(result).toEqual({
statusCode: -1,
explanation: 'Unable to fetch start URL via service worker.',
});
});

it('returns a explanation when manifest cannot be found', () => {
const driver = Object.assign({}, mockDriver);
const startUrlGatherer = new StartUrlGatherer();
it('sets the status code to page status code', async () => {
mockDriver.goOffline = jest.fn();
mockDriver.goOnline = jest.fn();
mockDriver.evaluateAsync = jest.fn().mockResolvedValue();
mockDriver.on = jest.fn();
mockDriver.off = jest.fn();

const passContext = {
baseArtifacts,
url: 'https://ifixit-pwa.appspot.com/',
driver,
baseArtifacts: createArtifactsWithURL('https://example.com/'),
driver: mockDriver,
};

return startUrlGatherer.afterPass(passContext)
.then(artifact => {
assert.equal(artifact.explanation,
`No usable web app manifest found on page.`);
});
const response = {
url: 'https://example.com/',
status: 200,
fromServiceWorker: true,
};

mockDriver.on.mockImplementation((_, onResponseReceived) => onResponseReceived({response}));

const result = await gatherer.afterPass(passContext);
expect(mockDriver.goOffline).toHaveBeenCalled();
expect(mockDriver.goOnline).toHaveBeenCalled();
expect(result).toEqual({
statusCode: 200,
});
});

it('returns a explanation when manifest cannot be parsed', () => {
const driver = Object.assign({}, mockDriver);
const startUrlGatherer = new StartUrlGatherer();
it('sets the status code to -1 when not from service worker', async () => {
mockDriver.goOffline = jest.fn();
mockDriver.goOnline = jest.fn();
mockDriver.evaluateAsync = jest.fn().mockResolvedValue();
mockDriver.on = jest.fn();
mockDriver.off = jest.fn();

const passContext = {
baseArtifacts,
url: 'https://ifixit-pwa.appspot.com/',
driver,
baseArtifacts: createArtifactsWithURL('https://example.com/'),
driver: mockDriver,
};

const response = {
url: 'https://example.com/',
status: 200,
fromServiceWorker: false,
};

baseArtifacts.WebAppManifest = parseManifest(
'this is invalid',
'https://ifixit-pwa.appspot.com/manifest.json',
passContext.url
);

return startUrlGatherer.afterPass(passContext)
.then(artifact => {
assert.equal(artifact.explanation,
`Error fetching web app manifest: ERROR: file isn't valid JSON: ` +
`SyntaxError: Unexpected token h in JSON at position 1.`);
});
mockDriver.on.mockImplementation((_, onResponseReceived) => onResponseReceived({response}));

const result = await gatherer.afterPass(passContext);
expect(mockDriver.goOffline).toHaveBeenCalled();
expect(mockDriver.goOnline).toHaveBeenCalled();
expect(result).toEqual({
statusCode: -1,
explanation: 'Fetched start URL but not via service worker.',
});
});

it('times out when a start_url is too slow to respond', async () => {
const startUrlGatherer = new StartUrlGatherer();
it('sets the status code to -1 when times out waiting for a matching url', async () => {
mockDriver.goOffline = jest.fn();
mockDriver.goOnline = jest.fn();
mockDriver.evaluateAsync = jest.fn().mockResolvedValue();
mockDriver.on = jest.fn();
mockDriver.off = jest.fn();

const passContext = {
url: 'https://ifixit-pwa.appspot.com/',
baseArtifacts: createArtifactsWithURL('https://ifixit-pwa.appspot.com/'),
driver: wrapSendCommand(mockDriver, ''),
baseArtifacts: createArtifactsWithURL('https://example.com/'),
driver: mockDriver,
};

const response = {
url: 'https://random-other-url.com/',
status: 200,
fromServiceWorker: true,
};

const resultPromise = startUrlGatherer.afterPass(passContext);
mockDriver.on.mockImplementation((_, onResponseReceived) => onResponseReceived({response}));

const resultPromise = gatherer.afterPass(passContext);
// Wait a tick for the evaluateAsync promise resolution to hold for the timer.
await Promise.resolve();
// Skip past our timeout of 3000.
jest.advanceTimersByTime(5000);
const artifact = await resultPromise;
assert.equal(artifact.explanation, 'Timed out waiting for fetched start_url.');
const result = await resultPromise;

expect(mockDriver.goOffline).toHaveBeenCalled();
expect(mockDriver.goOnline).toHaveBeenCalled();
expect(result).toEqual({
statusCode: -1,
explanation: 'Timed out waiting for fetched start_url.',
});
});
});