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

refactor(StartUrl): switch from error to debugString object #2549

Merged
merged 10 commits into from
Jul 11, 2017
Merged

refactor(StartUrl): switch from error to debugString object #2549

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
fbd724e
refactor(StartUrl): switch from error to debugString object
patrickhulce Jun 20, 2017
d3c8dfd
feedback
patrickhulce Jun 21, 2017
f98d4b9
Merge branch 'master' into start_url_debug_string
patrickhulce Jun 22, 2017
6589cdd
allow debugString during passing
patrickhulce Jun 22, 2017
1cf9b18
fix test
patrickhulce Jun 23, 2017
369058d
pre-existing bug fix
patrickhulce Jun 23, 2017
6cefcd6
Merge branch 'master' into start_url_debug_string
patrickhulce Jun 29, 2017
5c9b84b
Merge branch 'master' into start_url_debug_string
patrickhulce Jul 11, 2017
4a0e7d4
more feedback
patrickhulce Jul 11, 2017
c552cab
more more feedback
patrickhulce Jul 11, 2017
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
6 changes: 5 additions & 1 deletion lighthouse-core/audits/webapp-install-banner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ class WebappInstallBanner extends MultiCheckAudit {
}

static assessOfflineStartUrl(artifacts, failures) {
const hasOfflineStartUrl = artifacts.StartUrl === 200;
const hasOfflineStartUrl = artifacts.StartUrl.statusCode === 200;
if (artifacts.StartUrl.debugString) {
failures.push(artifacts.StartUrl.debugString);
}

if (!hasOfflineStartUrl) {
failures.push('Manifest start_url is not cached by a Service Worker');
}
Expand Down
29 changes: 11 additions & 18 deletions lighthouse-core/gather/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,25 +264,18 @@ class Driver {
}

getAppManifest() {
return new Promise((resolve, reject) => {
this.sendCommand('Page.getAppManifest')
.then(response => {
// We're not reading `response.errors` however it may contain critical and noncritical
// errors from Blink's manifest parser:
// https://chromedevtools.github.io/debugger-protocol-viewer/tot/Page/#type-AppManifestError
if (!response.data) {
if (response.url) {
return reject(new Error(`Unable to retrieve manifest at ${response.url}.`));
}

// If both the data and the url are empty strings, the page had no manifest.
return reject('No web app manifest found.');
}
return this.sendCommand('Page.getAppManifest')
.then(response => {
// We're not reading `response.errors` however it may contain critical and noncritical
// errors from Blink's manifest parser:
// https://chromedevtools.github.io/debugger-protocol-viewer/tot/Page/#type-AppManifestError
if (!response.data) {
Copy link
Member

Choose a reason for hiding this comment

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

why drop the response.url check? If the manifest 404s, for instance, response.data will be undefined but that should probably be considered an error state, not that there is no manifest

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

IMO, anything that's not a bug should not say 'GathererError: blah', if the manifest 404s that's not a bug so it shouldn't be an error, at best we can give them the slightly different debug strings, is that what you're pushing back against?

Copy link
Member

Choose a reason for hiding this comment

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

Saying "No web app manifest found on page" is definitely wrong in that case, so a better error string is needed. But I'm not sure I agree about Gatherer errors. I have no problem with a method fetching a manifest throwing on a manifest URL that doesn't exist.

But I assume the issue is us conflating Lighthouse bugs with exceptional states? We may need to get back into that conversation to get the functionality we need. We don't want to slide back into -1 land where every audit has to handle all possible gatherer error states, but agreed we probably need a better separation between the classes of errors.

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 have no problem with a method fetching a manifest throwing on a manifest URL that doesn't exist.

I've got no problem with this throwing either just the overall pass throwing instead of returning debugString. There should be some distinguishing from a user's perspective between a bug they have no control over and something they need to take action on.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also re:

Saying "No web app manifest found on page" is definitely wrong in that case, so a better error string is needed.

I'd don't actually agree that "Unable to retrieve manifest" is better than "No web app manifest found on page", as a user receiving the former I may believe that it's LH problem not being able to retrieve it and there's nothing for me to do whereas the latter gets me to check my manifest. There's probably room for improvement on both, but calling it "definitely wrong" seems overly critical. It's not wrong, there was no usable manifest found.

Copy link
Member

Choose a reason for hiding this comment

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

maybe we need to start laying out the failure states for an audit ahead of time so we can decide on them once (in theory) and then it's obvious when they're implemented or not :)

If there's no <link rel="manifest"> in the page, it's obviously not an error, the audits simply just won't pass. However, to me, if there is a <link rel="manifest"> and the url 404s, that is an error worth reporting to the user, as much as a manifest made up of invalid JSON would be. They tried to include one and it's failing for some reason.

Agreed "Unable to retrieve manifest" is a bad error string for that and we should be as specific as possible to help with those situations. Also totally on board with exploring non-exception error returns, expected errors, etc after this so we can separate the two (or more?) clear classes of errors we're dealing with here.

Copy link
Member

@brendankenny brendankenny Jun 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

"Unable to retrieve manifest" is a bad error string for that and we should be as specific as possible to help with those situations

One downside of the current Page.getAppManifest protocol call is that it gives no network status information (as far as I can tell). So e.g. if the server returns a 404 with a Cannot GET /manifest.json response string, the protocol will just return a JSON.parse error from trying to parse that string (with no other indication of what happened) and we just have to infer that there was a 404. Maybe we could see about getting more detail there.

Copy link
Member

Choose a reason for hiding this comment

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

(the devtools Application panel suffers from the same problem, only showing "Errors and warnings: Line: 1, column: 1, Unexpected token." and you have to notice the 404 in the console/network panel to see what happened)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One downside of the current Page.getAppManifest protocol call is that it gives no network status information (as far as I can tell).

Yeah feels like it should return something similar to an artifact {value: null, debugString: ''} or whatever

// If the data is empty, the page had no manifest.
return null;
}

resolve(response);
})
.catch(err => reject(err));
});
return response;
});
}

