feat: support category and audit whitelisting #1988

patrickhulce · 2017-04-10T22:13:32Z

addresses burndown items for I/O #1937

usage to run just TTI, PSI, and the PWA audits

{
  settings: {
     onlyAudits: ["time-to-interactive", "speed-index-metric"],
     onlyCategories: ["pwa"]
  }
}

brendankenny

this looks like it's going to be great.

General things:

I've seen the : format used more for referencing special values (e.g. 'lighthouse:default') rather than just giving the type of thing. Does it make more sense to have onlyCategories and onlyAudits properties instead of the less explicit only prop?
We need to be clear about the hierarchy here (but where?). It's a little confusing that it's one or the other, category or audit, but not both. e.g. if you say only: ["audit:service-worker", "category:pwa"], the category wins and you run all of the pwa category, not just the service-worker audit within it
definitely want warnings to the user about things like trying to do only an audit when you're also doing only the category that includes it (see above) and unknown audit/category names to catch misspellings and the like.
tests! :)

brendankenny · 2017-04-10T23:20:53Z

lighthouse-core/config/config.js

@@ -344,14 +356,21 @@ class Config {
   * Filter out any unrequested categories from the categories object.
   * @param {!Object<string, {audits: !Array<{id: string}>}>} categories
   * @param {Array<string>=} categoryIds
+   * @param {Array<string>=} auditIds


looks like we missed this before and these should be {!Array<string>=}

brendankenny · 2017-04-10T23:23:51Z

lighthouse-core/config/config.js

@@ -344,14 +356,21 @@ class Config {
   * Filter out any unrequested categories from the categories object.


need to update

brendankenny · 2017-04-10T23:28:02Z

lighthouse-core/config/perf.json

-      "dobetterweb/tags-blocking-first-paint",
-      "dobetterweb/optimized-images"
-    ]
+  "extends": "lighthouse:default",


this is a lie depending on the current check just being truthiness of extends, right?

patrickhulce · 2017-04-11T00:44:46Z

Does it make more sense to have onlyCategories and onlyAudits properties instead of the less explicit only prop?

I'm fine with this too, but with the goal of having all config settings map to CLI flags that seemed slightly polluting. WFM if others agree to split though.

We need to be clear about the hierarchy here (but where?)

Agreed, this keeps it simple by being 100% additive so if it's listed in only it's guaranteed to run, added warnings too.

paulirish · 2017-04-11T00:51:23Z

Does it make more sense to have onlyCategories and onlyAudits properties instead of the less explicit only prop?

I'm fine with both and consider them equivalent from an ergonomics POV.

patrickhulce · 2017-04-12T20:08:44Z

this waiting on anything else? PTAL :)

brendankenny

almost :)

brendankenny · 2017-04-12T23:05:17Z

lighthouse-core/config/config.js

    const categories = {};

+    // warn if the category is not found
+    categoryIds.forEach(category => {


category -> categoryId

brendankenny · 2017-04-12T23:05:36Z

lighthouse-core/config/config.js

+    });
+
+    // warn if the audit is not found in a category
+    auditIds.forEach(audit => {


audit -> auditId

brendankenny · 2017-04-12T23:19:47Z

lighthouse-core/config/config.js

+    // warn if the category is not found
+    categoryIds.forEach(category => {
+      if (!oldCategories[category]) {
+        log.warn('config', `unrecognized category: ${category}`);


need a clearer way of saying this, since users will see it without context. Not sure what would be good though... maybe just unrecognized category in `onlyCategories`: ${category}?

brendankenny · 2017-04-12T23:19:56Z

lighthouse-core/config/config.js

+      });
+
+      if (!foundCategory) {
+        log.warn('config', `unrecognized audit: ${audit}`);


same for this

brendankenny · 2017-04-12T23:20:50Z

lighthouse-core/config/config.js

+      }
+
+      if (categoryIds.includes(foundCategory)) {
+        log.warn('config', `${foundCategory} category already includes "${audit}"`);


I'm not sure how to word this one either. ${audit} in `onlyAudits` already included by `onlyCategories` entry ${foundCategory}? Or something less terrible :)

brendankenny · 2017-04-12T23:28:03Z

lighthouse-core/test/config/config-test.js

+    });
+
+    assert.ok(config.audits.length, 'inherited audits by extension');
+    assert.ok(config.audits.length < origConfig.audits.length, 'filtered out audits');


could you do origConfig.categories.performance.audits.length + 1? Seems future compatible

brendankenny · 2017-04-12T23:31:31Z

lighthouse-core/test/config/config-test.js

+      const auditCount = Object.keys(selectedCategory.audits).length;
+      assert.equal(config.passes.length, 3, 'incorrect # of passes');
+      assert.equal(config.audits.length, auditCount, 'audit filtering failed');
+    });


