|
|
DescriptionRemove <title> in writing_layout_tests documentation as it
doesn't appear in test results (thanks to Alan for pointing it out).
Instead we should focus on writing good test descriptions.
BUG=
Review-Url: https://codereview.chromium.org/2617083002
Cr-Commit-Position: refs/heads/master@{#441876}
Committed: https://chromium.googlesource.com/chromium/src/+/f0c6c6901e6addca3e3025c07f999333a9c53901
Patch Set 1 #Patch Set 2 : Remove mentioning of <title> in writing_layout_tests.md since it doesnt appear in test results and … #Messages
Total messages: 21 (8 generated)
[email protected] changed reviewers: + [email protected]
Hi Alan, Thanks for pointing out that <title> does not appear and test() should always come with a description parameter since that's what's shown in the test output. Let's update the documentation accordingly? Thanks, Kevin
On 2017/01/06 at 02:52:38, ktyliu wrote: > Hi Alan, > > Thanks for pointing out that <title> does not appear and test() should always come with a description parameter since that's what's shown in the test output. > > Let's update the documentation accordingly? > > Thanks, > Kevin this is excellent :) might be even better if we included in the documentation that explanation of why it's important to include the test case description
Thanks for the comment. Given <title> doesn't appear at all in test results, how about let's just remove <title> in the documentation? The remaining documentation should be clear then that test case description is what we should write.
Thanks for going the extra mile! lgtm
[email protected] changed reviewers: + [email protected], [email protected]
LGTM but adding thakis@ for a second opinion. He's sick atm so we can probably just land this, but nico I'd be interested in your thoughts after this lands (we can always revert it).
Description was changed from ========== Remove incorrect writing_layout_tests documentation on <title> with single test case BUG= ========== to ========== Remove <title> in writing_layout_tests documentation as it doesn't appear in test results. Instead we should focus on writing good test descriptions. BUG= ==========
Description was changed from ========== Remove <title> in writing_layout_tests documentation as it doesn't appear in test results. Instead we should focus on writing good test descriptions. BUG= ========== to ========== Remove <title> in writing_layout_tests documentation as it doesn't appear in test results (thanks to Alan for pointing it out). Instead we should focus on writing good test descriptions. BUG= ==========
Thanks for the review and adding thakis@ for opinion. Okay, let me land the review as suggested. (and updated the issue subject/description to the new scope)
The CQ bit was checked by [email protected]
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": 20001, "attempt_start_ts": 1483677909922140, "parent_rev": "d6dfbbf145c5977edd5d53c056856286506a7434", "commit_rev": "f0c6c6901e6addca3e3025c07f999333a9c53901"}
Message was sent while issue was closed.
Description was changed from ========== Remove <title> in writing_layout_tests documentation as it doesn't appear in test results (thanks to Alan for pointing it out). Instead we should focus on writing good test descriptions. BUG= ========== to ========== Remove <title> in writing_layout_tests documentation as it doesn't appear in test results (thanks to Alan for pointing it out). Instead we should focus on writing good test descriptions. BUG= Review-Url: https://codereview.chromium.org/2617083002 Cr-Commit-Position: refs/heads/master@{#441876} Committed: https://chromium.googlesource.com/chromium/src/+/f0c6c6901e6addca3e3025c07f99... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/f0c6c6901e6addca3e3025c07f99...
Message was sent while issue was closed.
[email protected] changed reviewers: + [email protected]
Message was sent while issue was closed.
I general, I think blink team likes to discuss things like this in minute detail on blink-dev before changing things, so that's what I'd do if I wanted to change this file. I think Victor wrote this file, so he might have opinions. The change makes sense to me thouh.
Message was sent while issue was closed.
On 2017/01/09 16:45:40, Nico wrote: > I general, I think blink team likes to discuss things like this in minute detail > on blink-dev before changing things, so that's what I'd do if I wanted to change > this file. > I think Victor wrote this file, so he might have opinions. > > The change makes sense to me thouh. This was discussed when the paragraph was introduced. https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l... If the title does not show up in the testing output, that seems like a bug. Can you please point me to the CL where that happens? FWIW, the code in testharness.js at least implies that the title would be used. https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources... https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources... https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources... https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources...
Message was sent while issue was closed.
On 2017/01/09 17:46:22, pwnall wrote: > On 2017/01/09 16:45:40, Nico wrote: > > I general, I think blink team likes to discuss things like this in minute > detail > > on blink-dev before changing things, so that's what I'd do if I wanted to > change > > this file. > > I think Victor wrote this file, so he might have opinions. > > > > The change makes sense to me thouh. > > This was discussed when the paragraph was introduced. > > https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l... > > If the title does not show up in the testing output, that seems like a bug. Can > you please point me to the CL where that happens? > > FWIW, the code in testharness.js at least implies that the title would be used. > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources... > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources... > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources... > https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/resources... Also, in general, this document has a few things that were not properly discussed, and will be removed and re-discussed... but I didn't think that <title> was one of them.
Message was sent while issue was closed.
It was pointed out in another code review that <title> doesn't appear and references to sample tests in codebase pointed to, but I've just tried and <title> text do appear if test description is left out. Let me revert this patch then. Thanks for the explanation and pointers.
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2624533003/ by [email protected]. The reason for reverting is: pwnall@ pointed out that <title> is used in next_default_test_name(). |