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

report: audit warnings are not top-level #5270

Merged
merged 8 commits into from
May 24, 2018
Merged

report: audit warnings are not top-level #5270

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
5f6f86d
dont emit warnings to toplevel
paulirish May 19, 2018
aa249d4
renderer side of warnings.
paulirish May 19, 2018
05f3922
tests: address phulce's skipped test.
paulirish May 19, 2018
078c2b0
patrick feeedback.
paulirish May 21, 2018
949b591
wip new viz treatment to warnings.
paulirish May 23, 2018
836d9ef
warnings match visual spec.
paulirish May 23, 2018
f484b62
byebye lh-debug
paulirish May 23, 2018
0f0165a
feedback on tests.
paulirish May 23, 2018
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
4 changes: 2 additions & 2 deletions lighthouse-core/audits/viewport.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ class Viewport extends Audit {
const parsedProps = Parser.parseMetaViewPortContent(artifacts.Viewport);

if (Object.keys(parsedProps.unknownProperties).length) {
warnings.push(`Invalid properties found: ${JSON.stringify(parsedProps.unknownProperties)}.`);
warnings.push(`Invalid properties found: ${JSON.stringify(parsedProps.unknownProperties)}`);
}
if (Object.keys(parsedProps.invalidValues).length) {
warnings.push(`Invalid values found: ${JSON.stringify(parsedProps.invalidValues)}.`);
warnings.push(`Invalid values found: ${JSON.stringify(parsedProps.invalidValues)}`);
Copy link
Member

Choose a reason for hiding this comment

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

tests for these will need to be updated (they assert a . in there :)

}

const viewportProps = parsedProps.validProperties;
Expand Down
19 changes: 17 additions & 2 deletions lighthouse-core/report/html/renderer/category-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,23 @@ class CategoryRenderer {
const tooltip = this.dom.createChildOf(textEl, 'div', 'tooltip lh-debug');
tooltip.textContent = audit.result.errorMessage || 'Report error: no audit information';
} else if (audit.result.explanation) {
const explanationEl = this.dom.createChildOf(titleEl, 'div', 'lh-debug');
explanationEl.textContent = audit.result.explanation;
const explEl = this.dom.createChildOf(titleEl, 'div', 'lh-debug lh-debug--explanation');
explEl.textContent = audit.result.explanation;
}
const warnings = audit.result.warnings;
if (!warnings || warnings.length === 0) return auditEl;

// Add list of warnings or singular warning
const warningsEl = this.dom.createChildOf(titleEl, 'div', 'lh-debug lh-debug--warnings');
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we have any cases of warning + explanation? if so, does it still look okish :)

Copy link
Member Author

Choose a reason for hiding this comment

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

yup! looks ok

here's explanation + warnings + displayValue all at once
image

if (warnings.length === 1) {
warningsEl.textContent = `Warning: ${warnings.join('')}`;
} else {
warningsEl.textContent = 'Warnings: ';
const warningsUl = this.dom.createChildOf(warningsEl, 'ul');
for (const warning of warnings) {
const item = this.dom.createChildOf(warningsUl, 'li');
item.textContent = warning;
}
}
return auditEl;
}
Expand Down
4 changes: 3 additions & 1 deletion lighthouse-core/report/html/renderer/performance-category-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ class PerformanceCategoryRenderer extends CategoryRenderer {
valueEl.textContent = Util.formatDisplayValue(audit.result.displayValue);

const descriptionEl = this.dom.find('.lh-metric__description', tmpl);
descriptionEl.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description));
if (audit.result.description) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

a little confused by removing the ? from description but making this conditional... what's up with that?

Copy link
Member Author

Choose a reason for hiding this comment

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

good catch. it's assumed to be there now.

descriptionEl.appendChild(this.dom.convertMarkdownLinkSnippets(audit.result.description));
}

