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

core(installable-manifest): always run InstallabilityErrors in legacy mode #13622

Merged
merged 6 commits into from
Mar 23, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/smoke.yml
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ jobs:

- run: sudo apt-get install xvfb
- name: yarn smoke --fraggle-rock
run: xvfb-run --auto-servernum yarn smoke --debug --fraggle-rock -j=1 --retries=2 --invert-match pwa offline
run: xvfb-run --auto-servernum yarn smoke --debug --fraggle-rock -j=1 --retries=2

# Fail if any changes were written to source files.
- run: git diff --exit-code
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ const expectations = {
},
'installable-manifest': {
score: 0,
details: {items: [{reason: 'No manifest was fetched'}]},
details: {items: [{reason: 'Page has no manifest <link> URL'}]},
},
'splash-screen': {
score: 0,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const expectations = {
},
'installable-manifest': {
score: 0,
details: {items: [{reason: 'No manifest was fetched'}]},
details: {items: [{reason: 'Page has no manifest <link> URL'}]},
},
'splash-screen': {
score: 0,
Expand Down
17 changes: 11 additions & 6 deletions lighthouse-core/audits/installable-manifest.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ class InstallableManifest extends Audit {
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
supportedModes: ['navigation'],
requiredArtifacts: ['URL', 'WebAppManifest', 'InstallabilityErrors'],
requiredArtifacts: ['WebAppManifest', 'InstallabilityErrors'],
};
}

Expand Down Expand Up @@ -195,7 +195,6 @@ class InstallableManifest extends Audit {
*
*/
static async audit(artifacts, context) {
const manifestValues = await ManifestValues.request(artifacts, context);
const {i18nErrors, warnings} = InstallableManifest.getInstallabilityErrors(artifacts);

const manifestUrl = artifacts.WebAppManifest ? artifacts.WebAppManifest.url : null;
Expand All @@ -210,10 +209,16 @@ class InstallableManifest extends Audit {
const errorReasons = i18nErrors.map(reason => {
return {reason};
});
/** DevTools InstallabilityErrors does not emit an error unless there is a manifest, so include manifestValues's error */
if (manifestValues.isParseFailure) {
errorReasons.push({
reason: manifestValues.parseFailureReason});

// If InstallabilityErrors is empty, double check ManifestValues to make sure nothing was missed.
// InstallabilityErrors can be empty erroneously in our DevTools web tests.
Copy link
Member Author

Choose a reason for hiding this comment

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

For some reason, InstallabilityErrors is empty in our DevTools web tests even though no manifest was fetched. Assuming this is a valid possibility, we can fall back to the old manifest failure reason.

if (!errorReasons.length) {
const manifestValues = await ManifestValues.request(artifacts, context);
if (manifestValues.isParseFailure) {
errorReasons.push({
adamraine marked this conversation as resolved.
Show resolved Hide resolved
reason: manifestValues.parseFailureReason,
});
}
}

// Include the detailed pass/fail checklist as a diagnostic.
Expand Down
6 changes: 2 additions & 4 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -438,10 +438,8 @@ class GatherRunner {
}

try {
if (baseArtifacts.WebAppManifest) {
baseArtifacts.InstallabilityErrors = await InstallabilityErrors.getInstallabilityErrors(
passContext.driver.defaultSession);
}
baseArtifacts.InstallabilityErrors = await InstallabilityErrors.getInstallabilityErrors(
passContext.driver.defaultSession);
} catch (err) {
log.error('GatherRunner InstallabilityErrors', err);
baseArtifacts.InstallabilityErrors = {
Expand Down
3 changes: 2 additions & 1 deletion lighthouse-core/test/audits/installable-manifest-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,12 @@ describe('PWA: webapp install banner audit', () => {

it('fails with a non-parsable manifest', () => {
const artifacts = generateMockArtifacts('{,:}');
artifacts.InstallabilityErrors.errors.push({errorId: 'manifest-empty', errorArguments: []});
const context = generateMockAuditContext();
return InstallableManifestAudit.audit(artifacts, context).then(result => {
assert.strictEqual(result.score, 0);
const items = result.details.items;
assert.ok(items[0].reason.includes('failed to parse as valid JSON'));
expect(items[0].reason).toBeDisplayString(/could not be parsed/);
});
});

Expand Down
7 changes: 6 additions & 1 deletion lighthouse-core/test/results/artifacts/artifacts.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@
"BenchmarkIndex": 1549,
"WebAppManifest": null,
"InstallabilityErrors": {
"errors": []
"errors": [
{
"errorId": "no-manifest",
"errorArguments": []
}
]
},
"Stacks": [
{
Expand Down
11 changes: 7 additions & 4 deletions lighthouse-core/test/results/sample_v2.json
Original file line number Diff line number Diff line change
Expand Up @@ -799,7 +799,7 @@
],
"items": [
{
"reason": "No manifest was fetched"
"reason": "Page has no manifest <link> URL"
}
],
"debugData": {
Expand Down Expand Up @@ -7143,19 +7143,19 @@
},
{
"startTime": 0,
"name": "lh:computed:ManifestValues",
"name": "lh:audit:apple-touch-icon",
"duration": 100,
"entryType": "measure"
},
{
"startTime": 0,
"name": "lh:audit:apple-touch-icon",
"name": "lh:audit:splash-screen",
"duration": 100,
"entryType": "measure"
},
{
"startTime": 0,
"name": "lh:audit:splash-screen",
"name": "lh:computed:ManifestValues",
"duration": 100,
"entryType": "measure"
},
Expand Down Expand Up @@ -8388,6 +8388,9 @@
"lighthouse-core/audits/installable-manifest.js | columnValue": [
"audits[installable-manifest].details.headings[0].text"
],
"lighthouse-core/audits/installable-manifest.js | no-manifest": [
"audits[installable-manifest].details.items[0].reason"
],
"lighthouse-core/audits/apple-touch-icon.js | failureTitle": [
"audits[apple-touch-icon].title"
],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ Gathering: SourceMaps
Gathering: FullPageScreenshot
Populate base artifacts
Get webapp manifest
Get webapp installability errors
Collect stacks
Running offlinePass pass
Resetting state with about:blank
Expand Down