Separate layout test docs stating facts from guidelines.

Separate layout test docs stating facts from guidelines.

BUG=

Review-Url: https://codereview.chromium.org/2636213003
Cr-Commit-Position: refs/heads/master@{#446491}
Committed: https://chromium.googlesource.com/chromium/src/+/59aadcb2e99470baef2d3cc9e3f4c80b7698d299

[pwnall, 2017-01-19 22:13:53]

Patchset #1 (id:1) has been deleted

[pwnall, 2017-01-19 22:13:59]

Patchset #1 (id:20001) has been deleted

[pwnall, 2017-01-19 23:11:58]

Description was changed from

==========
Streamline layout test documents, pull out contentious parts.

BUG=
==========

to

==========
Separate layout test docs stating facts from guidelines.

BUG=
==========

[pwnall, 2017-01-19 23:12:05]

Patchset #1 (id:40001) has been deleted

[pwnall, 2017-01-19 23:12:26]

[email protected] changed reviewers:
+ [email protected]

[pwnall, 2017-01-19 23:12:27]

PTAL?

[jsbell, 2017-01-20 19:25:21]

lgtm with a couple nits

You should probably get ojan@ to review as well.

https://codereview.chromium.org/2636213003/diff/60001/docs/testing/layout_tes...
File docs/testing/layout_tests_tips.md (right):

https://codereview.chromium.org/2636213003/diff/60001/docs/testing/layout_tes...
docs/testing/layout_tests_tips.md:10: ## General Principles
I'd add a comment to the effect that blink's layout tests have a long history
with multiple coding styles and no firm conventions, and multiple test
harnesses. Most existing tests do not follow this style.

https://codereview.chromium.org/2636213003/diff/60001/docs/testing/writing_la...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2636213003/diff/60001/docs/testing/writing_la...
docs/testing/writing_layout_tests.md:159: * The `<meta name="assert">` describes
the purpose of the entire file, and
This is awkward. ".. is not redundant to title" but "don't add... when title is
sufficient". The first part refers to this specific example while the last part
is general, but they sound contradictory.

First off, somehow I never noticed <meta name=assert> before. Maybe this should
go in the tips document? 

Otherwise, reword to be prescriptive, e.g. "Only include <meta name=assert> when
it is not redundant with the title."

[pwnall, 2017-01-21 01:15:50]

jsbell: Thank you very much for the initial feedback!

https://codereview.chromium.org/2636213003/diff/60001/docs/testing/layout_tes...
File docs/testing/layout_tests_tips.md (right):

https://codereview.chromium.org/2636213003/diff/60001/docs/testing/layout_tes...
docs/testing/layout_tests_tips.md:10: ## General Principles
On 2017/01/20 19:25:21, jsbell wrote:
> I'd add a comment to the effect that blink's layout tests have a long history
> with multiple coding styles and no firm conventions, and multiple test
> harnesses. Most existing tests do not follow this style.

Done.

Argh, thank you for catching this! I intended to add an introduction along these
lines, and then I completely forgot.

https://codereview.chromium.org/2636213003/diff/60001/docs/testing/writing_la...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2636213003/diff/60001/docs/testing/writing_la...
docs/testing/writing_layout_tests.md:159: * The `<meta name="assert">` describes
the purpose of the entire file, and
On 2017/01/20 19:25:21, jsbell wrote:
> This is awkward. ".. is not redundant to title" but "don't add... when title
is
> sufficient". The first part refers to this specific example while the last
part
> is general, but they sound contradictory.
> 
> First off, somehow I never noticed <meta name=assert> before. Maybe this
should
> go in the tips document? 
> 
> Otherwise, reword to be prescriptive, e.g. "Only include <meta name=assert>
when
> it is not redundant with the title."

Done.
Thinking back, I don't recall having used it in real tests, so I just removed
it.
Hopefully, if it matters enough, someone else will add it back with a better
explanation.

[ojan, 2017-01-26 01:05:49]

https://codereview.chromium.org/2636213003/diff/80001/docs/testing/writing_la...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2636213003/diff/80001/docs/testing/writing_la...
docs/testing/writing_layout_tests.md:157: can't go wrong by having it in!
This is not what I see as standard practice. I think most reviewers would ask
you to remove an unnecessary <meta charset>.

[pwnall, 2017-01-26 23:14:15]

Thank you very much for the feedback!

https://codereview.chromium.org/2636213003/diff/80001/docs/testing/writing_la...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2636213003/diff/80001/docs/testing/writing_la...
docs/testing/writing_layout_tests.md:157: can't go wrong by having it in!
On 2017/01/26 01:05:49, ojan wrote:
> This is not what I see as standard practice. I think most reviewers would ask
> you to remove an unnecessary <meta charset>.

Done.

[pwnall, 2017-01-26 23:14:18]

The CQ bit was checked by [email protected]

[pwnall, 2017-01-26 23:14:19]

The patchset sent to the CQ was uploaded after l-g-t-m from [email protected]
Link to the patchset: https://codereview.chromium.org/2636213003/#ps100001
(title: "Addressed ojan feedback.")

[commit-bot: I haz the power, 2017-01-26 23:15:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-26 23:27:25]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1485472458601060,
"parent_rev": "8b610ac91e1304cdb809b0afee33ef3eeb7c4964", "commit_rev":
"59aadcb2e99470baef2d3cc9e3f4c80b7698d299"}

[commit-bot: I haz the power, 2017-01-26 23:28:04]

Description was changed from

==========
Separate layout test docs stating facts from guidelines.

BUG=
==========

to

==========
Separate layout test docs stating facts from guidelines.

BUG=

Review-Url: https://codereview.chromium.org/2636213003
Cr-Commit-Position: refs/heads/master@{#446491}
Committed:
https://chromium.googlesource.com/chromium/src/+/59aadcb2e99470baef2d3cc9e3f4...
==========

[commit-bot: I haz the power, 2017-01-26 23:28:05]

Committed patchset #3 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/59aadcb2e99470baef2d3cc9e3f4...