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

filter unneeded aggregations from config before extension runs #973

Merged
merged 1 commit into from
Nov 19, 2016

Conversation

brendankenny
Copy link
Member

fixes #921 (but for real this time)

The issue was that we were correctly filtering out audits from running based on what aggregation categories were selected in the extension, but we weren't filtering out the aggregations themselves. When report time comes along, the unselected aggregations were looking for results in audits that had been filtered out, causing the error.

@pavelfeldman: This needs to happen for both the extension and worker (DevTools) versions, though, which means it needs to happen in lighthouse-background.js. This means a (hopefully minor) breaking change for DevTools, as it will now need to pass in a set of aggregations to run, not a set of audits.

@brendankenny
Copy link
Member Author

the good news is that this means that when you filter out some audits, you no longer have that section of the report sticking around with a bunch of null scores :) Since the aggregation category itself will be filtered out, it just will never appear in the report

@patrickhulce
Copy link
Collaborator

don't have a ton of context here but is there a reason we didn't just prune after they were run and only show the audits we do have grouped by their respective aggregations? or is there an inherent assumption that you have to run all or none of the audits in a category?

@brendankenny
Copy link
Member Author

brendankenny commented Nov 18, 2016

This is extension (and devtools, for now) only, just to be clear.

So for aggregation categories define what audits to run: the idea was that you were probably not going to ever want to select and deselect individual audits in the extension UI (and if you did, time to move to the CLI and a config file), so we used the aggregation categories as a convenient way to group them and turn them off and on.

The other side of things (and this is for extension and CLI) is that it was previously really easy to get an audit name wrong and, when an audit wasn't found by that name, it was simply omitted from the report. As the report got bigger, it was easier and easier to miss that an audit didn't run. #914 made that an error, the thinking being that if you indicate that you want to score something, we should enforce that it's there to score. There is an argument for only scoring the data that you have, but the fact that we accidentally weren't scoring two audits we'd had for some time because we got the names wrong (see that PR for details) was convincing enough that we made it an error to try to score audits that never ran.

The two combined mean that if we want to filter the audits based on category, we also need to filter the aggregation categories.

@brendankenny brendankenny merged commit 59cef3b into master Nov 19, 2016
@brendankenny brendankenny deleted the filteraggregations branch November 19, 2016 00:24
andrewrota pushed a commit to andrewrota/lighthouse that referenced this pull request Jan 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unchecked audits in Extension options leads to error
3 participants