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

Jump to bottom

pipe manifest fetch/parse error strings up for display #102

Merged
merged 3 commits into from
Mar 31, 2016
Merged

pipe manifest fetch/parse error strings up for display #102

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d34914
pipe manifest fetch/parse error strings back to cli
brendankenny Mar 30, 2016
c0172b1
add debugString output to extension
brendankenny Mar 30, 2016
78e8b7c
convert manifest parser to also use `debugString` for its debug strings
brendankenny Mar 31, 2016
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
8 changes: 5 additions & 3 deletions audits/audit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,14 @@ class Audit {
/**
* @param {(boolean|number|string)} value
* @param {?(boolean|number|string)=} rawValue
* @param {string=} debugString Optional string to describe any error condition encountered.
* @return {!AuditResult}
*/
static generateAuditResult(value, rawValue) {
static generateAuditResult(value, rawValue, debugString) {
return {
value: value,
rawValue: rawValue,
value,
Copy link
Contributor

Choose a reason for hiding this comment

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

personally i really hate mixing the : syntax it is very confusing ... but maybe it is something to get used to for me.

rawValue,
debugString,
name: this.name,
tags: this.tags,
description: this.description
Expand Down
5 changes: 3 additions & 2 deletions audits/manifest/background-color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class ManifestBackgroundColor extends Audit {
}

/**
* @param {!Manifest} manifest
* @param {!Manifest=} manifest
* @return {boolean}
*/
static hasBackgroundColorValue(manifest) {
Expand All @@ -56,7 +56,8 @@ class ManifestBackgroundColor extends Audit {
* @return {!AuditResult}
*/
static audit(artifacts) {
const hasBackgroundColor = ManifestBackgroundColor.hasBackgroundColorValue(artifacts.manifest);
const hasBackgroundColor = ManifestBackgroundColor
.hasBackgroundColorValue(artifacts.manifest.value);

return ManifestBackgroundColor.generateAuditResult(hasBackgroundColor);
}
Expand Down
4 changes: 3 additions & 1 deletion audits/manifest/exists.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,9 @@ class ManifestExists extends Audit {
*/
static audit(artifacts) {
return ManifestExists.generateAuditResult(
typeof artifacts.manifest !== 'undefined'
typeof artifacts.manifest.value !== 'undefined',
undefined,
Copy link
Member

Choose a reason for hiding this comment

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

what's this undefined arg?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's the audit's rawValue, whatever that is. It could still be a boolean in this case, but then it'll be displayed twice with the current cli pretty printing.

The only place this is used currently is in the FMP output, which puts the actual time in parentheses. There's probably a better way of piping that through

artifacts.manifest.debugString
);
}
}
Expand Down
2 changes: 1 addition & 1 deletion audits/manifest/icons-192.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class ManifestIcons192 extends Audit {
*/
static audit(artifacts) {
let hasIcons = false;
const manifest = artifacts.manifest;
const manifest = artifacts.manifest.value;

if (manifest && manifest.icons.value) {
const icons192 = manifest.icons.value.find(icon => {
Expand Down
4 changes: 2 additions & 2 deletions audits/manifest/icons.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ class ManifestIcons extends Audit {
*/
static audit(artifacts) {
let hasIcons = false;
const manifest = artifacts.manifest;
const manifest = artifacts.manifest.value;

if (manifest && manifest.icons && manifest.icons.value) {
if (manifest && manifest.icons.value) {
hasIcons = (manifest.icons.value.length > 0);
}

Expand Down
2 changes: 1 addition & 1 deletion audits/manifest/name.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class ManifestName extends Audit {
*/
static audit(artifacts) {
let hasName = false;
const manifest = artifacts.manifest;
const manifest = artifacts.manifest.value;

if (manifest && manifest.name) {
hasName = (!!manifest.name.value);
Expand Down
2 changes: 1 addition & 1 deletion audits/manifest/short-name.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class ManifestShortName extends Audit {
*/
static audit(artifacts) {
let hasShortName = false;
const manifest = artifacts.manifest;
const manifest = artifacts.manifest.value;

if (manifest && manifest.short_name) {
hasShortName = (!!manifest.short_name.value);
Expand Down
2 changes: 1 addition & 1 deletion audits/manifest/start-url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class ManifestStartUrl extends Audit {
*/
static audit(artifacts) {
let hasStartUrl = false;
const manifest = artifacts.manifest;
const manifest = artifacts.manifest.value;

if (manifest && manifest.start_url) {
hasStartUrl = (!!manifest.start_url.value);
Expand Down
2 changes: 1 addition & 1 deletion audits/manifest/theme-color.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class ManifestThemeColor extends Audit {
*/
static audit(artifacts) {
let hasThemeColor = false;
const manifest = artifacts.manifest;
const manifest = artifacts.manifest.value;

if (manifest && manifest.theme_color) {
hasThemeColor = (!!manifest.theme_color.value);
Expand Down
3 changes: 3 additions & 0 deletions cli/printer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ const Printer = {
lineItem += ` (${subitem.rawValue})`;
}
output.log(lineItem);
if (subitem.debugString) {
output.log(` ${subitem.debugString}`);
}
});

output.log('');
Expand Down
2 changes: 1 addition & 1 deletion closure/typedefs/Artifacts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ Artifacts.prototype.networkRecords;
/** @type {?} */
Artifacts.prototype.traceContents;

/** @type {!Manifest} */
/** @type {!ManifestNode<(!Manifest|undefined)>} */
Artifacts.prototype.manifest;

/** @type {!ServiceWorkerVersions} */
Expand Down
3 changes: 3 additions & 0 deletions closure/typedefs/AuditResult.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ AuditResult.prototype.value;
/** @type {(boolean|number|string|undefined|null)} */
AuditResult.prototype.rawValue;

/** @type {(string|undefined)} */
AuditResult.prototype.debugString;

/** @type {string} */
AuditResult.prototype.name;

Expand Down
2 changes: 1 addition & 1 deletion closure/typedefs/Manifest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ ManifestNode.prototype.raw;
ManifestNode.prototype.value;

/** @type {string} */
ManifestNode.prototype.warning;
ManifestNode.prototype.debugString;

/**
* @struct
Expand Down
2 changes: 0 additions & 2 deletions extension/app/scripts.babel/app.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ document.addEventListener('DOMContentLoaded', _ => {

runPwaAudits(chrome).then(ret => {
resultsEl.innerHTML = ret;
}, err => {
resultsEl.innerHTML = `<div class="error">Unable to audit page: ${err.message}</div>`;
});
});

Expand Down
21 changes: 17 additions & 4 deletions extension/app/scripts.babel/pwa-check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,15 @@ const aggregators = [
require('../../../aggregators/is-sized-for-mobile-screen')
];

function escapeHTML(str) {
Copy link
Contributor

Choose a reason for hiding this comment

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

😬

return str.replace(/&/g, '&amp;')
.replace(/"/g, '&quot;')
.replace(/'/g, '&#39;')
.replace(/>/g, '&gt;')
.replace(/</g, '&lt;')
.replace(/`/g, '&#96;');
}

function createResultsHTML(results) {
let resultsHTML = '';

Expand All @@ -65,15 +74,18 @@ function createResultsHTML(results) {

let groupHTML = '';
item.score.subItems.forEach(subitem => {
const debugString = subitem.debugString ? ` title="${escapeHTML(subitem.debugString)}"` : '';

// TODO: make this work with numeric values.
const status = subitem.value ?
'<span class="pass">Pass</span>' : '<span class="fail">Fail</span>';
groupHTML += `<li>${subitem.description}: ${status}</li>`;
`<span class="pass" ${debugString}>Pass</span>` :
`<span class="fail" ${debugString}>Fail</span>`;
groupHTML += `<li>${escapeHTML(subitem.description)}: ${status}</li>`;
});

resultsHTML +=
`<li class="${groupClass}">
<span class="group-name">${item.name}</span>
<span class="group-name">${escapeHTML(item.name)}</span>
<span class="group-score">(${score}%)</span>
<ul>
${groupHTML}
Expand All @@ -91,5 +103,6 @@ export function runPwaAudits() {
.then(results => Aggregator.aggregate(aggregators, results))
.then(results => {
return createResultsHTML(results);
});
})
.catch(err => `<div class="error">Unable to audit page: ${escapeHTML(err.message)}</div>`);
}
5 changes: 5 additions & 0 deletions extension/app/styles/lighthouse.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,11 @@ header h2 {
margin: 0;
}

.results [title] {
cursor: help;
border-bottom: 1px dashed black;
}

.results .pass {
color: #44A736;
}
Expand Down
40 changes: 32 additions & 8 deletions gatherers/manifest.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,25 @@ class Manifest extends Gather {
const finalURL = url.resolve(options.url, manifestURL);

request(finalURL, function(err, response, body) {
Copy link
Contributor

Choose a reason for hiding this comment

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

we should add a test here around our handling of the 400+ response code.

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, what do you mean? We don't have any gatherers tests yet, and by the time it gets up to audits it's just a debugString, which we do test for correctly passing through.

Another twist on this is that this will all change when we switch to letting the page fetch the manifest in #83 (which I'm working on next)

Copy link
Contributor

Choose a reason for hiding this comment

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

k

if (err) {
return resolve('');
if (err || response.statusCode >= 400) {
return reject(`${response.statusCode}: ${response.statusMessage}`);
}

resolve(body);
});
});
}

static _errorManifest(errorString) {
return {
manifest: {
raw: undefined,
value: undefined,
debugString: errorString
}
};
}

static gather(options) {
const driver = options.driver;
/**
Expand All @@ -52,12 +62,26 @@ class Manifest extends Gather {
* resource is tracked in issue #83
*/
return driver.querySelector('head link[rel="manifest"]')
.then(node => node && node.getAttribute('href'))
.then(manifestURL => manifestURL && Manifest._loadFromURL(options, manifestURL))
.then(manifestContent => {
return {
manifest: manifestParser(manifestContent).value
};
.then(node => {
if (!node) {
return this._errorManifest('No <link rel="manifest"> found in DOM.');
}

return node.getAttribute('href').then(manifestURL => {
if (!manifestURL) {
return this._errorManifest('No href found on <link rel="manifest">.');
}

return Manifest._loadFromURL(options, manifestURL)
.then(manifestContent => {
return {
manifest: manifestParser(manifestContent)
};
})
.catch(reason => {
return this._errorManifest(`Unable to fetch manifest at ${manifestURL}: ${reason}.`);
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like we still need to add tests around these error messages.

Copy link
Member Author

Choose a reason for hiding this comment

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

added a simple pass-through check. The more involved tests will be on the gatherer itself.

});
});
});
}
}
Expand Down
Loading