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.

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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

OronW
Copy link

@OronW OronW commented May 27, 2023

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.

* throw error if an onlyX filter is an empty array

* added tests to onlyX filters throwing error on empty array
@OronW OronW requested a review from a team as a code owner May 27, 2023 10:11
@OronW OronW requested review from adamraine and removed request for a team May 27, 2023 10:11
Copy link
Member

@adamraine adamraine left a comment

Choose a reason for hiding this comment

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

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

@brendankenny
Copy link
Member

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
Copy link
Collaborator

connorjclark commented Jun 12, 2023

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.

image

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

@connorjclark
Copy link
Collaborator

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
Copy link
Member

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
Copy link
Author

OronW commented Jun 20, 2023

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants