Skip to content

core(errors-in-console): add ignoredPatterns option #9480

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

Merged
paulirish merged 7 commits into from
Oct 11, 2019
Merged

core(errors-in-console): add ignoredPatterns option #9480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6304c0b
core(errors-in-console): add ignoredPatterns option
patrickhulce Jul 29, 2019
76f0ae4
Merge branch 'master' into expected_console_errors
patrickhulce Oct 11, 2019
fb75724
sample json
patrickhulce Oct 11, 2019
123283d
lint
patrickhulce Oct 11, 2019
e22163f
test update
patrickhulce Oct 11, 2019
c3ee28d
optional
patrickhulce Oct 11, 2019
eb8a3f4
ignore a new error instead
patrickhulce Oct 11, 2019
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
3 changes: 3 additions & 0 deletions lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,9 @@
<!-- FAIL(errors-in-console): exception thrown -->
<script type="text/javascript" id="error-time">throw new Error('A distinctive error');</script>

<!-- PASS(errors-in-console): exception thrown but ignoredPatterns removes it -->
<script type="text/javascript">throw new Error('An ignored error');</script>

<!-- Add non-functional script to appear to be a Wordpress page for Stack Packs. -->
<script type="example" src="fake-script-wp-includes"></script>

Expand Down
4 changes: 4 additions & 0 deletions lighthouse-cli/test/smokehouse/dbw-config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,8 @@ module.exports = {
'apple-touch-icon', // pull in apple touch icon to test `LinkElements`
],
},
audits: [
// Test the `ignoredPatterns` audit option.
{path: 'errors-in-console', options: {ignoredPatterns: ['An ignored error']}},
],
};
20 changes: 10 additions & 10 deletions lighthouse-cli/test/smokehouse/dobetterweb/dbw-expectations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,16 +184,21 @@ const expectations = [
score: 0,
details: {
items: [
{
source: 'network',
description: 'Failed to load resource: the server responded with a status of 404 (Not Found)',
url: 'http://localhost:10200/dobetterweb/unknown404.css?delay=200',
},
{
source: 'other',
description: 'Application Cache Error event: Manifest fetch failed (404) http://localhost:10200/dobetterweb/clock.appcache',
url: 'http://localhost:10200/dobetterweb/dbw_tester.html',
},
{
source: 'Runtime.exception',
description: /^Error: A distinctive error\s+at http:\/\/localhost:10200\/dobetterweb\/dbw_tester.html:\d+:\d+$/,
url: 'http://localhost:10200/dobetterweb/dbw_tester.html',
},
{
source: 'network',
description: 'Failed to load resource: the server responded with a status of 404 (Not Found)',
url: 'http://localhost:10200/dobetterweb/unknown404.css?delay=200',
},
{
source: 'network',
description: 'Failed to load resource: the server responded with a status of 404 (Not Found)',
Expand All @@ -209,11 +214,6 @@ const expectations = [
description: 'Failed to load resource: the server responded with a status of 404 (Not Found)',
url: 'http://localhost:10200/dobetterweb/unknown404.css?delay=200',
},
{
source: 'Runtime.exception',
description: /^Error: A distinctive error\s+at http:\/\/localhost:10200\/dobetterweb\/dbw_tester.html:\d+:\d+$/,
url: 'http://localhost:10200/dobetterweb/dbw_tester.html',
},
],
},
},
Expand Down
39 changes: 37 additions & 2 deletions lighthouse-core/audits/errors-in-console.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ const UIStrings = {

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

/** @typedef {{ignoredPatterns?: Array<RegExp|string>}} AuditOptions */

class ErrorLogs extends Audit {
/**
* @return {LH.Audit.Meta}
Expand All @@ -42,11 +44,41 @@ class ErrorLogs extends Audit {
};
}

/** @return {AuditOptions} */
static defaultOptions() {
return {};
}


/**
* @template {{description: string | undefined}} T
* @param {Array<T>} items
Copy link
Contributor

@brendankenny brendankenny Oct 11, 2019

Choose a reason for hiding this comment

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

this is only ever called with /** @type {Array<{source: string, description: string|undefined, url: string|undefined}>} */, which is already declared once. Should just be a typedef instead of parametric? :)

Copy link
Collaborator Author

@patrickhulce patrickhulce Oct 11, 2019

Choose a reason for hiding this comment

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

why though? this method operates on things that have an optional description, no matter what they look like, and that's exactly how the types are expressed

Copy link
Contributor

@brendankenny brendankenny Oct 11, 2019

Choose a reason for hiding this comment

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

why though? this method operates on things that have an optional description, no matter what they look like, and that's exactly how the types are expressed

this is a nit, so can leave it, but it's only generic in the sense that all monomorphic functions are generic (over one type). Adding a parameter implies complexity that isn't there (it's only ever called with one type of thing).

Copy link
Collaborator Author

@patrickhulce patrickhulce Oct 11, 2019

Choose a reason for hiding this comment

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

true, and if typescript had another method of expressing monomorphic functions that operate on a subset of the type while preserving the output type I would happily switch to that method :)

Copy link
Collaborator

@connorjclark connorjclark Oct 11, 2019

Choose a reason for hiding this comment

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

:)

image

Copy link
Contributor

@brendankenny brendankenny Oct 11, 2019

Choose a reason for hiding this comment

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

method of expressing monomorphic functions that operate on a subset of the type while preserving the output type

I mean...

/**
 * @param {Array<ConcreteType>} items
 * @param {AuditOptions} options
 * @return {Array<ConcreteType>}
 */
static filterAccordingToOptions(items, options) {
  // ...
}

operates on a subset of the type just fine :P

(I blame everyone's emojis for the continuation of this conversation 👻📠)

* @param {AuditOptions} options
* @return {Array<T>}
*/
static filterAccordingToOptions(items, options) {
const {ignoredPatterns} = options;
if (!ignoredPatterns) return items;

return items.filter(({description}) => {
if (!description) return true;
for (const pattern of ignoredPatterns) {
if (pattern instanceof RegExp && pattern.test(description)) return false;
if (typeof pattern === 'string' && description.includes(pattern)) return false;
}

return true;
});
Copy link
Contributor

@brendankenny brendankenny Oct 11, 2019

Choose a reason for hiding this comment

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

what are we going to do with poorly formed ignoredPatterns? The error messages aren't going to be very comprehensible (e.g. not an Array) and the silent failure when there isn't an exception (e.g. misspelled or not a string or regex) isn't going to be noticeable

Copy link
Collaborator Author

@patrickhulce patrickhulce Oct 11, 2019

Choose a reason for hiding this comment

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

TypeError: ignoredPatterns is not iterable seems like a quite comprehensible error to fail with, IMO, especially given the sophistication necessary to set audit options at all.

I'm not sure how we could ever possibly know that a user has misspelled the error they want to ignore, so you've lost me on that one. users gotta learn to crawl on their own sometime 😆

Copy link
Contributor

@brendankenny brendankenny Oct 11, 2019

Choose a reason for hiding this comment

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

TypeError: ignoredPatterns is not iterable seems like a quite comprehensible error to fail with, IMO, especially given the sophistication necessary to set audit options at all.

yeah, that does read well

I'm not sure how we could ever possibly know that a user has misspelled the error they want to ignore, so you've lost me on that one. users gotta learn to crawl on their own sometime 😆

options: {ignoredPatternzzz: [/s.mple/i, 'really']} and nothing happens. How do I figure out why?

Copy link
Collaborator Author

@patrickhulce patrickhulce Oct 11, 2019

Choose a reason for hiding this comment

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

Oooooh misspell the option name, that makes much more sense. 👍

Honestly, I strongly dislike the pattern we've been starting to apply that unknown properties are fatal errors which breaks backwards/forwards compatibility. Would a warning of unrecognized property names address your concern?

Copy link
Contributor

@brendankenny brendankenny Oct 11, 2019

Choose a reason for hiding this comment

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

Honestly, I strongly dislike the pattern we've been starting to apply that unknown properties are fatal errors which breaks backwards/forwards compatibility. Would a warning of unrecognized property names address your concern?

yeah, sounds great 👍

}

/**
* @param {LH.Artifacts} artifacts
* @param {LH.Audit.Context} context
* @return {LH.Audit.Product}
*/
static audit(artifacts) {
static audit(artifacts, context) {
const auditOptions = /** @type {AuditOptions} */ (context.options);

const consoleEntries = artifacts.ConsoleMessages;
const runtimeExceptions = artifacts.RuntimeExceptions;
/** @type {Array<{source: string, description: string|undefined, url: string|undefined}>} */
Expand All @@ -73,7 +105,10 @@ class ErrorLogs extends Audit {
};
});

const tableRows = consoleRows.concat(runtimeExRows);
const tableRows = ErrorLogs.filterAccordingToOptions(
consoleRows.concat(runtimeExRows),
auditOptions
).sort((a, b) => (a.description || '').localeCompare(b.description || ''));

/** @type {LH.Audit.Details.Table['headings']} */
const headings = [
Expand Down
137 changes: 127 additions & 10 deletions lighthouse-core/test/audits/errors-in-console-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ describe('Console error logs audit', () => {
const auditResult = ErrorLogsAudit.audit({
ConsoleMessages: [],
RuntimeExceptions: [],
});
}, {options: {}});
assert.equal(auditResult.numericValue, 0);
assert.equal(auditResult.score, 1);
assert.ok(!auditResult.displayValue, 0);
Expand All @@ -34,7 +34,7 @@ describe('Console error logs audit', () => {
},
],
RuntimeExceptions: [],
});
}, {options: {}});
assert.equal(auditResult.numericValue, 0);
assert.equal(auditResult.score, 1);
assert.equal(auditResult.details.items.length, 0);
Expand Down Expand Up @@ -79,21 +79,21 @@ describe('Console error logs audit', () => {
'executionContextId': 3,
},
}],
});
}, {options: {}});

