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
6 changes: 5 additions & 1 deletion lighthouse-core/audits/webapp-install-banner.js
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
2 changes: 1 addition & 1 deletion lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,7 @@ class Driver {
}

// If both the data and the url are empty strings, the page had no manifest.
return reject('No web app manifest found.');
return resolve(null);
Copy link
Member

Choose a reason for hiding this comment

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

🎉

Copy link
Member

Choose a reason for hiding this comment

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

do you want to flatten the promise setup here while you're in here? Getting rid of the surrounding promise and then the resolve and rejects should be able to just be return/throw

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure

}

resolve(response);
Expand Down
9 changes: 1 addition & 8 deletions lighthouse-core/gather/gatherers/manifest.js
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
23 changes: 15 additions & 8 deletions lighthouse-core/gather/gatherers/start-url.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,24 +49,29 @@ 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.start_url || !manifest.value.start_url.raw) {
Copy link
Member

Choose a reason for hiding this comment

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

I may have been the cause of this original conditional, but I don't think it's quite right.

  • !manifest makes sense now (in case getAppManifest returned null, the real no-manifest case)
  • if there's a manifest, then manifest.value can be undefined if the manifest was malformed JSON
  • if there's a manifest and a manifest.value there will always be a manifest.value.start_url, as even if it's missing or the completely wrong type in the raw manifest, start_url will then default to the document URL

(!manifest || !manifest.value) should be sufficient, and up to you if you want to make the error strings different for the two cases

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

throw new Error(`No web app manifest found on page ${options.url}`);
}

if (manifest.value.start_url.debugString) {
return Promise.reject(new Error(manifest.value.start_url.debugString));
throw new Error(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.

do we want this to be an error? It'll be things like invalid start_url relative to ${manifestUrl} or start_url must be same-origin as document, which is nice to put in front of the developer, but the parsed manifest will still have a valid start_url in those cases.

Not sure if we want to fall on the "this is broken" side or the side of how the browser actually treats it since we don't have a great logging spot for that type of message if no error here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it wasn't it was being caught below

}

this.startUrl = manifest.value.start_url.value;
}).then(_ => this.executeFetchRequest(options.driver, this.startUrl));
})
.then(_ => this.executeFetchRequest(options.driver, this.startUrl))
.catch(err => {
this.debugString = err.message;
Copy link
Member

Choose a reason for hiding this comment

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

probably better to set this.debugString above and return rather than throwing errors and catching everything here. Doing that would also let errors from getAppManifest, the manifest parser, and executeFetchRequest get through without all being turned into debugStrings

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that was kinda the point though since all of this could reasonably fail not because of a bug but just because of the page not having a service worker, fine with switching for now though and going back to this if it's too noisy

});
}

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

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});
Copy link
Member

Choose a reason for hiding this comment

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

statusCode of -1 in this 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.

done

}

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

return options.driver.goOnline(options)
.then(_ => navigationRecord ? navigationRecord.statusCode : -1);
.then(_ => {
return {statusCode: navigationRecord ? navigationRecord.statusCode : -1};
Copy link
Member

Choose a reason for hiding this comment

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

debugString in the -1 case?

});
}
}

Expand Down
19 changes: 16 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,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
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
24 changes: 9 additions & 15 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,8 @@ describe('Start-url gatherer', () => {
startUrlGathererWithQueryString.pass(optionsWithQueryString)
.then(_ => startUrlGathererWithQueryString.afterPass(optionsWithQueryString, tracingData))
]).then(([artifact, artifactWithQueryString]) => {
assert.strictEqual(artifact, -1);
assert.strictEqual(artifactWithQueryString, -1);
assert.deepEqual(artifact, {statusCode: -1});
assert.deepEqual(artifactWithQueryString, {statusCode: -1});
});
});

Expand All @@ -113,12 +113,12 @@ 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/',
Expand All @@ -127,11 +127,8 @@ describe('Start-url gatherer', () => {

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, `No web app manifest found on page ${options.url}`);
});
});

Expand All @@ -144,11 +141,8 @@ describe('Start-url gatherer', () => {

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