Skip to content

core(errors-in-console): ignore BLOCKED_BY_CLIENT.Inspector errors #11901

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
devtools-bot merged 4 commits into from
Jan 12, 2021
Merged

core(errors-in-console): ignore BLOCKED_BY_CLIENT.Inspector errors #11901

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ee79c62
core(errors-in-console): ignore BLOCKED_BY_CLIENT.Inspector errors
paulirish Dec 29, 2020
f8e4294
Update lighthouse-core/test/audits/errors-in-console-test.js
paulirish Dec 29, 2020
2ad2cd2
move test to a new defaultOptions group
paulirish Jan 12, 2021
c9662ce
add small comment
paulirish Jan 12, 2021
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: 1 addition & 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 @@ -45,10 +45,9 @@ class ErrorLogs extends Audit {

/** @return {AuditOptions} */
static defaultOptions() {
return {};
return {ignoredPatterns: ['ERR_BLOCKED_BY_CLIENT.Inspector']};

@patrickhulce patrickhulce Jan 5, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

My initial reaction was .Inspector might be too narrow since I didn't remember it always having that suffix, but I couldn't find reports of it without it, so I might be making that up. Calling it out in case you had the same initial thought and were on the fence for another nudge :)

@paulirish paulirish Jan 12, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There's a bunch of other blockedReasons that are fairly legit, so i think preserving those results seems good. https://chromedevtools.github.io/devtools-protocol/tot/Network/#type-BlockedReason

@patrickhulce patrickhulce Jan 12, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Ya for sure, but those would be caught by {ignoredPatterns: ['ERR_BLOCKED_BY_CLIENT']}? I was questioning if the .Inspector part needs to be here.

}


/**
* @template {{description: string | undefined}} T
* @param {Array<T>} items
Expand Down
16 changes: 16 additions & 0 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 @@ -124,6 +124,22 @@ describe('ConsoleMessages error logs audit', () => {
'TypeError: Cannot read property \'msie\' of undefined');
});

// Checks bug #10198
Comment thread
paulirish marked this conversation as resolved.
Outdated
it('filters out blocked_by_client.inspector messages', () => {
const auditResult = ErrorLogsAudit.audit({
ConsoleMessages: [{
'source': 'exception',
'level': 'error',
'timestamp': 1506535813608.003,
'url': 'https://www.facebook.com/tr/',
'text': 'Failed to load resource: net::ERR_BLOCKED_BY_CLIENT.Inspector',

@connorjclark connorjclark Dec 29, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

IMO this test case is unnecessary, given the next few tests are exercising the filter feature. At minimum, i'd like to see it moved to after the filter tests (As it builds on them)

@patrickhulce patrickhulce Jan 5, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would like to see the test remain as it describes a new, important part of this audit's default state of being. No real preference for position on my end :)

@connorjclark connorjclark Jan 5, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it's not new, it's a parametrization of what multiple previous test cases has already proved to work (that the ignoredPatterns options works as intended). it might as well be assert.equal(ErrorLogsAudit.defaultOptions().ignoredPatterns, ['whatever']); (my preferred change, actually). Just an idea- I like the process of tests building on top of each other, sort of like a mathematical proof.

At the least, please move the test to after the ones that assert filtering works.

@patrickhulce patrickhulce Jan 5, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

it might as well be assert.equal(ErrorLogsAudit.defaultOptions().ignoredPatterns, ['whatever']); (my preferred change, actually)

Sorry, I didn't realize that's what you were suggesting. It sounded to me like you wanted the assertion of this being the default removed, which I didn't :)

@connorjclark connorjclark Jan 5, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It wasn't originally, your feedback evolved my stance :)

@brendankenny brendankenny Jan 5, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not sure why we're going to town on this fairly innocuous test, but I'll join in too :)

The test seems good as is to me. We don't care about how exactly the default pattern comes in, just that ERR_BLOCKED_BY_CLIENT errors are filtered out, so that's what should be tested.

If e.g. we decide we want to merge ERR_BLOCKED_BY_CLIENT.Inspector into whatever options are passed in instead of having the passed-in options overwrite the default (the normal auditOptions behavior) -- so ERR_BLOCKED_BY_CLIENT.Inspector couldn't go into defaultOptions -- there's no reason for this test to have to be rewritten.

Agreed that it makes sense to move into the 'options' test block, and (supernit) maybe it('filters out blocked_by_client.inspector messages by default', ...?

@paulirish paulirish Jan 12, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

test left impl left as is. moved it to a new defaultOptions group right after options:

image

}],
}, {options: ErrorLogsAudit.defaultOptions()});
assert.equal(auditResult.score, 1);
assert.equal(auditResult.details.items.length, 0);
});


describe('options', () => {
it('does nothing with an empty pattern', () => {
const options = {ignoredPatterns: ''};
Expand Down