assert.equal(auditResult.numericValue, 3);
assert.equal(auditResult.score, 0);
assert.equal(auditResult.details.items.length, 3);
assert.equal(auditResult.details.items[0].url, 'http://www.example.com/favicon.ico');
assert.equal(auditResult.details.items[0].description,
'The server responded with a status of 404 (Not Found)');
assert.equal(auditResult.details.items[1].url, 'http://www.example.com/wsconnect.ws');
assert.equal(auditResult.details.items[1].description,
'WebSocket connection failed: Unexpected response code: 500');
assert.equal(auditResult.details.items[2].url,
assert.equal(auditResult.details.items[1].url,
'http://example.com/fancybox.js');
assert.equal(auditResult.details.items[2].description,
assert.equal(auditResult.details.items[1].description,
'TypeError: Cannot read property \'msie\' of undefined');
assert.equal(auditResult.details.items[2].url, 'http://www.example.com/wsconnect.ws');
assert.equal(auditResult.details.items[2].description,
'WebSocket connection failed: Unexpected response code: 500');
});

it('handle the case when some logs fields are undefined', () => {
Expand All @@ -106,7 +106,7 @@ describe('Console error logs audit', () => {
},
],
RuntimeExceptions: [],
});
}, {options: {}});
assert.equal(auditResult.numericValue, 1);
assert.equal(auditResult.score, 0);
assert.equal(auditResult.details.items.length, 1);
Expand Down Expand Up @@ -137,12 +137,129 @@ describe('Console error logs audit', () => {
'executionContextId': 3,
},
}],
});
}, {options: {}});
assert.equal(auditResult.numericValue, 1);
assert.equal(auditResult.score, 0);
assert.equal(auditResult.details.items.length, 1);
assert.strictEqual(auditResult.details.items[0].url, 'http://example.com/fancybox.js');
assert.strictEqual(auditResult.details.items[0].description,
'TypeError: Cannot read property \'msie\' of undefined');
});