if (audit.result.scoreDisplayMode === 'error') {
descriptionEl.textContent = '';
Expand Down
22 changes: 3 additions & 19 deletions lighthouse-core/report/html/renderer/report-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ class ReportRenderer {

/**
* Place the AuditResult into the auditDfn (which has just weight & group)
* @param {Object<string, AuditResultJSON>} audits
* @param {Object<string, LH.Audit.Result>} audits
* @param {Array<CategoryJSON>} reportCategories
*/
static smooshAuditResultsIntoCategories(audits, reportCategories) {
Expand All @@ -194,29 +194,13 @@ if (typeof module !== 'undefined' && module.exports) {
self.ReportRenderer = ReportRenderer;
}

/**
* @typedef {{
rawValue: (number|boolean|undefined),
id: string,
title: string,
description: string,
explanation?: string,
errorMessage?: string,
displayValue?: string|Array<string|number>,
scoreDisplayMode: string,
error: boolean,
score: (number|null),
details?: DetailsJSON,
}} AuditResultJSON
*/

/**
* @typedef {{
id: string,
score: (number|null),
weight: number,
group?: string,
result: AuditResultJSON
result: LH.Audit.Result
Copy link
Member

Choose a reason for hiding this comment

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

👍

}} AuditJSON
*/

Expand Down Expand Up @@ -248,7 +232,7 @@ if (typeof module !== 'undefined' && module.exports) {
finalUrl: string,
runWarnings?: Array<string>,
artifacts: {traces: {defaultPass: {traceEvents: Array}}},
audits: Object<string, AuditResultJSON>,
audits: Object<string, LH.Audit.Result>,
categories: Object<string, CategoryJSON>,
reportCategories: Array<CategoryJSON>,
categoryGroups: Object<string, GroupJSON>,
Expand Down
14 changes: 13 additions & 1 deletion lighthouse-core/report/html/report-styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,14 +589,26 @@
margin-top: 0;
}

.lh-audit__title .lh-debug {
margin-top: 3px;
}

.lh-debug {
font-size: var(--caption-font-size);
line-height: var(--caption-line-height);
color: var(--fail-color);
margin-bottom: 3px;
}

.lh-debug--explanation {
color: var(--fail-color);

}
.lh-debug--warnings {
color: var(--average-color);
}
.lh-debug--warnings ul {
margin: 0;
}

/* Report */

Expand Down
5 changes: 0 additions & 5 deletions lighthouse-core/runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,6 @@ class Runner {
const resultsById = {};
for (const audit of auditResults) {
resultsById[audit.id] = audit;

if (audit.warnings && audit.warnings.length) {
const prefixedWarnings = audit.warnings.map(msg => `${audit.title}: ${msg}`);
lighthouseRunWarnings.push(...prefixedWarnings);
}
}

/** @type {Object<string, LH.Result.Category>} */
Expand Down
28 changes: 11 additions & 17 deletions lighthouse-core/test/report/html/renderer/category-renderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,17 @@ describe('CategoryRenderer', () => {
assert.ok(auditDOM.matches('.lh-audit--informative'));
});

it('renders audits with warnings', () => {
const auditResult = {
title: 'Audit',
description: 'Learn more',
warnings: ['It may not have worked!'],
score: 1,
};
const auditDOM = renderer.renderAudit({id: 'foo', score: 1, result: auditResult});
assert.ok(auditDOM.querySelector('.lh-debug--warnings'), 'did not render debug message');
Copy link
Member

Choose a reason for hiding this comment

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

want to test at least that the warning text is in there as well? And probably want a plural/bulleted test in addition

});

it('renders a category', () => {
const category = sampleResults.reportCategories.find(c => c.id === 'pwa');
const categoryDOM = renderer.render(category, sampleResults.categoryGroups);
Expand Down Expand Up @@ -114,23 +125,6 @@ describe('CategoryRenderer', () => {
category.description = prevDesc;
});

// TODO(phulce): revisit if top-level warnings approach is too noisy
it.skip('renders audits with warnings as failed', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to render an audit with warnings as failed? or keep it in passed?

Copy link
Member Author

Choose a reason for hiding this comment

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

good policy question!

Let's see. here's the possible warnings we have right now (that are placed into Passed)

image
image
image

I see the argument for these being visible by default, however in the case of timing/decode, there's not much a user can do about them. Either way, to be visible by default, they'd have to be placed in "Failed" and either we keep the green checkmark or fail them. Both are pretty weird.

So I'm comfy with saying that warnings are orthogonal to passing/failing and default report visibility. wdyt?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I don't think there's much anyone can do about them either, I'm fine to hide especially now that we've got Sentry tracking 'em for Httparchive runs 👍

Copy link
Member

Choose a reason for hiding this comment

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

oh right, I was good with warnings and failures being independent of each other, but I forgot that means that passing audits with warnings would then be collapsed by default. That definitely takes away from the usefulness of warnings since that means they'll essentially never be seen if the audit is marked as passing.

I can kind of see an argument for hiding some warnings normally, not much the user can do about them, but things like "your manifest technically works but you have weird stuff in there that's getting fallbacks" seems worth bringing up even though the audit passed (and even the warnings about lighthouse's shortcomings might be relevant, which is why we made them warnings in the first place e.g. we can't actually verify all your external links or your images are encoded well except for this list of 20 images, which, while you can't make lighthouse fix, you can verify them yourself).

So while I would say that the existence of warnings should be independent of failure state, I would say that it's definitely not independent of whether or not to show them, and marking the audit as failed doesn't seem like a good way to handle that.

Copy link
Member

Choose a reason for hiding this comment

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

My suggestion in #5273 (comment) was to use warnings as a way to get attention to an issue without penalizing the score. So I would definitely prefer for the warning to be visible by default (not collapsed). I understand that warnings may also be used for things outside of the developer's control so collapsing may make more sense. Is this something that can be configured by the audit at runtime, rather than forcing all warnings to behave the same?

I'd also love to see different UI treatment for warnings; not a green check and not a red exclamation. Could it be a yellow/orange exclamation to distinguish it that something's amiss?

Copy link

@hwikyounglee hwikyounglee May 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Here're some ideas to visually distinguish the orthogonal warnings from audits.

inlinewarning-ideas

I think A is similar to the top-level warning treatment. Probably consider changing the text to dark for legibility.

Update: B seems to make things more visible (recommended).

Copy link

@hwikyounglee hwikyounglee May 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

And for warnings that are associated with passed audits (green checkmarks), how about collating them right below the Passed Audits zippy. So those warnings have visibility and draw attention while keeping the passed audits collapsed.

Updated:
inlinewarning-for-passed-audits-ideas

Copy link
Member Author

Choose a reason for hiding this comment

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

Sg. broke this into it's own issue: #5327 ("make sure warnings within Passed Audits are visible")

const auditResult = {
title: 'Audit',
description: 'Learn more',
warnings: ['It may not have worked!'],
score: 1,
};
const audit = {result: auditResult, score: 1};
const category = {name: 'Fake', description: '', score: 1, audits: [audit]};
const categoryDOM = renderer.render(category, sampleResults.reportGroups);
assert.ok(categoryDOM.querySelector(
'.lh-category > .lh-audit-group:not(.lh-passed-audits) > .lh-audit'),
'did not render as failed');
assert.ok(categoryDOM.querySelector('.lh-debug'), 'did not render debug message');
});

it('renders manual audits if the category contains them', () => {
const pwaCategory = sampleResults.reportCategories.find(cat => cat.id === 'pwa');
const categoryDOM = renderer.render(pwaCategory, sampleResults.categoryGroups);
Expand Down
6 changes: 4 additions & 2 deletions lighthouse-core/test/report/html/renderer/report-renderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,8 +135,10 @@ describe('ReportRenderer', () => {
const output = renderer.renderReport(sampleResults, container);

const warningEls = output.querySelectorAll('.lh-run-warnings > ul > li');
assert.strictEqual(warningEls.length, 1);
assert.ok(/Links.*unsafe/.test(warningEls[0].textContent), 'did not add warning text');
assert.strictEqual(warningEls.length, sampleResults.runWarnings.length);
Copy link
Member

Choose a reason for hiding this comment

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

there's already a "no lighthouseRunWarnings" test above this, so probably want to make a const warningResults = Object.assign({}, sampleResults, {runWarnings: ['Warnings are great!']}); or whatever and pass that in

for (const warningEl of warningEls) {
assert.ok(/Links.*unsafe/.test(warningEl.textContent), 'did not add warning text');
Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't this just be deleted now that warningEls should be length 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

good call.

}
});

it('renders a footer', () => {
Expand Down
4 changes: 1 addition & 3 deletions lighthouse-core/test/results/sample_v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,7 @@
"fetchTime": "2018-03-13T00:55:45.840Z",
"requestedUrl": "http://localhost/dobetterweb/dbw_tester.html",
"finalUrl": "http://localhost:10200/dobetterweb/dbw_tester.html",
"runWarnings": [
"Links to cross-origin destinations are unsafe: Unable to determine the destination for anchor tag. If not used as a hyperlink, consider removing target=_blank."
],
"runWarnings": [],
"audits": {
"is-on-https": {
"id": "is-on-https",
Expand Down
4 changes: 2 additions & 2 deletions typings/audit.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,9 +132,9 @@ declare global {
scoreDisplayMode: ScoreDisplayMode;
title: string;
id: string;
description?: string;
description: string;
// TODO(bckenny): define details
details?: object;
details?: any;
}

export interface Results {
Expand Down