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: add group support for plugins #7304

Merged
merged 4 commits into from
Mar 1, 2019

Conversation

jburger424
Copy link
Contributor

@jburger424 jburger424 commented Feb 22, 2019

Summary
Allow for plugin to add custom groups and assign custom to plugin auditRefs.
Modify tests to verify groups added and assigned properly.

Related Issues/PRs
#6959
#6070

Update tests to ensure groups are added properly.
@jburger424 jburger424 changed the title Add group support for plugins. core: add group support for plugins. Feb 22, 2019
@jburger424 jburger424 changed the title core: add group support for plugins. core: add group support for plugins Feb 22, 2019
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @jburger424!! mind posting a screenshot with the motivating example? :)

Apologies if you may have already communicated this internally, and I'm just out of the loop.

lighthouse-core/config/config-plugin.js Outdated Show resolved Hide resolved
lighthouse-core/config/config-plugin.js Outdated Show resolved Hide resolved
lighthouse-core/config/config-plugin.js Outdated Show resolved Hide resolved
lighthouse-core/config/config-plugin.js Outdated Show resolved Hide resolved
lighthouse-core/config/config-plugin.js Outdated Show resolved Hide resolved
lighthouse-core/config/config-plugin.js Outdated Show resolved Hide resolved
lighthouse-core/config/config-plugin.js Show resolved Hide resolved
@jburger424
Copy link
Contributor Author

Thanks for the contribution @jburger424!! mind posting a screenshot with the motivating example? :)

Apologies if you may have already communicated this internally, and I'm just out of the loop.

The motivating example for this is our plugin Ad Speed Insights.
ad-speed-insights-screenshot

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM % few nits

I'll defer to @brendankenny, the plugin master, though for approval.

lighthouse-core/config/config-plugin.js Show resolved Hide resolved
lighthouse-core/config/config-plugin.js Outdated Show resolved Hide resolved
lighthouse-core/config/config-plugin.js Outdated Show resolved Hide resolved
Minor changes in response to PR comments.
@patrickhulce patrickhulce dismissed their stale review February 27, 2019 19:59

Approved, but not official

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

thanks for doing this and for all those tests! 😍

just a few changes, mostly nits

lighthouse-core/config/config-plugin.js Show resolved Hide resolved
lighthouse-core/config/config-plugin.js Outdated Show resolved Hide resolved
lighthouse-core/config/config-plugin.js Outdated Show resolved Hide resolved
lighthouse-core/config/config-plugin.js Outdated Show resolved Hide resolved
lighthouse-core/config/config-plugin.js Outdated Show resolved Hide resolved
lighthouse-core/config/config-plugin.js Outdated Show resolved Hide resolved
lighthouse-core/test/config/config-test.js Outdated Show resolved Hide resolved
@@ -84,6 +93,7 @@ describe('ConfigPlugin', () => {
categories: {
'lighthouse-plugin-evil': evilCategory,
},
groups: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

do these two additions add anything to the test? My brain may be just foggy today and I'm missing it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests were failing without this because the undefined group object is not yet stripped at this point.

lighthouse-core/test/config/config-plugin-test.js Outdated Show resolved Hide resolved
audits: [
{path: 'redirects'},
{path: 'user-timings'},
],
category: {
title: 'Simple',
auditRefs: [],
auditRefs: [
{id: 'redirects', weight: 1, group: 'new-group'},
Copy link
Member

Choose a reason for hiding this comment

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

oh, ha, there weren't any audit refs to add a group to before. Sorry I missed that :)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks again for taking this on

@brendankenny brendankenny merged commit bfb1249 into GoogleChrome:master Mar 1, 2019
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.

3 participants