getSecurityState() {
Expand Down
9 changes: 1 addition & 8 deletions lighthouse-core/gather/gatherers/manifest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,7 @@ class Manifest extends Gatherer {
afterPass(options) {
return options.driver.getAppManifest()
.then(response => {
return manifestParser(response.data, response.url, options.url);
})
.catch(err => {
if (err === 'No web app manifest found.') {
return null;
}

return Promise.reject(err);
return response && manifestParser(response.data, response.url, options.url);
});
}
}
Expand Down
27 changes: 19 additions & 8 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 @@ -49,24 +49,28 @@ class StartUrl extends Gatherer {
pass(options) {
return options.driver.getAppManifest()
.then(response => {
return manifestParser(response.data, response.url, options.url);
return response && manifestParser(response.data, response.url, options.url);
})
.then(manifest => {
if (!manifest.value.start_url || !manifest.value.start_url.raw) {
return Promise.reject(new Error(`No web app manifest found on page ${options.url}`));
if (!manifest || !manifest.value) {
this.debugString = `No web app manifest found on page ${options.url}`;
return;
}

if (manifest.value.start_url.debugString) {
return Promise.reject(new Error(manifest.value.start_url.debugString));
this.debugString = manifest.value.start_url.debugString;
Copy link
Member

Choose a reason for hiding this comment

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

should this be a state with a debugString and try to still fetch the start_url?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hm, why? there's nothing to fetch if the start_url is bad

Copy link
Member

Choose a reason for hiding this comment

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

hm, why? there's nothing to fetch if the start_url is bad

if there's a parseable JSON file there's always a good start_url, even if it has to fall back to just the page's URL.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh I see now, faulty start_urls are always ignored

debugString seems rather useless then there will always be a usable start_url...

Copy link
Member

Choose a reason for hiding this comment

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

debugString seems rather useless then there will always be a usable start_url

it is useful for debugging to see if your manifest isn't acting like you think it should...e.g. you have one start_url in mind and it's defaulting to another. That might be really hard to track down (or even notice that it's happening) otherwise.

I guess that makes me fall on the side of the current code and just show the error :) Lighthouse can be stricter than the spec in that case

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 guess this gets into the division of labor between DevTools and LH. LH shouldn't be your debugger IMO outside of general messages to get you on the right track/where to start looking. If everything is working from the PWA perspective (and document url was the right start url), do you really care about being flagged that you don't have a valid start url?

Either way, I added back the debug string to still do the check so if it passes it should pass with the debugString

Copy link
Member

Choose a reason for hiding this comment

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

Either way, I added back the debug string to still do the check so if it passes it should pass with the debugString

I think that's a good solution for now and the devtools/LH discussion is a good one to revisit now that Audits 2.0 is a real thing. Pavel and Paul also love drawing diagrams about that sort of division so I'm sure they'd be happy to get in on it :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pavel and Paul also love drawing diagrams about that sort of division so I'm sure they'd be happy to get in on it :)

haha :)

Copy link
Member

Choose a reason for hiding this comment

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

maybe a comment here as a reminder that even with a debugString there will be a fallback start_url

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

return;
}

this.startUrl = manifest.value.start_url.value;
}).then(_ => this.executeFetchRequest(options.driver, this.startUrl));
return this.executeFetchRequest(options.driver, this.startUrl);
});
}

