summaryrefslogtreecommitdiffstats
path: root/testing/web-platform/tests/docs/reviewing-tests
diff options
context:
space:
mode:
authorDaniel Baumann <daniel.baumann@progress-linux.org>2024-04-07 19:33:14 +0000
committerDaniel Baumann <daniel.baumann@progress-linux.org>2024-04-07 19:33:14 +0000
commit36d22d82aa202bb199967e9512281e9a53db42c9 (patch)
tree105e8c98ddea1c1e4784a60a5a6410fa416be2de /testing/web-platform/tests/docs/reviewing-tests
parentInitial commit. (diff)
downloadfirefox-esr-36d22d82aa202bb199967e9512281e9a53db42c9.tar.xz
firefox-esr-36d22d82aa202bb199967e9512281e9a53db42c9.zip
Adding upstream version 115.7.0esr.upstream/115.7.0esr
Signed-off-by: Daniel Baumann <daniel.baumann@progress-linux.org>
Diffstat (limited to 'testing/web-platform/tests/docs/reviewing-tests')
-rw-r--r--testing/web-platform/tests/docs/reviewing-tests/checklist.md157
-rw-r--r--testing/web-platform/tests/docs/reviewing-tests/email.md7
-rw-r--r--testing/web-platform/tests/docs/reviewing-tests/git.md83
-rw-r--r--testing/web-platform/tests/docs/reviewing-tests/index.md62
-rw-r--r--testing/web-platform/tests/docs/reviewing-tests/reverting.md23
5 files changed, 332 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 ![Files changed](../assets/files-changed.png) view (e.g. https://github.com/web-platform-tests/wpt/pull/1234/files)
+3. Locate the files you wish to change, and click the ![pencil](../assets/pencil-icon.png) 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 ![Commit Changes](../assets/commitbtn.png) 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:
+ ![Add more commits by pushing to the name-of-the-PR-branch branch on author-id/repo-name.](../assets/more-commits.png)_
+
+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..d374f0558e
--- /dev/null
+++ b/testing/web-platform/tests/docs/reviewing-tests/reverting.md
@@ -0,0 +1,23 @@
+# 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).
+
+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.