core(config): throw an error if a filter is an empty array

[OronW]

Summary
This is a bugfix, which fixes this issue:
#14986
Logic fix and relevant unit tests were added.

Description
TLDR: Throw an Error In case an empty array of onlyCategories / onlyAudits is passed

In the case an empty array of onlyCategories is passed, all categories are filtered (as only 0 categories are to be kept).
This results in an unexpected behavior, as an empty array might be interpreted as "don't filter anything".
Since passing an empty array serves no use, an error is thrown.

TBD
The original issue referred to throwing an error also for the case in which audits is left empty after filtering, however empty/missing audits is being handled in following code, and throwing an error breaks this logic.
Throwing an error for empty audits also breaks 30 tests.

I would suggest not changing any audits logic.

[adamraine]

Looks good! CI is currently broken for unrelated reasons so we can merge once that's fixed.

[brendankenny]

Sorry to reopen the bikeshed for #14986:

I'm kind of hung up on the fact that you're allowed to have a config with categories: {} but after this change you won't be allowed to have onlyCategories: []. It's the empty set of category ids; seems perfectly valid to request a default LHR with all the categories stripped out, particularly if you're a programmatic user and are only interested in an audit's output. No need to do extra ceremony to add a category you don't need and aren't going to look at just to run your audit.

(I feel the same way about onlyAudits and half think we should be consistent on these, but I can't think of any legitimate reason you'd ever need to run onlyAudits: [] if you weren't a Lighthouse test)

Could be a warning instead?

[connorjclark]

It's the empty set of category ids; seems perfectly valid to request a default LHR with all the categories stripped out, particularly if you're a programmatic user and are only interested in an audit's output.

I had no idea this was a thing.

So the empty category filter keeps all the audits, but give it any valid category and .audits will start to be pruned...

[connorjclark]

No need to do extra ceremony to add a category you don't need and aren't going to look at just to run your audit.

Isn't this use case covered by supplying onlyAudits to just the audit(s) you want? Drops all the stuff you don't care about in audits.

I don't think trimming off the minimal data in .categories (like in the above image) is worth the complication in filtering here. If that small amount of data is troubling a programatic user, they can easily delete it, as they probably are already doing other manual tasks to reduce the size of the payload anyway.

[brendankenny]

I don't think trimming off the minimal data in .categories (like in the above image) is worth the complication in filtering here

It's not adding complication, it works by default :) Adding new thrown errors is the extra complication, which is pretty much my objection as well.

#14986 says that the mental model of filtering the categories to only those listed in onlyCategories isn't always clear, but breaking it so there is no consistent mental model is arguably worse. Hence the question about switching to a warning.

The problem wasn't fatal, it was that lhr.categories was {}, which was unexpected to the user. A logged warning would have explained what happened.

[OronW]

@connorjclark @adamraine @brendankenny
Hey guys,
I read the comments and see that this was still not merged.
Is there anything you'd want me to update/change so you'd feel comfortable moving forward?

Please let me know what's decided, so this PR will not hang and I could work on it if needed.
Thanks