afterPass(options, tracingData) {
if (!this.startUrl) {
return Promise.reject(new Error('No start_url found inside the manifest'));
if (this.debugString || !this.startUrl) {
Copy link
Member

Choose a reason for hiding this comment

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

seems like either there's a debugString or a startUrl for all possible states?

Copy link
Collaborator Author

@patrickhulce patrickhulce Jun 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

not if manifestParser/getAppManifest fails, but I guess that's a different can of worms altogether

Copy link
Member

Choose a reason for hiding this comment

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

not if manifestParser/getAppManifest fails, but I guess that's a different can of worms altogether

yeah, in that case (if pass rejects) afterPass should be ignored, but it will still run, which is weird. We should probably fix that...

const debugString = this.debugString || 'No start_url found inside the manifest';
return Promise.resolve({debugString, statusCode: -1});
}

const networkRecords = tracingData.networkRecords;
Expand All @@ -76,7 +80,14 @@ class StartUrl extends Gatherer {
}).pop(); // Take the last record that matches.

return options.driver.goOnline(options)
.then(_ => navigationRecord ? navigationRecord.statusCode : -1);
.then(_ => {
if (!navigationRecord) {
const debugString = 'Did not fetch start URL from service worker';
return {debugString, statusCode: -1};
}

return {statusCode: navigationRecord.statusCode};
});
}
}

