Adds support for custom audits and gatherers

[paullewis]

This PR allows us to specify additional paths for audits and gatherers in the config, which get searched when we do the requires.

The properties in the config are thus:

{
  "paths": {
    "audits": [
      "/path/to/my/audits"
    ],

    "gatherers": [
      "/path/to/my/gatherers"
    ]
  }
}

A custom gatherer looks like this:

/**
 *
 * Copyright 2016 Google Inc. All rights reserved.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

'use strict';

const LighthouseGatherer = require('lighthouse/custom/gatherer');

class MediaQueries extends LighthouseGatherer {
  pass(options) {
    const driver = options.driver;
    return driver.sendCommand('CSS.getMediaQueries')
        .then(result => {
          this.artifact = result
        })
  }
}

module.exports = MediaQueries;

and a custom audit like this:

/**
 *
 * Copyright 2016 Google Inc. All rights reserved.
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

'use strict';

const LighthouseAudit = require('lighthouse/custom/audit');

class MediaQueries extends LighthouseAudit {
  static get meta () {
    return {
      category: 'Custom',
      name: 'media-queries',
      description: 'Has media queries',
      requiredArtifacts: ['MediaQueries']
    };
  }

  static audit(artifacts) {
    return MediaQueries.generateAuditResult({
      rawValue: artifacts.MediaQueries.length > 0
    });
  }
}

module.exports = MediaQueries;

Then if you fancy it you can just adjust the reporting to suit.

[paulirish]

I think there's a path.resolve() or something to sort out absolute vs not. Would rather that then us handling the branch.

[paulirish]

Yup: The last 3 here are exactly what we need:

[brendankenny]

This will be great to have (and love the audit validation :). Two more thoughts:

• Some discussion about this earlier: the custom audits and gatherers may need to be specified relative to the config file, as this is really the only fixed point we know (who knows how the user has installed and is running Lighthouse and so where config/index.js is relative to where they think the root of their project is).
• I wonder if this should use inline paths instead of an explicit paths property on the config. If there are many gatherers or audits in the same directory, it will cause more noise, but if there are many different directories (like a directory per audit e.g. from npm modules), the paths thing gets a bit unwieldy. For maintenance purposes it also may just be better to have path and module name all in one place.

The config would then look something like:

{
  "audits": [
    "is-on-https",
    "redirects-http",
    "service-worker",
    "path/to/my/audits/audit1/cool-audit",
    "audit-security-mixedcontent/audits/better-audit"
  ]
}

(similar for gatherers)

This would also support a more straightforward include process, basically

• get Runner.getAuditList()
• if the audit is in there, require it from ./audits/{$auditName}
• otherwise adjust the path to be relative to config/index.js
• require() the new string

This lets require get us all its normal functionality like starting with ./ or ../ for relative paths, traversing folders to the root looking for the target in any node_modules/, supporting $NODE_PATH, etc. The flexible node_modules/ support I think will be particularly helpful and avoid having to write support for many Lighthouse use corner cases ourselves.

It also sometimes eliminates the need for paths in the config file. If they are npm modules or are put in a directory require() searches, you can do the normal thing and drop the path, so it goes back to

{
  "audits": [
    "is-on-https",
    "redirects-http",
    "service-worker",
    "cool-audit",
    "better-audit"
  ]
}

[paullewis]

Okay, hopefully I've struck the right balance here:

• Changed it so that we assume requires in this order: 1) core (checked through Runner.getAuditList(), 2) require directly (via require.resolve), 3) require relatively via the config path.
• Added checks for invalid gatherers as well as invalid audits.
• Removed the custom folder from the root; exported the Audit and Gatherer classes via the Lighthouse module, i.e. require('lighthouse').Audit

PTAL

[paulirish]

This set of checks duplicates what's already in runner. : https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/runner.js#L29-L44

I think we should only check where neccessary and not end up with all the duplication. If we can't keep straight if we need a safety check, then we probably need to untangle a bit. (I'm thinking of our refactor for traceEvents a bit ago. There's a few more in the wings.)

[paullewis]

I agree. This looks like an artifact of my refactoring perhaps, rather than something that should be there.

[paulirish]

lgtm

[paulirish]

I do think we should get @ebidel @hsablonniere or @blaine to bang around with this API to make sure this works well.

[paullewis]

Agreed. I'm very open to this changing with developer feedback. My feeling is we should merge, and then revisit it once folks can have a play with it and let us know how well it works for them.

[hsablonniere]

I won't be able to play with it over the weekend. Maybe next week. This definitely seems interesting.

[paulirish]

K merging so it's available.

@paullewis let's get some notes in the readme on how to use it.

[ebidel]

Been waiting for this to merge so I can give feedback. Too early to tell
right now :)

On Fri, Aug 19, 2016, 6:55 PM Paul Irish [email protected] wrote:

Merged #593 #593.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#593 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAOigHUGahUYEhVeAqBb0X6QnFe7coFhks5qhl6dgaJpZM4JlloK
.