diff options
Diffstat (limited to 'testing/web-platform/tests/docs/reviewing-tests')
5 files changed, 337 insertions, 0 deletions
diff --git a/testing/web-platform/tests/docs/reviewing-tests/checklist.md b/testing/web-platform/tests/docs/reviewing-tests/checklist.md new file mode 100644 index 0000000000..be0f4d134e --- /dev/null +++ b/testing/web-platform/tests/docs/reviewing-tests/checklist.md @@ -0,0 +1,157 @@ +# Review Checklist + +The following checklist is provided as a guideline to assist in reviewing +tests; in case of any contradiction with requirements stated elsewhere in the +documentation it should be ignored +(please [file a bug](https://github.com/web-platform-tests/wpt/issues/new)!). + +As noted on the [reviewing tests](./index.md) page, nits need not block PRs +from landing. + + +## All tests + +<label><input type="checkbox"> +The CI jobs on the pull request have passed. +</label> + +<label><input type="checkbox"> +It is obvious what the test is trying to test. +</label> + +<label><input type="checkbox"> +The test passes when it's supposed to pass. +</label> + +<label><input type="checkbox"> +The test fails when it's supposed to fail. +</label> + +<label><input type="checkbox"> +The test is testing what it thinks it's testing. +</label> + +<label><input type="checkbox"> +The spec backs up the expected behavior in the test. +</label> + +<label><input type="checkbox"> +The test is automated as either [reftest](../writing-tests/reftests) or +a [script test](../writing-tests/testharness) unless there's a very good reason for it not to be. +</label> + +<label><input type="checkbox"> +The test does not use external resources. +</label> + +<label><input type="checkbox"> +The test does not use proprietary features (vendor-prefixed or otherwise). +</label> + +<label><input type="checkbox"> +The test does not contain commented-out code. +</label> + +<label><input type="checkbox"> +The test is placed in the relevant directory. +</label> + +<label><input type="checkbox"> +The test has a reasonable and concise filename. +</label> + +<label><input type="checkbox"> +If the test needs code running on the server side, the server code must be +written in Python, and the Python code must not do anything potentially unsafe. +</label> + +<label><input type="checkbox"> +If the test needs to be run in some non-standard configuration or needs user +interaction, it is a manual test. +</label> + +<label><input type="checkbox"> +**Nit**: The title is descriptive but not too wordy. +</label> + + +## Reftests Only + +<label><input type="checkbox"> +The reference file is accurate and will render pixel-perfect +identically to the test on all platforms. +</label> + +<label><input type="checkbox"> +The reference file uses a different technique that won't fail in +the same way as the test. +</label> + +<label><input type="checkbox"> +The test and reference render within a 800x600 viewport, only displaying +scrollbars if their presence is being tested. +</label> + +<label><input type="checkbox"> +**Nit**: The test has a self-describing statement. +</label> + +<label><input type="checkbox"> +**Nit**: The self-describing statement is accurate, precise, simple, and +self-explanatory. Someone with no technical knowledge should be able to say +whether the test passed or failed within a few seconds, and not need to spend +several minutes thinking or asking questions. +</label> + + +## Script Tests Only + +<label><input type="checkbox"> +The number of tests in each file and the test names are consistent +across runs and browsers. It is best to avoid the pattern where there is +a test that asserts that the feature is supported and bails out without +running the rest of the tests in the file if it isn't. +</label> + +<label><input type="checkbox"> +The test avoids patterns that make it less likely to be stable. +In particular, tests should avoid setting internal timeouts, since the +time taken to run it may vary on different devices; events should be used +instead (if at all possible). +</label> + +<label><input type="checkbox"> +The test uses the most specific asserts possible (e.g. doesn't use +`assert_true` for everything). +</label> + +<label><input type="checkbox"> +The test uses `idlharness.js` if it is testing basic IDL-defined behavior. +</label> + +<label><input type="checkbox"> +**Nit**: Tests in a single file are separated by one empty line. +</label> + + +## Visual Tests Only + +<label><input type="checkbox"> +The test has a self-describing statement. +</label> + +<label><input type="checkbox"> +The self-describing statement is accurate, precise, simple, and +self-explanatory. Someone with no technical knowledge should be able to say +whether the test passed or failed within a few seconds, and not need to spend +several minutes thinking or asking questions. +</label> + +<label><input type="checkbox"> +The test renders within a 800x600 viewport, only displaying scrollbars if their +presence is being tested. +</label> + +<label><input type="checkbox"> +The test renders to a fixed, static page with no animation. +</label> diff --git a/testing/web-platform/tests/docs/reviewing-tests/email.md b/testing/web-platform/tests/docs/reviewing-tests/email.md new file mode 100644 index 0000000000..55bbf44367 --- /dev/null +++ b/testing/web-platform/tests/docs/reviewing-tests/email.md @@ -0,0 +1,7 @@ +# Email Filters + +See the [GitHub support page](https://help.github.com/articles/about-email-notifications/) +for how to filter certain types of email notifications. These are the most +useful `Cc` addresses when reviewing in web-platform-tests: +- `review_requested@noreply.github.com` when you are added as a (suggested) `reviewer` on a pull request. +- `assign@noreply.github.com` when you are added as the `assignee` (i.e. as _the_ reviewer) on a pull request. diff --git a/testing/web-platform/tests/docs/reviewing-tests/git.md b/testing/web-platform/tests/docs/reviewing-tests/git.md new file mode 100644 index 0000000000..b74b4b77ae --- /dev/null +++ b/testing/web-platform/tests/docs/reviewing-tests/git.md @@ -0,0 +1,83 @@ +# Working with Pull Requests as a reviewer + +In order to do a thorough review, +it is sometimes desirable to have a local copy of the tests one wishes to review. + +Reviewing tests also often results in wanting a few things to be changed. +Generally, the reviewer should ask the author to make the desired changes. +However, sometimes the original author does not respond to the requests, +or the changes are so trivial (e.g. fixing a typo) +that bothering the original author seems like a waste of time. + +Here is how to do all that. + +## Trivial cases + +If it is possible to review the tests without a local copy, +but the reviewer still wants to make some simple tweaks to the tests before merging, +it is possible to do so via the Github web UI. + +1. Open the pull request. E.g. https://github.com/web-platform-tests/wpt/pull/1234 +2. Go to the  view (e.g. https://github.com/web-platform-tests/wpt/pull/1234/files) +3. Locate the files you wish to change, and click the  icon in the upper right corner +4. Make the desired change +5. Write a commit message (including a good title) at the bottom +6. Make sure the ![Commit directly to the [name-of-the-PR-branch] branch.](../assets/commit-directly.png) radio button is selected. + + _Note: If the PR predates the introduction of this feature by Github, + or if the author of the PR has disabled write-access by reviewers to the PR branch, + this may not be available, + and your only option would be to commit to a new branch, creating a new PR._ +7. Click the  button. + + +## The Normal Way + +This is how to import the Pull Request's branch into your existing local +checkout of the repository. If you don't have one, go [fork][fork], +[clone][clone], and [configure][configure] it. + +1. Move into your local clone: `cd wherever-you-put-your-repo` +2. Add a remote for the PR author's repo: `git remote add <author-id> git://github.com/<author-id>/<repo-name>.git` +3. Fetch the PR: `git fetch <author-id> <name-of-the-PR-branch>` +4. Checkout that branch: `git checkout <name-of-the-PR-branch>` + + _The relevant `<author-id>`, `<repo-name>`, and `<name-of-the-PR-branch>` can be found by looking for this sentence in on the Github page of the PR: + _ + +If all you meant to do was reviewing files locally, you're all set. +If you wish to make changes to the PR branch: + +1. Make changes and [commit][commit] normally +2. Push your changes upstream: `git push <author-id> <name-of-the-PR-branch>` + + _Note: If the PR predates the introduction of this feature by Github, + or if the author of the PR has disabled write-access by reviewers to the PR branch, + this will not work, and you will need to use the alternative described below._ + +If, instead of modifying the existing PR, you wish to make a new one based on it: + +1. Set up a new branch that contains the existing PR by doing one of the following: + 1. Create a new branch from the tip of the PR: + `git branch <your-new-branch> <name-of-the-PR-branch> && git checkout <your-new-branch>` + 2. Create a new branch from `master` and merge the PR into it: + `git branch <your-new-branch> master && git checkout <your-new-branch> && git merge <name-of-the-PR-branch>` +2. Make changes and [commit][commit] normally +3. Push your changes to **your** repo: `git push origin <your-new-branch>` +4. Go to the Github Web UI to [submit a new Pull Request][submit]. + + _Note: You should also close the original pull request._ + +When you're done reviewing or making changes, +you can delete the branch: `git branch -d <name-of-the-PR-branch>` +(use `-D` instead of `-d` to delete a branch that has not been merged into master yet). + +If you do not expect work with more PRs from the same author, +you may also discard your connection to their repo: +`git remote remove <author-id>` + +[clone]: ../writing-tests/github-intro.html#clone +[commit]: ../writing-tests/github-intro.html#commit +[configure]: ../writing-tests/github-intro.html#configure-remote-upstream +[fork]: ../writing-tests/github-intro.html#fork-the-test-repository +[submit]: ../writing-tests/github-intro.html#submit diff --git a/testing/web-platform/tests/docs/reviewing-tests/index.md b/testing/web-platform/tests/docs/reviewing-tests/index.md new file mode 100644 index 0000000000..e313f84596 --- /dev/null +++ b/testing/web-platform/tests/docs/reviewing-tests/index.md @@ -0,0 +1,62 @@ +# Reviewing Tests + +In order to encourage a high level of quality in the W3C test +suites, test contributions must be reviewed by a peer. + +```eval_rst +.. toctree:: + :maxdepth: 1 + + checklist + email + git + reverting +``` + +## Test Review Policy + +The reviewer can be anyone (other than the original test author) that +has the required experience with both the spec under test and with +the [general test guidelines](../writing-tests/general-guidelines). + +The review must happen in public, but there is no requirement for it +to happen in any specific location. In particular if a vendor is +submitting tests that have already been publicly reviewed in their own +review system, that review may be carried forward. For other submissions, we +recommend using GitHub's built-in review tools. + +Regardless of what review tool is used, the review must be clearly +linked in the pull request. + +In general, we tend on the side of merging things with nits (i.e., +anything sub-optimal that isn't absolutely required to be right) and +then opening issues to leaving pull requests open indefinitely waiting +on the original submitter to fix them; when tests are being upstreamed +from vendors it is frequently the case that the author has moved on to +working on other things as tests frequently only get pushed upstream +once the code lands in their implementation. + +To assist with test reviews, a [review checklist](checklist) is available. + +[GitHub.com allows reviewers to formally signal their approval of a pull +request through a dedicated user +interface.](https://help.github.com/en/articles/about-pull-request-reviews) +Every pull request submitted to WPT must be approved by at least one project +collaborator before it can be merged. + +## Notifications + +META.yml files are used to indicate who should be notified of pull +requests. If you are interested in receiving notifications of proposed +changes to tests in a given directory, feel free to add your GitHub account +username to the `suggested_reviewers` list in the META.yml file. + +## Finding contributions to review + +Here are a few search filters to find things to review: + +* [Open PRs (excluding vendor exports)](https://github.com/web-platform-tests/wpt/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+-label%3A%22mozilla%3Agecko-sync%22+-label%3A%22chromium-export%22+-label%3A%22webkit-export%22+-label%3A%22servo-export%22+-label%3Avendor-imports) +* [Reviewed but still open PRs (excluding vendor exports)](https://github.com/web-platform-tests/wpt/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+-label%3Amozilla%3Agecko-sync+-label%3Achromium-export+-label%3Awebkit-export+-label%3Aservo-export+-label%3Avendor-imports+review%3Aapproved+-label%3A%22do+not+merge+yet%22+-label%3A%22status%3Aneeds-spec-decision%22) (Merge? Something left to fix? Ping other reviewer?) +* [Open PRs without reviewers](https://github.com/web-platform-tests/wpt/pulls?q=is%3Apr+is%3Aopen+label%3Astatus%3Aneeds-reviewers) +* [Open PRs with label `infra` (excluding vendor exports)](https://github.com/web-platform-tests/wpt/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+label%3Ainfra+-label%3A%22mozilla%3Agecko-sync%22+-label%3A%22chromium-export%22+-label%3A%22webkit-export%22+-label%3A%22servo-export%22+-label%3Avendor-imports) +* [Open PRs with label `docs` (excluding vendor exports)](https://github.com/web-platform-tests/wpt/pulls?utf8=%E2%9C%93&q=is%3Apr+is%3Aopen+label%3Adocs+-label%3A%22mozilla%3Agecko-sync%22+-label%3A%22chromium-export%22+-label%3A%22webkit-export%22+-label%3A%22servo-export%22+-label%3Avendor-imports) diff --git a/testing/web-platform/tests/docs/reviewing-tests/reverting.md b/testing/web-platform/tests/docs/reviewing-tests/reverting.md new file mode 100644 index 0000000000..277ccb047a --- /dev/null +++ b/testing/web-platform/tests/docs/reviewing-tests/reverting.md @@ -0,0 +1,28 @@ +# Reverting Changes + +Testing is imperfect and from time to time changes are merged into master which +break things for users of web-platform-tests. Such breakage can include: + + * Failures in Travis or Taskcluster runs for this repository, either on the + master branch or on pull requests following the breaking change. + + * Breakage in browser engine repositories which import and run + web-platform-tests, such as Chromium, Edge, Gecko, Servo and WebKit. + + * Breakage in results collections systems for results dashboards, such as + [wpt.fyi](https://wpt.fyi). + + * Breakage in supplemental tooling used by working groups, such as the + [CSS build system][]. + +When such breakage happens, if the maintainers of the affected systems request +it, pull requests to revert the original change should normally be approved and +merged as soon as possible. (When the original change itself was fixing a +serious problem, it's a judgement call, but prefer the fastest path to a stable +state acceptable to everyone.) + +Once a revert has happened, the maintainers of the affected systems are +expected to work with the original patch author to resolve the problem so that +the change can be relanded. A reasonable timeframe to do so is within one week. + +[CSS build system]: https://github.com/web-platform-tests/wpt/tree/master/css/tools |