Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2681293004:
Clarify documentation about importing new WPT directories. (Closed)
DescriptionClarify documentation about importing new WPT directories.
This CL also adds a section about what kinds of changes
will be upstreamed, and the contributing guidelines for
web-platform-tests.
Review-Url: https://codereview.chromium.org/2681293004
Cr-Commit-Position: refs/heads/master@{#449798}
Committed: https://chromium.googlesource.com/chromium/src/+/9c9781a50ee6ae0a81392efbc3ed2c4b9938a0b5
Patch Set 1 #Patch Set 2 : Expand #
Total comments: 6
Patch Set 3 : Apply feedback #Messages
Total messages: 17 (7 generated)
qyearsley@chromium.org changed reviewers: + foolip@chromium.org
Does this seem clearer? Any other minor edits that could be made?
LGTM, several people have been wondering about this, thanks! One additional thing is how to add a new directory entirely. Can you say that just adding a new directory works as long as it's also added to W3CImportExpectations? And also, that the only way to add the OWNERS file is to do so in an upstream web-platform-tests PR.
Description was changed from ========== Clarify documentation about importing new WPT directories. ========== to ========== Clarify documentation about importing new WPT directories. This CL also adds a section about what kinds of changes will be upstreamed, and the contributing guidelines for web-platform-tests. ==========
qyearsley@chromium.org changed reviewers: + rbyers@chromium.org
Now updated this CL with info about adding an entirely new directory, and based on https://github.com/whatwg/meta/pull/17, also added a section about contributing guidelines. Can the wording still be made clearer?
On 2017/02/10 at 17:55:10, qyearsley wrote: > Now updated this CL with info about adding an entirely new directory, and based on https://github.com/whatwg/meta/pull/17, also added a section about contributing guidelines. > > Can the wording still be made clearer? Also, given Rick's note added in https://codereview.chromium.org/2690553003: > Note that tests in Web Platform Tests are expected to match behavior defined by the relevant WHATWG or W3C specification, not simply Blink's behavior. If in doubt, please request code review from someone with expertise in the relevant specification text. I'm not sure if part of this change is now repetitive or redundant.
Oh sorry - I didn't realize you had this CL in progress (a down-side to using codereview for docs I guess). I just did something quick and dirty, what you've written looks better in general IMHO.
https://codereview.chromium.org/2681293004/diff/20001/docs/testing/web_platfo... File docs/testing/web_platform_tests.md (right): https://codereview.chromium.org/2681293004/diff/20001/docs/testing/web_platfo... docs/testing/web_platform_tests.md:102: Remember that in general, tests correspond to specs, and tests in Web Platform I guess I'd suggest keeping everything above, and just using the (slightly more detailed) paragraph I wrote for this in place of these 3 lines.
I'll hold off with any of my own changes until this has landed. (Mainly about things to consider when writing and reviewing tests.) https://codereview.chromium.org/2681293004/diff/20001/docs/testing/web_platfo... File docs/testing/web_platform_tests.md (right): https://codereview.chromium.org/2681293004/diff/20001/docs/testing/web_platfo... docs/testing/web_platform_tests.md:96: should be added to `W3CImportExpectations`. I think we should recommend doing this upstream since that's the only way to add a correct OWNERS file. It is of course OK to do that after adding the initial tests. https://codereview.chromium.org/2681293004/diff/20001/docs/testing/web_platfo... docs/testing/web_platform_tests.md:101: contributors](https://github.com/w3c/web-platform-tests/blob/master/CONTRIBUTING.md). This documentation is just legal stuff, I think linking to it isn't very helpful. Perhaps once gsnedders has refactored some documentation we can link to that. I was also tweaking this documentation, this is what I had written: Most tests are written using testharness.js, see [Writing Layout Tests](./writing_layout_tests.md) and [Layout Tests Tips](./layout_tests_tips.md) for general guidelines.
The CQ bit was checked by qyearsley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org Link to the patchset: https://codereview.chromium.org/2681293004/#ps40001 (title: "Apply feedback")
https://codereview.chromium.org/2681293004/diff/20001/docs/testing/web_platfo... File docs/testing/web_platform_tests.md (right): https://codereview.chromium.org/2681293004/diff/20001/docs/testing/web_platfo... docs/testing/web_platform_tests.md:96: should be added to `W3CImportExpectations`. On 2017/02/10 at 23:43:33, foolip wrote: > I think we should recommend doing this upstream since that's the only way to add a correct OWNERS file. It is of course OK to do that after adding the initial tests. Makes sense, now changed this recommendation. https://codereview.chromium.org/2681293004/diff/20001/docs/testing/web_platfo... docs/testing/web_platform_tests.md:101: contributors](https://github.com/w3c/web-platform-tests/blob/master/CONTRIBUTING.md). On 2017/02/10 at 23:43:33, foolip wrote: > This documentation is just legal stuff, I think linking to it isn't very helpful. Perhaps once gsnedders has refactored some documentation we can link to that. > > I was also tweaking this documentation, this is what I had written: > > Most tests are written using testharness.js, see > [Writing Layout Tests](./writing_layout_tests.md) and > [Layout Tests Tips](./layout_tests_tips.md) for general guidelines. For this CL, I'll just remove this section; we could expand it later. https://codereview.chromium.org/2681293004/diff/20001/docs/testing/web_platfo... docs/testing/web_platform_tests.md:102: Remember that in general, tests correspond to specs, and tests in Web Platform On 2017/02/10 at 18:15:52, Rick Byers wrote: > I guess I'd suggest keeping everything above, and just using the (slightly more detailed) paragraph I wrote for this in place of these 3 lines. Removing these 3 lines
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1486770980728890,
"parent_rev": "f3b00fa5e74ebdcdf63419a8c8b1e2cfe7d7024f", "commit_rev":
"9c9781a50ee6ae0a81392efbc3ed2c4b9938a0b5"}
Message was sent while issue was closed.
Description was changed from ========== Clarify documentation about importing new WPT directories. This CL also adds a section about what kinds of changes will be upstreamed, and the contributing guidelines for web-platform-tests. ========== to ========== Clarify documentation about importing new WPT directories. This CL also adds a section about what kinds of changes will be upstreamed, and the contributing guidelines for web-platform-tests. Review-Url: https://codereview.chromium.org/2681293004 Cr-Commit-Position: refs/heads/master@{#449798} Committed: https://chromium.googlesource.com/chromium/src/+/9c9781a50ee6ae0a81392efbc3ed... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9c9781a50ee6ae0a81392efbc3ed... |
