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

[paulirish]

the .Inspector Network.BlockedReason indicates it's devtools instrumentation itself that's blocking. And we see cases where this crops up and is just noise.

fixes #10198

[connorjclark]

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]

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]

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]

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]

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

[brendankenny]

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]

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

[patrickhulce]

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]

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]

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.

[paulirish]

.