tests for the new warnings would be good, too. log.events.addListener('warning', callback) can be an easy way to do that rather than going to the trouble of overriding log, like is done elsewhere in that file

brendankenny · 2017-04-12T23:33:57Z

lighthouse-extension/app/src/lighthouse-background.js

@@ -99,7 +99,7 @@ function filterOutArtifacts(result) {
 * @return {!Promise}
 */
 window.runLighthouseForConnection = function(connection, url, options, categoryIDs) {
-  const newConfig = Config.generateNewConfigOfCategories(defaultConfig, categoryIDs);
+  const newConfig = Config.generateNewFilteredConfig(defaultConfig, categoryIDs);


should we switch this to just

const newConfig = { "extends": "lighthouse:default", "settings": { "onlyCategories": categoryIDs } };

brendankenny

sweet, LGTM!

feat: support category and audit whitelisting

d5caec3

brendankenny requested changes Apr 10, 2017

View reviewed changes

warn more

7b53816

split into 2 fields, add tests

39f439b

brendankenny requested changes Apr 12, 2017

View reviewed changes

more feedback

48d85ee

brendankenny approved these changes Apr 13, 2017

View reviewed changes

brendankenny merged commit 16b0b04 into master Apr 13, 2017

brendankenny deleted the cat_audit_whitelisting branch April 13, 2017 19:44

paulirish mentioned this pull request Apr 13, 2017

[meta] I/O Burndown #1937

Closed

52 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support category and audit whitelisting #1988

feat: support category and audit whitelisting #1988

patrickhulce commented Apr 10, 2017 •

edited

Loading

brendankenny left a comment

brendankenny Apr 10, 2017

patrickhulce Apr 11, 2017

brendankenny Apr 10, 2017

patrickhulce Apr 11, 2017

brendankenny Apr 10, 2017

patrickhulce Apr 10, 2017

patrickhulce commented Apr 11, 2017

paulirish commented Apr 11, 2017

patrickhulce commented Apr 12, 2017

brendankenny left a comment

brendankenny Apr 12, 2017

patrickhulce Apr 13, 2017

brendankenny Apr 12, 2017

patrickhulce Apr 13, 2017

brendankenny Apr 12, 2017

patrickhulce Apr 13, 2017

brendankenny Apr 12, 2017

patrickhulce Apr 13, 2017

brendankenny Apr 12, 2017

patrickhulce Apr 13, 2017

brendankenny Apr 12, 2017

patrickhulce Apr 13, 2017

brendankenny Apr 12, 2017

patrickhulce Apr 13, 2017

brendankenny Apr 12, 2017

patrickhulce Apr 13, 2017

brendankenny left a comment

		@@ -344,14 +356,21 @@ class Config {
		* Filter out any unrequested categories from the categories object.

feat: support category and audit whitelisting #1988

feat: support category and audit whitelisting #1988

Conversation

patrickhulce commented Apr 10, 2017 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Apr 11, 2017

paulirish commented Apr 11, 2017

patrickhulce commented Apr 12, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

patrickhulce commented Apr 10, 2017 •

edited

Loading