From b1b4d49ef4c2c1b3bd5eaa9da14e6a73f946b048 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Fri, 1 May 2020 14:10:51 -0400 Subject: [PATCH 1/2] docs(contributing): add tips for audit and gatherer PRs --- CONTRIBUTING.md | 121 ++++++++++++++++++++++++++++--------------- docs/hacking-tips.md | 11 ---- proto/README.md | 27 +++++++--- readme.md | 25 --------- 4 files changed, 99 insertions(+), 85 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index aac5e4156b32..356bb23cb876 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -8,59 +8,40 @@ We tag issues that are good candidates for those new to the code with [`good fir ## Follow the coding style -The `.eslintrc` file defines all. We use [JSDoc](http://usejsdoc.org/) with [TypeScript](https://github.com/Microsoft/TypeScript/wiki/JSDoc-support-in-JavaScript). Annotations are encouraged for all contributions. - -## Pull request titles - -We're using [conventional-commit](https://conventionalcommits.org/) for our commit messages. Since all PRs are squashed, we enforce this format for PR titles rather than individual git commits. A [`commitlint` bot](https://github.com/paulirish/commitlintbot) will update the status of your PR based on the title's conformance. The expected format is: - -> type(scope): message subject - -* The `type` must be one of: `new_audit` `core` `tests` `docs` `deps` `report` `cli` `extension` `misc`. (See [`.cz-config`](https://github.com/GoogleChrome/lighthouse/blob/master/.cz-config.js#L13)) -* The `scope` is optional, but recommended. Any string is allowed; it should indicate what the change affects. -* The `message subject` should be pithy and direct. - -The [commitizen CLI](https://github.com/commitizen/cz-cli) can help to construct these commit messages. +The `.eslintrc.js` file defines all. We use [JSDoc](http://usejsdoc.org/) with [TypeScript `checkJs`](https://www.typescriptlang.org/docs/handbook/type-checking-javascript-files.html#supported-jsdoc). Annotations are encouraged for all contributions. ## Learn about the architecture See [Lighthouse Architecture](./docs/architecture.md), our overview and tour of the codebase. -## Sign the Contributor License Agreement - -We'd love to accept your sample apps and patches! Before we can take them, we have to jump a couple of legal hurdles. - -Please fill out either the individual or corporate Contributor License Agreement (CLA). - -* If you are an individual writing original source code and you're sure you own the intellectual property, then you'll need to sign an [individual CLA](https://developers.google.com/open-source/cla/individual). -* If you work for a company that wants to allow you to contribute your work, then you'll need to sign a [corporate CLA](https://developers.google.com/open-source/cla/corporate). - -Follow either of the two links above to access the appropriate CLA and instructions for how to sign and return it. Once we receive it, we'll be able to -accept your pull requests. - ## Contributing a patch If you have a contribution for our [documentation](https://web.dev/learn/#lighthouse), please submit it in the [web.dev repo](https://github.com/GoogleChrome/web.dev). -1. Submit an issue describing your proposed change to the repo in question. -1. The repo owner will respond to your issue promptly. -1. If your proposed change is accepted, and you haven't already done so, sign a Contributor License Agreement (see details above). +1. Submit an issue describing your proposed change. +1. The maintainers will respond to your issue promptly. +1. If your proposed change is accepted, and you haven't already done so, sign a Contributor License Agreement (see details below). 1. Fork the repo, develop and test your code changes. 1. Ensure that your code adheres to the existing style in the sample to which you are contributing. 1. Submit a pull request. If you've submitted a number of significant patches, feel free to add yourself in a PR to the project's `AUTHORS` [file](https://github.com/GoogleChrome/lighthouse/blob/master/AUTHORS) in the root of the repo to be recognized for your contributions! -## Adding Images to a Readme +## Audit PRs -If you are adding an image to a readme use the absolute path to the image for the specific commit hash where the image was introduced. This requires multiple commits. -1. Make the commit to introduce the image. -1. Get the [absolute path](https://help.github.com/articles/getting-permanent-links-to-files/) to the image with the commit hash e.g. `https://raw.githubusercontent.com/GoogleChrome/lighthouse/e7997b3db01de3553d8cb208a40f3d4fd350195c/assets/example_dev_tools.png` -1. Add to readme as an absolute reference to that image. +If proposing a new audit for Lighthouse, see the [new audit proposal guide](./docs/new-audits.md) and open an issue for discussion before starting. -If you are updating an image that already exists: commit it, then update the readme to point the image with that new commits hash absolute url. +A PR for a new audit or changing an existing audit almost always needs the following: + +1. If new, add the audit to the [default config file](lighthouse-core/config/default-config.js) (or, rarely, one of the other config files) so Lighthouse will run it. -## Description Guidelines +1. **Unit tests**: in the matching test file (e.g. tests for `lighthouse-core/audits/my-swell-audit.js` go in `lighthouse-core/test/audits/my-swell-audit-test.js`). + +1. **Smoke (end-to-end) tests**: search through the [existing expectations](lighthouse-cli/test/smokehouse/test-definitions/) to see if there's a logical place to add a check for your change, or (as a last resort) add a new smoke test. + +1. Run `yarn update:sample-json` to update the [sample Lighthouse result JSON](lighthouse-core/test/results/sample_v2.json) kept in the repo for testing. This will also pull any strings needed for localization into the correct files. + +### Audit `description` Guidelines Keep the `description` of an audit as short as possible. When a reference doc for the audit exists on developers.google.com/web, the `description` should only explain *why* the user should care @@ -79,15 +60,71 @@ Don't: If no reference doc exists yet, then you can use the `description` as a stopgap for explaining both why the audit is important and how to fix it. -## Updating sample artifacts and LHR JSON +## Gatherer PRs -``` -yarn run update:sample-artifacts # update all artifacts -yarn run update:sample-artifacts ScriptElements # update just one artifact -yarn run update:sample-json # update sample LHR based on sample artifacts -``` +Gatherers have to interface with the inherently-complex real world and they also run *while* loading and/or testing the page, risking interfering with themselves. As a result, gatherers should strive to be as simple as possible and leave as much processing as possible to computed artifacts and audits. + +It can be tempting to serialize the entire state of the world into the artifact the gatherer produces. Try to limit the artifact to exactly the information needed for current audits while leaving room for extensibility as needs change in the future. + +A PR adding or changing a gatherer almost always needs to include the following: + +1. If new, add the gatherer to the [default config file](lighthouse-core/config/default-config.js) (or, rarely, one of the other config files) so Lighthouse will run it. + +1. **Unit tests**: gatherer execution often takes place mostly on the browser side, either through protocol functionality or executing javascript in the test page. This makes gatherers difficult to unit test without extensive mocking, ending up mostly exercising the mocks instead of the actual gatherer. + + As a result, we mostly rely on smoke testing for gatherers. However, if there are parts of a gatherer that naturally lend themselves to unit testing, the new tests would go in the matching test file (e.g. tests for `lighthouse-core/gather/gatherers/reap.js` go in `lighthouse-core/test/gather/gatherers/reap-test.js`). -When updating artifacts, usually you'll need to revert changes to the `*.devtoolslog.json` and `*.trace.json` files and manually review changes to `artifacts.json` to make sure they are related to your work. +1. **Smoke (end-to-end) tests**: search through the [existing expectations](lighthouse-cli/test/smokehouse/test-definitions/) to see if there's a logical place to add a check for your change, or (as a last resort) add a new smoke test if one is required. + + It's most important to get true end-to-end coverage, so be sure that audits that consume the new gatherer output are in the expectations. Artifacts can also have expectations for those intermediate results. + +1. **Golden artifacts**: `sample_v2.json` is generated from a set of artifacts that come from running LH against `dbw_tester.html`. Those artifacts likely need to be updated after gatherer changes with `yarn update:sample-artifacts`, but limit to just the artifact being altered if possible. For example: + + ```sh + # update just the ScriptElements artifact + yarn update:sample-artifacts ScriptElements + ``` + + This command works for updating `yarn update:sample-artifacts devtoolsLogs` or `traces` as well, but the resulting `sample_v2.json` churn may be extensive and you might be better off editing manually. + +1. Run `yarn update:sample-json` to update the [sample Lighthouse result JSON](lighthouse-core/test/results/sample_v2.json) kept in the repo for testing. This will also pull any strings needed for localization into the correct files. + +## Protobuf errors + +If there is an error in one of the proto tests (`proto-test.js` or `psi-test.js`), you may need to install `protobuf` locally for debugging. See the instructions for installing and running in the [proto readme](proto/README.md). + +## Adding Images to a Readme + +If you are adding an image to a readme use the absolute path to the image for the specific commit hash where the image was introduced. This requires multiple commits. +1. Make the commit to introduce the image. +1. Get the [absolute path](https://help.github.com/articles/getting-permanent-links-to-files/) to the image with the commit hash e.g. `https://raw.githubusercontent.com/GoogleChrome/lighthouse/e7997b3db01de3553d8cb208a40f3d4fd350195c/assets/example_dev_tools.png` +1. Add to readme as an absolute reference to that image. + +If you are updating an image that already exists: commit it, then update the readme to point the image with that new commits hash absolute url. + +## Pull request titles + +We're using [conventional-commit](https://conventionalcommits.org/) for our commit messages. Since all PRs are squashed, we enforce this format for PR titles rather than individual git commits. A [`commitlint` bot](https://github.com/paulirish/commitlintbot) will update the status of your PR based on the title's conformance. The expected format is: + +> type(scope): message subject + +* The `type` must be one of: `new_audit` `core` `tests` `i18n`, `docs` `deps` `report` `cli` `clients` `misc`. (See [`.cz-config`](https://github.com/GoogleChrome/lighthouse/blob/master/.cz-config.js#L13)) +* The `scope` is optional, but recommended. Any string is allowed; it should indicate what the change affects. +* The `message subject` should be pithy and direct. + +The [commitizen CLI](https://github.com/commitizen/cz-cli) can help to construct these commit messages. + +## Sign the Contributor License Agreement + +We'd love to accept your sample apps and patches! Before we can take them, we have to jump a couple of legal hurdles. + +Please fill out either the individual or corporate Contributor License Agreement (CLA). + +* If you are an individual writing original source code and you're sure you own the intellectual property, then you'll need to sign an [individual CLA](https://developers.google.com/open-source/cla/individual). +* If you work for a company that wants to allow you to contribute your work, then you'll need to sign a [corporate CLA](https://developers.google.com/open-source/cla/corporate). + +Follow either of the two links above to access the appropriate CLA and instructions for how to sign and return it. Once we receive it, we'll be able to +accept your pull requests. ## Tracking Errors diff --git a/docs/hacking-tips.md b/docs/hacking-tips.md index 1e900fcbc132..7d312d7b23ba 100644 --- a/docs/hacking-tips.md +++ b/docs/hacking-tips.md @@ -28,17 +28,6 @@ Use [`--trace-warnings`](https://medium.com/@jasnell/introducing-process-warning node --trace-warnings lighthouse-cli http://example.com ``` -## Updating fixture dumps - -`lighthouse-core/test/results/samples_v2.json` is generated from running LH against -dbw_tester.html. To update this file, start a local server on port `8080` and serve the directory `lighthouse-cli/test/fixtures`. Then run: - -```sh -npm run start -- --output=json --output-path=lighthouse-core/test/results/sample_v2.json http://localhost:8080/dobetterweb/dbw_tester.html -``` - -After updating, consider deleting any irrelevant changes from the diff (exact timings, timestamps, etc). Be sure to run the tests. - ## Iterating on the report This will generate new reports from the same results json. diff --git a/proto/README.md b/proto/README.md index f0f1a735f9c5..beadcbc70915 100644 --- a/proto/README.md +++ b/proto/README.md @@ -1,12 +1,25 @@ ## How to compile protos + use locally -1. Install the proto compiler - 1. Manual install - 1. Get the latest proto [release](https://github.com/protocolbuffers/protobuf/releases) (select one with python included if you want to run this validator) - 1. Install the [C++ Protocol Buffer Runtime](https://github.com/protocolbuffers/protobuf/blob/master/src/README.md) - 1. Brew install - 1. `brew install protobuf` -1. Run `yarn compile-proto` then `yarn build-proto-roundtrip` +You'll need to have v3.7.1 of the protocol-buffer/protobuf compiler installed. (v3.7.1 is known to be compatible, and 3.11.x is known to be **not** compatible.). + +1. Install the proto compiler by either + 1. Brew installing - `brew install protobuf@3.7` + 1. Manually installing from an [official release](https://github.com/protocolbuffers/protobuf/releases/tag/v3.7.1). There are [installation instructions](https://github.com/protocolbuffers/protobuf#protocol-compiler-installation), but these steps have worked well for us: + ```sh + mkdir protobuf-install && cd protobuf-install + curl -L -o protobuf-python-3.7.1.zip https://github.com/protocolbuffers/protobuf/releases/download/v3.7.1/protobuf-python-3.7.1.zip + unzip protobuf-python-3.7.1.zip + cd protobuf-3.7.1 + + cd python + python setup.py build + python setup.py test + (cd .. && autogen.sh && configure && make) + (cd .. && sudo make install) + python setup.py build --cpp_implementation + sudo python setup.py install --cpp_implementation + ``` +1. Run `yarn test-proto` ## Proto Resources - [Protobuf Github Repo](https://github.com/protocolbuffers/protobuf) diff --git a/readme.md b/readme.md index 977d57821b8d..cdc1a491fbd5 100644 --- a/readme.md +++ b/readme.md @@ -274,31 +274,6 @@ yarn yarn build-all ``` -#### installing protobuf -If changing audit output, you'll need to have v3.7.1 of the protocol-buffer/protobuf compiler installed. (v3.7.1 is known to be compatible, and 3.11.x is known to be **not** compatible.). - -Homebrew should be able to install it correctly: `brew install protobuf@3.7` - -But if you want to do it manually, these steps that have worked well for us: - -```sh -mkdir protobuf-install && cd protobuf-install -curl -L -o protobuf-python-3.7.1.zip https://github.com/protocolbuffers/protobuf/releases/download/v3.7.1/protobuf-python-3.7.1.zip -unzip protobuf-python-3.7.1.zip -cd protobuf-3.7.1 - -cd python -python setup.py build -python setup.py test -(cd .. && autogen.sh && configure && make) -(cd .. && sudo make install) -python setup.py build --cpp_implementation -sudo python setup.py install --cpp_implementation -``` - -Also, see the [official installation instructions](https://github.com/protocolbuffers/protobuf#protocol-compiler-installation). - - ### Run ```sh From 45140df44bcfae7d53d38a22c95443bd5aca6401 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Fri, 1 May 2020 14:34:36 -0400 Subject: [PATCH 2/2] add contributing.md to PR message template --- .github/PULL_REQUEST_TEMPLATE.md | 6 +++++- CONTRIBUTING.md | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 82b3b9028f3c..87ec304999c6 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,4 +1,8 @@ - + **Summary** diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 356bb23cb876..1a0d2b8f534a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -37,7 +37,7 @@ A PR for a new audit or changing an existing audit almost always needs the follo 1. **Unit tests**: in the matching test file (e.g. tests for `lighthouse-core/audits/my-swell-audit.js` go in `lighthouse-core/test/audits/my-swell-audit-test.js`). -1. **Smoke (end-to-end) tests**: search through the [existing expectations](lighthouse-cli/test/smokehouse/test-definitions/) to see if there's a logical place to add a check for your change, or (as a last resort) add a new smoke test. +1. **Smoke (end-to-end) tests**: search through the [existing test expectations](lighthouse-cli/test/smokehouse/test-definitions/) to see if there's a logical place to add a check for your change, or (as a last resort) add a new smoke test. 1. Run `yarn update:sample-json` to update the [sample Lighthouse result JSON](lighthouse-core/test/results/sample_v2.json) kept in the repo for testing. This will also pull any strings needed for localization into the correct files. @@ -74,7 +74,7 @@ A PR adding or changing a gatherer almost always needs to include the following: As a result, we mostly rely on smoke testing for gatherers. However, if there are parts of a gatherer that naturally lend themselves to unit testing, the new tests would go in the matching test file (e.g. tests for `lighthouse-core/gather/gatherers/reap.js` go in `lighthouse-core/test/gather/gatherers/reap-test.js`). -1. **Smoke (end-to-end) tests**: search through the [existing expectations](lighthouse-cli/test/smokehouse/test-definitions/) to see if there's a logical place to add a check for your change, or (as a last resort) add a new smoke test if one is required. +1. **Smoke (end-to-end) tests**: search through the [existing test expectations](lighthouse-cli/test/smokehouse/test-definitions/) to see if there's a logical place to add a check for your change, or (as a last resort) add a new smoke test if one is required. It's most important to get true end-to-end coverage, so be sure that audits that consume the new gatherer output are in the expectations. Artifacts can also have expectations for those intermediate results.