Expand Down
19 changes: 16 additions & 3 deletions lighthouse-core/test/audits/webapp-install-banner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ function generateMockArtifacts() {
scriptURL: 'https://example.com/sw.js'
}]
},
StartUrl: 200,
StartUrl: {statusCode: 200},
URL: {finalUrl: 'https://example.com'}
}));
const mockArtifacts = Object.assign({}, computedArtifacts, clonedArtifacts);
Expand Down Expand Up @@ -128,7 +128,7 @@ describe('PWA: webapp install banner audit', () => {
it('fails if page had no SW', () => {
const artifacts = generateMockArtifacts();
artifacts.ServiceWorker.versions = [];
artifacts.StartUrl = -1;
artifacts.StartUrl = {statusCode: -1};

return WebappInstallBannerAudit.audit(artifacts).then(result => {
assert.strictEqual(result.rawValue, false);
Expand All @@ -141,7 +141,7 @@ describe('PWA: webapp install banner audit', () => {

it('fails if start_url is not cached', () => {
const artifacts = generateMockArtifacts();
artifacts.StartUrl = -1;
artifacts.StartUrl = {statusCode: -1};

return WebappInstallBannerAudit.audit(artifacts).then(result => {
assert.strictEqual(result.rawValue, false);
Expand All @@ -150,4 +150,17 @@ describe('PWA: webapp install banner audit', () => {
assert.strictEqual(failures.length, 1, failures);
});
});

it('includes debugString from start_url', () => {
const artifacts = generateMockArtifacts();
artifacts.StartUrl = {debugString: 'Oops it failed!'};

return WebappInstallBannerAudit.audit(artifacts).then(result => {
assert.strictEqual(result.rawValue, false);
assert.ok(result.debugString.includes('start_url'), result.debugString);
const failures = result.extendedInfo.value.failures;
assert.strictEqual(failures.length, 2, failures);
assert.equal(failures[0], 'Oops it failed!');
});
});
});
2 changes: 1 addition & 1 deletion lighthouse-core/test/gather/gatherers/manifest-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ describe('Manifest gatherer', () => {
return manifestGather.afterPass({
driver: {
getAppManifest() {
return Promise.reject('No web app manifest found.');
return Promise.resolve(null);
}
}
}).then(artifact => {
Expand Down
30 changes: 13 additions & 17 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 @@ -90,8 +90,10 @@ describe('Start-url gatherer', () => {
startUrlGathererWithQueryString.pass(optionsWithQueryString)
.then(_ => startUrlGathererWithQueryString.afterPass(optionsWithQueryString, tracingData))
]).then(([artifact, artifactWithQueryString]) => {
assert.strictEqual(artifact, -1);
assert.strictEqual(artifactWithQueryString, -1);
assert.equal(artifact.statusCode, -1);
assert.ok(artifact.debugString, 'did not set debug string');
assert.equal(artifactWithQueryString.statusCode, -1);
assert.ok(artifactWithQueryString.debugString, 'did not set debug string');
});
});

Expand All @@ -113,25 +115,22 @@ describe('Start-url gatherer', () => {
startUrlGathererWithFragment.pass(optionsWithQueryString)
.then(_ => startUrlGathererWithFragment.afterPass(optionsWithQueryString, tracingData))
]).then(([artifact, artifactWithFragment]) => {
assert.strictEqual(artifact, 200);
assert.strictEqual(artifactWithFragment, 200);
assert.deepEqual(artifact, {statusCode: 200});
assert.deepEqual(artifactWithFragment, {statusCode: 200});
});
});

it('returns an error when manifest cannot be found', () => {
it('returns a debugString when manifest cannot be found', () => {
const startUrlGatherer = new StartUrlGatherer();
const options = {
url: 'https://ifixit-pwa.appspot.com/',
driver: wrapSendCommand(mockDriver, '')
};

startUrlGatherer.pass(options)
return startUrlGatherer.pass(options)
.then(_ => startUrlGatherer.afterPass(options, tracingData))
.then(_ => {
assert.ok(false, 'should fail because manifest is empty');
})
.catch(err => {
assert.strictEqual(err.message, `No web app manifest found on page ${options.url}`);
.then(artifact => {
assert.equal(artifact.debugString, 'ERROR: start_url string empty');
});
});

Expand All @@ -142,13 +141,10 @@ describe('Start-url gatherer', () => {
driver: wrapSendCommand(mockDriver, 'https://not-same-origin.com/')
};

startUrlGatherer.pass(options)
return startUrlGatherer.pass(options)
.then(_ => startUrlGatherer.afterPass(options, tracingData))
.then(_ => {
assert.ok(false, 'should fail because origin is not the same');
})
.catch(err => {
assert.strictEqual(err.message, 'ERROR: start_url must be same-origin as document');
.then(artifact => {
assert.equal(artifact.debugString, 'ERROR: start_url must be same-origin as document');
});
});
});