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.

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
3 changes: 2 additions & 1 deletion lighthouse-core/audits/multi-check-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ class MultiCheckAudit extends Audit {
// Otherwise, we pass
return {
rawValue: true,
extendedInfo
extendedInfo,
debugString: result.debugString,
};
}

Expand Down
34 changes: 20 additions & 14 deletions lighthouse-core/audits/webapp-install-banner.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ class WebappInstallBanner extends MultiCheckAudit {
};
}

static assessManifest(manifestValues, failures) {
static assessManifest(artifacts, result) {
const {manifestValues, failures} = result;
if (manifestValues.isParseFailure) {
failures.push(manifestValues.parseFailureReason);
return;
Expand All @@ -66,32 +67,37 @@ class WebappInstallBanner extends MultiCheckAudit {
}


static assessServiceWorker(artifacts, failures) {
static assessServiceWorker(artifacts, result) {
const hasServiceWorker = SWAudit.audit(artifacts).rawValue;
if (!hasServiceWorker) {
failures.push('Site does not register a Service Worker');
result.failures.push('Site does not register a Service Worker');
}
}

static assessOfflineStartUrl(artifacts, failures) {
const hasOfflineStartUrl = artifacts.StartUrl === 200;
static assessOfflineStartUrl(artifacts, result) {
const hasOfflineStartUrl = artifacts.StartUrl.statusCode === 200;

if (!hasOfflineStartUrl) {
failures.push('Manifest start_url is not cached by a Service Worker');
result.failures.push('Manifest start_url is not cached by a Service Worker');

if (artifacts.StartUrl.debugString) {
result.failures.push(artifacts.StartUrl.debugString);
}
} else if (artifacts.StartUrl.debugString) {
result.debugString = artifacts.StartUrl.debugString;
Copy link
Member

Choose a reason for hiding this comment

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

weird, I left this comment before (and it was in the email), but it was eaten in the web interface:

MultiCheckAudit is already somewhat confusing and this doesn't exactly simplify things :)

Short of a MultiCheckAudit refactor, what about something like a warnings array, which is used for debugString, just like failures but they won't fail the audit?

Just an idea, maybe we can come up with something simpler/more self-explanatory

so here (for this idea) it would result.warnings.push(artifacts.StartUrl.debugString); whether or not it hasOfflineStartUrl, and then MultiCheckAudit would handle appending result.warnings.join(', ') to the debugString

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

}
}

static audit_(artifacts) {
const failures = [];

return artifacts.requestManifestValues(artifacts.Manifest).then(manifestValues => {
WebappInstallBanner.assessManifest(manifestValues, failures);
WebappInstallBanner.assessServiceWorker(artifacts, failures);
WebappInstallBanner.assessOfflineStartUrl(artifacts, failures);

return {
failures,
manifestValues
};
const result = {failures, manifestValues};
Copy link
Member

Choose a reason for hiding this comment

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

if result already has failures and manifestValues on it but all three are being passed into these methods, should they instead just have a single result parameter?

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

WebappInstallBanner.assessManifest(artifacts, result);
WebappInstallBanner.assessServiceWorker(artifacts, result);
WebappInstallBanner.assessOfflineStartUrl(artifacts, result);

return result;
});
}
}
Expand Down
29 changes: 11 additions & 18 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -265,25 +265,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

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
11 changes: 4 additions & 7 deletions lighthouse-core/gather/gatherers/manifest.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,16 @@ class Manifest extends Gatherer {
afterPass(options) {
return options.driver.getAppManifest()
.then(response => {
if (!response) {
return null;
}

const isBomEncoded = response.data.charCodeAt(0) === BOM_FIRSTCHAR;
if (isBomEncoded) {
response.data = Buffer.from(response.data).slice(BOM_LENGTH).toString();
}

return manifestParser(response.data, response.url, options.url);
})
.catch(err => {
if (err === 'No web app manifest found.') {
return null;
}

return Promise.reject(err);
});
}
}
Expand Down
40 changes: 30 additions & 10 deletions lighthouse-core/gather/gatherers/start-url.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,34 +49,54 @@ 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) {
const detailedMsg = manifest && manifest.debugString;
this.debugString = detailedMsg ?
`Error fetching web app manifest: ${detailedMsg}` :
`No usable 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));
// Even if the start URL had an error, the browser will still supply a fallback URL.
// Therefore, we only set the debugString here and continue with the fetch.
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

}

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'));
}

const networkRecords = tracingData.networkRecords;
const navigationRecord = networkRecords.filter(record => {
return URL.equalWithExcludedFragments(record._url, this.startUrl) &&
record._fetchedViaServiceWorker;
}).pop(); // Take the last record that matches.

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

Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/lib/url-shim.js
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ URL.elideDataURI = function elideDataURI(url) {
// Why? Special handling was added by Chrome team to allow a pushState transition between chrome:// pages.
// As a result, the network URL (chrome://chrome/settings/) doesn't match the final document URL (chrome://settings/).
function rewriteChromeInternalUrl(url) {
if (!url.startsWith('chrome://')) return url;
if (!url || !url.startsWith('chrome://')) return url;
Copy link
Member

Choose a reason for hiding this comment

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

what new path needs this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was a drive-by fix of something I encountered on another branch, can't remember what site now though it was over a month ago :P

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but to answer your question the equalWithExcludedFragments can now be run on a non-existent startUrl which causes this to fail for every site that doesn't have the manifest

Copy link
Member

Choose a reason for hiding this comment

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

but to answer your question the equalWithExcludedFragments can now be run on a non-existent startUrl which causes this to fail for every site that doesn't have the manifest

ha, I had forgotten we had done that. I guess that's fine vs moving the .map(rewriteChromeInternalUrl); into the try/catch

return url.replace(/^chrome:\/\/chrome\//, 'chrome://');
}

Expand Down
16 changes: 13 additions & 3 deletions lighthouse-core/test/audits/webapp-install-banner-test.js
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,14 @@ describe('PWA: webapp install banner audit', () => {
assert.strictEqual(failures.length, 1, failures);
});
});

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

return WebappInstallBannerAudit.audit(artifacts).then(result => {
assert.strictEqual(result.rawValue, true);
assert.equal(result.debugString, 'Warning!');
});
});
});
2 changes: 1 addition & 1 deletion lighthouse-core/test/gather/gatherers/manifest-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,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
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.equal(artifact.statusCode, 200);
assert.equal(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');
});
});
});