Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2476573006:
Move layout test documentation to Markdown. (Closed)
DescriptionMove webkit_layout_tests documentation to Markdown.
In the interest of avoiding any controversy, this is a direct conversion
of https://www.chromium.org/developers/testing/webkit-layout-tests
There are many opportunities for improving the contents of the docs, and
for removing redundancy. This can happen in follow-up CLs.
BUG=
Committed: https://crrev.com/ae101a5f5a95fe2748de3f7f0dacfc2e8382ea31
Cr-Commit-Position: refs/heads/master@{#430438}
Patch Set 1 : Move webkit_layout_tests documentation to Markdown. #
Total comments: 37
Patch Set 2 : Addressed feedback #Messages
Total messages: 20 (12 generated)
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by pwnall@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
pwnall@chromium.org changed reviewers: + jsbell@chromium.org
I started moving our documentation for using/writing/debugging LayoutTests into the repository, so we can discuss my proposal to use testharness.js for pixel tests using our code review tools. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
A smattering of feedback. Since this a direct conversion of https://dev.chromium.org/developers/testing/webkit-layout-tests it mostly just inherits issues with that doc, so we can land and iterate, but a few things I noted. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... File docs/testing/webkit_layout_tests.md (right): https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:1: # Layout Tests Can we name this file something w/o webkit ? Why not just layout_tests.md ? https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:1: # Layout Tests Also, this is in docs/testing - do we need that extra new 'testing' directory? https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:22: [currently limited to KitKat and earlier](http://crbug.com/567947)) you need to https https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:40: ### Running the Tests Some mention of testing/xvfb.py would be useful in here somewhere. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:90: Example: To run the layout tests with a debug build of `content_shell`, but only Consider using *** promo here for the example block - see https://gerrit.googlesource.com/gitiles/+/master/Documentation/markdown.md https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:116: **Linux Note**: We try to match the Windows render tree output exactly by Use *** note ? https://gerrit.googlesource.com/gitiles/+/master/Documentation/markdown.md#No... https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:122: **Mac Note**: While the tests are running, a bunch of Appearance settings are Use *** note ? https://gerrit.googlesource.com/gitiles/+/master/Documentation/markdown.md#No... https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:139: | `--debug` | Run the debug build of the test shell (default is release). Equivalent to `-t Debug` | extra whitespace? https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:139: | `--debug` | Run the debug build of the test shell (default is release). Equivalent to `-t Debug` | also, with gn and out/Default is this still a thing that should be used? (I have a wrapper script so I don't know any more...) https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:154: `src/out/{Debug,Release}/layout_test_results/`, and by default launch a browser Now that we use gn should this match defaults, i.e. out/Default ? https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:161: NOTE: Tests which use [testharness.js](https://github.com/w3c/testharness.js/) Use *** note ? https://gerrit.googlesource.com/gitiles/+/master/Documentation/markdown.md#No... https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:185: ```bash Be consistent: ```shell is used above, ```bash here. Does it matter? https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:233: [waterfall builders](http://dev.chromium.org/developers/testing/chromium-build-infrastructure/tour-of-the-chromium-buildbot) https https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:251: [LayoutTests](http://crbug.com/?q=label:LayoutTests) label. Depending on how https, and update the query https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:254: * **Unconfirmed** -- you aren't sure if this is a simple rebaseline, possible Interesting that Untriaged is not used here. I'd add it, e.g. * **Untriaged** -- confirmed but unsure of priority or root cause. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:264: * **Area-WebKit** Some of these labels don't make sense w/ monorail - can you update? https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:519: By default, text-only tests (ones that call `layoutTestController.dumpAsText()`) testRunner ? https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:542: ## W3C Tests This seems like an odd place for this section. (Overall the structure of this doc could be improved)
lgtm to land, but we should do some tidying ASAP.
Description was changed from ========== Move layout test documentation to Markdown. BUG= ========== to ========== Move webkit_layout_tests documentation to Markdown. In the interest of avoiding any controversy, this is a direct conversion of https://www.chromium.org/developers/testing/webkit-layout-tests There are many opportunities for improving the contents of the docs, and for removing redundancy. This can happen in follow-up CLs. BUG= ==========
Thanks for looking at this so quickly! I forgot to mention -- I kept the text almost identical to the Google Sites version in order to avoid any controversy. I added that to the CL description now. PTAL? https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... File docs/testing/webkit_layout_tests.md (right): https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:1: # Layout Tests On 2016/11/07 21:13:10, jsbell wrote: > Can we name this file something w/o webkit ? Why not just layout_tests.md ? Done. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:1: # Layout Tests On 2016/11/07 21:13:10, jsbell wrote: > Also, this is in docs/testing - do we need that extra new 'testing' directory? I was following the site structure. This page has 4 sub-pages, so I imagined testing/ will make a nice home for all of them. I'm a bit overwhelmed by the huge list of files in docs/. That being said, I'm happy to put it under docs/ if you think that makes more sense. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:22: [currently limited to KitKat and earlier](http://crbug.com/567947)) you need to On 2016/11/07 21:13:11, jsbell wrote: > https Done. Also replaced wherever feasible throughout the document. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:40: ### Running the Tests On 2016/11/07 21:13:11, jsbell wrote: > Some mention of testing/xvfb.py would be useful in here somewhere. Added TODO :D https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:90: Example: To run the layout tests with a debug build of `content_shell`, but only On 2016/11/07 21:13:11, jsbell wrote: > Consider using *** promo here for the example block - see > https://gerrit.googlesource.com/gitiles/+/master/Documentation/markdown.md Done. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:116: **Linux Note**: We try to match the Windows render tree output exactly by On 2016/11/07 21:13:10, jsbell wrote: > Use *** note ? > https://gerrit.googlesource.com/gitiles/+/master/Documentation/markdown.md#No... Done. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:122: **Mac Note**: While the tests are running, a bunch of Appearance settings are On 2016/11/07 21:13:11, jsbell wrote: > Use *** note ? > https://gerrit.googlesource.com/gitiles/+/master/Documentation/markdown.md#No... Done. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:139: | `--debug` | Run the debug build of the test shell (default is release). Equivalent to `-t Debug` | On 2016/11/07 21:13:11, jsbell wrote: > also, with gn and out/Default is this still a thing that should be used? (I have > a wrapper script so I don't know any more...) My personal opinion is that --debug should be removed, and the default should be out/Default, not out/Release. I'd rather punt on that, as I'm already in the middle of a discussion regarding changing the test runner script. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:139: | `--debug` | Run the debug build of the test shell (default is release). Equivalent to `-t Debug` | On 2016/11/07 21:13:10, jsbell wrote: > extra whitespace? Done. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:154: `src/out/{Debug,Release}/layout_test_results/`, and by default launch a browser On 2016/11/07 21:13:11, jsbell wrote: > Now that we use gn should this match defaults, i.e. out/Default ? Done. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:161: NOTE: Tests which use [testharness.js](https://github.com/w3c/testharness.js/) On 2016/11/07 21:13:10, jsbell wrote: > Use *** note ? > https://gerrit.googlesource.com/gitiles/+/master/Documentation/markdown.md#No... Done. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:185: ```bash On 2016/11/07 21:13:11, jsbell wrote: > Be consistent: ```shell is used above, ```bash here. Does it matter? Done. Thank you very much for pointing this out! I looked at the docs, and shell isn't even listed as a supported language :D So, I guess it matters. The bad news is I stole "shell" from another document, and we use it quite a lot. I was used to "bash" because of GitHub, so I'm perfectly fine with using "bash" everywhere :D https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:233: [waterfall builders](http://dev.chromium.org/developers/testing/chromium-build-infrastructure/tour-of-the-chromium-buildbot) On 2016/11/07 21:13:11, jsbell wrote: > https Done. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:251: [LayoutTests](http://crbug.com/?q=label:LayoutTests) label. Depending on how On 2016/11/07 21:13:11, jsbell wrote: > https, and update the query Done. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:254: * **Unconfirmed** -- you aren't sure if this is a simple rebaseline, possible On 2016/11/07 21:13:10, jsbell wrote: > Interesting that Untriaged is not used here. I'd add it, e.g. > > * **Untriaged** -- confirmed but unsure of priority or root cause. Done. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:264: * **Area-WebKit** On 2016/11/07 21:13:11, jsbell wrote: > Some of these labels don't make sense w/ monorail - can you update? Done. I replaced the label list with the matching monorails fields. https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:519: By default, text-only tests (ones that call `layoutTestController.dumpAsText()`) Done. I agree that we should have a preferred name, even though both names work [1]. I'm guessing we'll keep the alias, because it's used in 3rd-party tests [2]. [1] https://cs.chromium.org/chromium/src/components/test_runner/test_runner.cc?q=... [2] https://cs.chromium.org/search/?q=layoutTestController+file:third_party/webgl... https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:542: ## W3C Tests On 2016/11/07 21:13:11, jsbell wrote: > This seems like an odd place for this section. (Overall the structure of this > doc could be improved) I very much agree. That seems like a project of its own :)
lgtm ! https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... File docs/testing/webkit_layout_tests.md (right): https://codereview.chromium.org/2476573006/diff/60001/docs/testing/webkit_lay... docs/testing/webkit_layout_tests.md:139: | `--debug` | Run the debug build of the test shell (default is release). Equivalent to `-t Debug` | On 2016/11/07 22:23:24, pwnall wrote: > On 2016/11/07 21:13:11, jsbell wrote: > > also, with gn and out/Default is this still a thing that should be used? (I > have > > a wrapper script so I don't know any more...) > > My personal opinion is that --debug should be removed, and the default should be > out/Default, not out/Release. > > I'd rather punt on that, as I'm already in the middle of a discussion regarding > changing the test runner script. sgtm
The CQ bit was checked by pwnall@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Move webkit_layout_tests documentation to Markdown. In the interest of avoiding any controversy, this is a direct conversion of https://www.chromium.org/developers/testing/webkit-layout-tests There are many opportunities for improving the contents of the docs, and for removing redundancy. This can happen in follow-up CLs. BUG= ========== to ========== Move webkit_layout_tests documentation to Markdown. In the interest of avoiding any controversy, this is a direct conversion of https://www.chromium.org/developers/testing/webkit-layout-tests There are many opportunities for improving the contents of the docs, and for removing redundancy. This can happen in follow-up CLs. BUG= ==========
Message was sent while issue was closed.
Committed patchset #2 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Move webkit_layout_tests documentation to Markdown. In the interest of avoiding any controversy, this is a direct conversion of https://www.chromium.org/developers/testing/webkit-layout-tests There are many opportunities for improving the contents of the docs, and for removing redundancy. This can happen in follow-up CLs. BUG= ========== to ========== Move webkit_layout_tests documentation to Markdown. In the interest of avoiding any controversy, this is a direct conversion of https://www.chromium.org/developers/testing/webkit-layout-tests There are many opportunities for improving the contents of the docs, and for removing redundancy. This can happen in follow-up CLs. BUG= Committed: https://crrev.com/ae101a5f5a95fe2748de3f7f0dacfc2e8382ea31 Cr-Commit-Position: refs/heads/master@{#430438} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/ae101a5f5a95fe2748de3f7f0dacfc2e8382ea31 Cr-Commit-Position: refs/heads/master@{#430438} |