describe('options', () => {
it('does nothing with an empty pattern', () => {
const options = {ignoredPatterns: ''};
const result = ErrorLogsAudit.audit({
ConsoleMessages: [
{
entry: {
level: 'error',
source: 'network',
text: 'This is a simple error msg',
},
},
],
RuntimeExceptions: [],
}, {options});

expect(result.score).toBe(0);
expect(result.details.items).toHaveLength(1);
});

it('does nothing with an empty description', () => {
const options = {ignoredPatterns: 'pattern'};
const result = ErrorLogsAudit.audit({
ConsoleMessages: [
{
entry: {
level: 'error',
},
},
],
RuntimeExceptions: [],
}, {options});

expect(result.score).toBe(0);
expect(result.details.items).toHaveLength(1);
});

it('does nothing with an empty description', () => {
const options = {ignoredPatterns: 'pattern'};
const result = ErrorLogsAudit.audit({
ConsoleMessages: [
{
entry: {
level: 'error',
},
},
],
RuntimeExceptions: [],
}, {options});

expect(result.score).toBe(0);
expect(result.details.items).toHaveLength(1);
});

it('filters console messages as a string', () => {
const options = {ignoredPatterns: ['simple']};
const result = ErrorLogsAudit.audit({
ConsoleMessages: [
{
entry: {
level: 'error',
source: 'network',
text: 'This is a simple error msg',
},
},
],
RuntimeExceptions: [],
}, {options});

expect(result.score).toBe(1);
expect(result.details.items).toHaveLength(0);
});

it('filters console messages as a regex', () => {
const options = {ignoredPatterns: [/simple.*msg/]};
const result = ErrorLogsAudit.audit({
ConsoleMessages: [
{
entry: {
level: 'error',
source: 'network',
text: 'This is a simple error msg',
},
},
],
RuntimeExceptions: [],
}, {options});

expect(result.score).toBe(1);
expect(result.details.items).toHaveLength(0);
});

it('filters exceptions with both regex and strings', () => {
const options = {ignoredPatterns: [/s.mple/i, 'really']};
const result = ErrorLogsAudit.audit({
ConsoleMessages: [],
RuntimeExceptions: [
{
exceptionDetails: {
url: 'http://example.com/url.js',
text: 'Simple Error: You messed up',
},
},
{
exceptionDetails: {
url: 'http://example.com/url.js',
text: 'Bad Error: You really messed up',
},
},
],
}, {options});

expect(result.score).toBe(1);
expect(result.details.items).toHaveLength(0);
});
});
});
10 changes: 5 additions & 5 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 @@ -236,6 +236,11 @@
"description": "Application Cache Error event: Manifest fetch failed (404) http://localhost:10200/dobetterweb/clock.appcache",
"url": "http://localhost:10200/dobetterweb/dbw_tester.html"
},
{
"source": "Runtime.exception",
"description": "Error: An error\n at http://localhost:10200/dobetterweb/dbw_tester.html:42:38",
"url": "http://localhost:10200/dobetterweb/dbw_tester.html"
},
{
"source": "network",
"description": "Failed to load resource: the server responded with a status of 404 (Not Found)",
Expand All @@ -250,11 +255,6 @@
"source": "network",
"description": "Failed to load resource: the server responded with a status of 404 (Not Found)",
"url": "http://localhost:10200/dobetterweb/unknown404.css?delay=200"
},
{
"source": "Runtime.exception",
"description": "Error: An error\n at http://localhost:10200/dobetterweb/dbw_tester.html:42:38",
"url": "http://localhost:10200/dobetterweb/dbw_tester.html"
}
]
}
Expand Down
10 changes: 5 additions & 5 deletions proto/sample_v2_round_trip.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,11 @@
"source": "other",
"url": "http://localhost:10200/dobetterweb/dbw_tester.html"
},
{
"description": "Error: An error\n at http://localhost:10200/dobetterweb/dbw_tester.html:42:38",
"source": "Runtime.exception",
"url": "http://localhost:10200/dobetterweb/dbw_tester.html"
},
{
"description": "Failed to load resource: the server responded with a status of 404 (Not Found)",
"source": "network",
Expand All @@ -614,11 +619,6 @@
"description": "Failed to load resource: the server responded with a status of 404 (Not Found)",
"source": "network",
"url": "http://localhost:10200/dobetterweb/unknown404.css?delay=200"
},
{
"description": "Error: An error\n at http://localhost:10200/dobetterweb/dbw_tester.html:42:38",
"source": "Runtime.exception",
"url": "http://localhost:10200/dobetterweb/dbw_tester.html"
}
],
"type": "table"
Expand Down