New layout test guidelines

[Victor Costan]

Dear blink-dev,

I recently landed an almost-complete rewrite of the document that has
best practices for writing layout tests.
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_layout_tests.md

The landed version of the document is intended to be a draft that
serves as a subject for public discussion. Please see this as an
opportunity to reduce the number of nit comments you have to create in
code review to help new project members get up to speed, and
contribute your advice and fixes! The CL's code review was focused on
removing glaring errors, so the community can focus on more subtle
issues.

Here are the more contentious issues that (I remember) surfaced during
the review, in (rough) document appearance order.


1. Prefer === to ==.

Google's current style guide is currently silent on the issue, but I
was told that it used to recommend ==. My reasons for recommending ===
by default are that (I think) it's easier to debug a failing test that
uses ===, that testharness.js uses === in assert_equals and
same_value, and that WebKit also (subtly) recommends ===.

2. Gracefully degrade to manual tests when testing APIs are not present.

Degrading to manual tests makes it possible to iterate on a layout
test in a browser, and to compare behavior across browsers. On the
flip side, the degradation requires a non-trivial amount of code, and
may add complexity by turning a sync test into an async test.

3. Prefer modern platform features (that are shipped on all major
developed browsers) to legacy features. Specific examples: const and
let > var, classes > other OOP constructs, Promises > ad-hoc async
constructs.

This clashes with the WPT recommendation for have cross-platform
tests. My reasoning is that the WPT project is only relevant for
browsers that are still under development (e.g., not IE), so it makes
sense to use a feature shipped in all major browsers to gain
readability.

The most contentious specific example appears to be preferring
const+let to var. My reasons are that const and let are less
surprising (block scoping, no hoisting), and allow the writer to
express a mutability intention. On the flip side, the vast majority of
current tests use var.

4. Always use <meta charset=utf-8> (even for pure ASCII tests).

This is more strict than the WPT recommendation, which makes an
exception for pure ASCII tests. My only reason for a stricter rule is
that Firefox issues a console warning without the <meta>.


Thank you very much,
Victor

[Robert Hogan]

You don't really cover check-layout or painting tests in there. Those are the two most popular forms of test when writing layout/ or paint/ code. A section on each would be very useful I think. 

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Philip Jägenstedt]

On Tue, Nov 29, 2016 at 3:57 AM Victor Costan <pwn...@chromium.org> wrote:

Dear blink-dev,

I recently landed an almost-complete rewrite of the document that has
best practices for writing layout tests.
https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_layout_tests.md

The landed version of the document is intended to be a draft that
serves as a subject for public discussion. Please see this as an
opportunity to reduce the number of nit comments you have to create in
code review to help new project members get up to speed, and
contribute your advice and fixes! The CL's code review was focused on
removing glaring errors, so the community can focus on more subtle
issues.

Here are the more contentious issues that (I remember) surfaced during
the review, in (rough) document appearance order.


1. Prefer === to ==.

Google's current style guide is currently silent on the issue, but I
was told that it used to recommend ==. My reasons for recommending ===
by default are that (I think) it's easier to debug a failing test that
uses ===, that testharness.js uses === in assert_equals and
same_value, and that WebKit also (subtly) recommends ===.

That assert_equals uses === internally is clearly good, but I don't think it needs to be recommended in general. If one has a counter and wants to end the test on the count of 3, then if (count == 3) t.done() is just fine I think.

There's a similar thing with truthy values, and I don't think if (document.doctype !== null) would be an improvement over if (document.doctype).

2. Gracefully degrade to manual tests when testing APIs are not present.

Degrading to manual tests makes it possible to iterate on a layout
test in a browser, and to compare behavior across browsers. On the
flip side, the degradation requires a non-trivial amount of code, and
may add complexity by turning a sync test into an async test.

I think it's OK to leave this to author and reviewer judgement, to do it if it's plausible that anyone will test it manually and adding the extra stuff isn't much work.

 

3. Prefer modern platform features (that are shipped on all major
developed browsers) to legacy features. Specific examples: const and
let > var, classes > other OOP constructs, Promises > ad-hoc async
constructs.

This clashes with the WPT recommendation for have cross-platform
tests. My reasoning is that the WPT project is only relevant for
browsers that are still under development (e.g., not IE), so it makes
sense to use a feature shipped in all major browsers to gain
readability.

The most contentious specific example appears to be preferring
const+let to var. My reasons are that const and let are less
surprising (block scoping, no hoisting), and allow the writer to
express a mutability intention. On the flip side, the vast majority of
current tests use var.

I think people should be free to use const, let, classes, promises, arrow functions and $futurething when it's interoperable enough for web-platform-tests, with the understanding that it's your job to fix any problems it causes, to avoid unrelated failures.

For example, for (const x of []) doesn't work in in Firefox <51, so trying to use const everywhere can currently get in the way of testing on stable Firefox.

Ultimately I think tooling should solve this problem. When we can add web-platform-tests directly as part of Chromium, we should also invest in the tooling to run those tests on stable and dev versions of all browsers before landing the Chromium CL, and one will not need to guess whether the tests will work or not.

 

4. Always use <meta charset=utf-8> (even for pure ASCII tests).

This is more strict than the WPT recommendation, which makes an
exception for pure ASCII tests. My only reason for a stricter rule is
that Firefox issues a console warning without the <meta>.

I'd be happy to not have this requirement, but it's pretty harmless.

[PhistucK]

On Tue, Nov 29, 2016 at 1:32 PM, Philip Jägenstedt <foo...@chromium.org> wrote:

That assert_equals uses === internally is clearly good, but I don't think it needs to be recommended in general. If one has a counter and wants to end the test on the count of 3, then if (count == 3) t.done() is just fine I think.

There's a similar thing with truthy values, and I don't think if (document.doctype !== null) would be an improvement over if (document.doctype).

​It is about habits. If you are used to using ==, you will likely use it in ambiguous cases as well as unambiguous cases​ and thus hurt debugging failures. It is just a single extra character when typing, so I respectfully disagree with you. It should be required.

Truthy values is a different thing and they should be used. Not using them makes the code significantly longer for mostly no actual benefit.

☆PhistucK

[Philip Jägenstedt]

On Tue, Nov 29, 2016 at 2:37 PM PhistucK <phis...@gmail.com> wrote:

On Tue, Nov 29, 2016 at 1:32 PM, Philip Jägenstedt <foo...@chromium.org> wrote:

That assert_equals uses === internally is clearly good, but I don't think it needs to be recommended in general. If one has a counter and wants to end the test on the count of 3, then if (count == 3) t.done() is just fine I think.

There's a similar thing with truthy values, and I don't think if (document.doctype !== null) would be an improvement over if (document.doctype).

​It is about habits. If you are used to using ==, you will likely use it in ambiguous cases as well as unambiguous cases​ and thus hurt debugging failures. It is just a single extra character when typing, so I respectfully disagree with you. It should be required.

If a reviewer asks for it for, fine, but requiring it amounts to asking reviewers to enforce it even when they see no value, or when they're not concerned about the habits of the author. Maybe I've been lucky, but the difference between == and === hasn't ever been the cause of wasted debugging time for me.

 

Truthy values is a different thing and they should be used. Not using them makes the code significantly longer for mostly no actual benefit.

Glad we agree on that one :)

[Nico Weber]

If you're changing things anyway, does it make sense to align the JS recommendations in layout tests with the JS recommendations elsewhere in chrome (e.g. webui)? Or are layout tests too different from regular JS for this to make sense?

[Victor Costan]

On Mon, Nov 28, 2016 at 11:36 PM, Robert Hogan <robh...@gmail.com> wrote:
> You don't really cover check-layout or painting tests in there. Those are
> the two most popular forms of test when writing layout/ or paint/ code. A
> section on each would be very useful I think.

I haven't worked in either of those two parts of the codebase, so I'm
not qualified to write those sections from scratch.

If you think there's value in having a uniform voice throughout the
document, I'd be happy to get taught about how these tests work, and
then write what I learned. Otherwise, more knowledgeable folks should
submit a CL. Be the change you want to see in the world :)

Victor

[Victor Costan]

On Tue, Nov 29, 2016 at 8:27 AM, Nico Weber <tha...@chromium.org> wrote:
> If you're changing things anyway, does it make sense to align the JS
> recommendations in layout tests with the JS recommendations elsewhere in
> chrome (e.g. webui)? Or are layout tests too different from regular JS for
> this to make sense?

FWIW, I don't see my writeup as changing things :) Layout tests (used
to) have very little documentation, and I tried to put together the
best practices that I've learned over time.

That being said, thank you very much for bringing up WebUI!

I think that layout tests and WebUI are, in a sense, opposite ends of
a spectrum -- WebUI is a large modular codebase, while layout tests
are self-contained and (hopefully) small independent pages. This
warrants some style differences -- here are some examples below:
* HTML/CSS/JS separation makes perfect sense for apps, but is an
anti-pattern in layout tests
* IMO the JSDoc overhead is not warranted for layout tests
* Closure, Grit, Polymer, and $ do not exist for layout tests

I think that we should harmonize the guidelines where it makes sense,
to reduce the context switching overhead for developers working on all
parts of the browser. I imagine that getting there would first require
doing a pass through the WebUI style guide and separating the parts
that summarize the Google HTML/CSS/JS guides from the parts that
diverge from the Google guides.

My remarks above rely on the assumption that
https://www.chromium.org/developers/web-development-style-guide
contains an accurate and complete WebUI style guide. I found that page
from https://www.chromium.org/developers/webui

Victor

[Victor Costan]

On Tue, Nov 29, 2016 at 6:12 AM, Philip Jägenstedt <foo...@chromium.org> wrote:
>> On Tue, Nov 29, 2016 at 1:32 PM, Philip Jägenstedt <foo...@chromium.org>
>> wrote:
>>>
>>> That assert_equals uses === internally is clearly good, but I don't think
>>> it needs to be recommended in general. If one has a counter and wants to end
>>> the test on the count of 3, then if (count == 3) t.done() is just fine I
>>> think.
>>>
>>> There's a similar thing with truthy values, and I don't think if
>>> (document.doctype !== null) would be an improvement over if
>>> (document.doctype).
>>
>>
>> It is about habits. If you are used to using ==, you will likely use it in
>> ambiguous cases as well as unambiguous cases and thus hurt debugging
>> failures. It is just a single extra character when typing, so I respectfully
>> disagree with you. It should be required.
>
>
> If a reviewer asks for it for, fine, but requiring it amounts to asking
> reviewers to enforce it even when they see no value, or when they're not
> concerned about the habits of the author. Maybe I've been lucky, but the
> difference between == and === hasn't ever been the cause of wasted debugging
> time for me.

How about adding a clause OKing == in contexts where it is obvious
(e.g. can be inferred from the previous 10-25 lines) that the two
sides have the same type?

If we'd go down that route, I'd like to add a warning encouraging
authors to familiarize themselves with [1, 2] before using ==.

The point about reviewers being / not being concerns with the authors'
habits makes me worried about the possibility of ending up in a world
where some contributors receive code review feedback along the lines
of "I don't trust you to use == yet / anymore". If at all possible,
I'd rather that our guidelines are conducive to more objective
feedback, like "you used ==, but I can't see why the two variables
have the same type".

[1] http://dorey.github.io/JavaScript-Equality-Table/
[2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness

>> Truthy values is a different thing and they should be used. Not using them
>> makes the code significantly longer for mostly no actual benefit.
>
>
> Glad we agree on that one :)

The draft recommendation says "prefer === over ==". I did mean to
imply anything about "if(document.doctype)". I think that the JS
truthiness rules are similar enough to other languages that truthiness
is easy to remember.

Do you think I should add wording that explicitly OKs the use of
non-boolean values in contexts that require booleans?

Thank you very much for sharing your thoughts!
Victor

[Geoffrey Sneddon]

Couple of minor points about WPT:

On Tue, Nov 29, 2016 at 2:57 AM, Victor Costan <pwn...@chromium.org> wrote:
> Dear blink-dev,
>
> I recently landed an almost-complete rewrite of the document that has
> best practices for writing layout tests.
> https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_layout_tests.md
>
> The landed version of the document is intended to be a draft that
> serves as a subject for public discussion. Please see this as an
> opportunity to reduce the number of nit comments you have to create in
> code review to help new project members get up to speed, and
> contribute your advice and fixes! The CL's code review was focused on
> removing glaring errors, so the community can focus on more subtle
> issues.
>
> Here are the more contentious issues that (I remember) surfaced during
> the review, in (rough) document appearance order.
>
>
> 1. Prefer === to ==.
>
> Google's current style guide is currently silent on the issue, but I
> was told that it used to recommend ==. My reasons for recommending ===
> by default are that (I think) it's easier to debug a failing test that
> uses ===, that testharness.js uses === in assert_equals and
> same_value, and that WebKit also (subtly) recommends ===.

testharness.js *doesn't* use `===`, it uses a JS implementation of the
ES SameValue spec function (the differences are that NaN is the same
value as NaN and +0 and -0 are different values).

> 2. Gracefully degrade to manual tests when testing APIs are not present.
>
> Degrading to manual tests makes it possible to iterate on a layout
> test in a browser, and to compare behavior across browsers. On the
> flip side, the degradation requires a non-trivial amount of code, and
> may add complexity by turning a sync test into an async test.
>
> 3. Prefer modern platform features (that are shipped on all major
> developed browsers) to legacy features. Specific examples: const and
> let > var, classes > other OOP constructs, Promises > ad-hoc async
> constructs.
>
> This clashes with the WPT recommendation for have cross-platform
> tests. My reasoning is that the WPT project is only relevant for
> browsers that are still under development (e.g., not IE), so it makes
> sense to use a feature shipped in all major browsers to gain
> readability.
>
> The most contentious specific example appears to be preferring
> const+let to var. My reasons are that const and let are less
> surprising (block scoping, no hoisting), and allow the writer to
> express a mutability intention. On the flip side, the vast majority of
> current tests use var.

The WPT requirement is that the test must work in the latest stable
releases of Edge, Firefox, Chrome, and Safari. I think the only place
where we deviate from this is much of the tooling, with testharness.js
itself trying to support as much is as practical without
overcomplicating it (e.g., adding extra now-optional arguments is
fine, rewriting large parts isn't; but this also implies that it won't
get large-scale changes to move it over to classes, etc.).

For example: const and let are fine since September (Safari 10),
classes have been fine since March (Firefox 45), Promises since last
December (Firefox 43).

> 4. Always use <meta charset=utf-8> (even for pure ASCII tests).
>
> This is more strict than the WPT recommendation, which makes an
> exception for pure ASCII tests. My only reason for a stricter rule is
> that Firefox issues a console warning without the <meta>.

I'm pretty sure nobody, Firefox included, requires charset generally
in tests, despite their console warnings.

/gsnedders

[Victor Costan]

Thank you very much for the correction! Does this mean that assert_equals essentialy reimplements Object.is?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is

Thanks,

    Victor

[Geoffrey Sneddon]

Yes. (Object.is just returns the value of SameValue, so it's literally
identical.)

/g

[Christian Biesinger]

check-layout has a version that uses testharness.js and I would
encourage you to use that one (just include check-layout-th.js in
addition to the testharness files), e.g.
https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/css3/flexbox/relayout-image-load.html?q=check-layout-th.js&sq=package:chromium&l=9&dr=C

On the document: It encourages a lot more metadata in the tests (the
<meta>, a number of <link>s). I assume that's intentional for WPT
compatibility (?), but it does not match how the vast majority of our
tests are written. Should this be acknowledged somehow? I'm not sure I
like this verboseness for tests that will not get upstreamed :/

Christian

[Philip Rogers]

Is it possible to enforce these conventions with a style check on upload? Clang formatting C++ has been a boon to the project, and I bet we'd see similar results with javascript.

>> email to blink-dev+unsubscribe@chromium.org.
>>
>

[Geoffrey Sneddon]

On Tue, Nov 29, 2016 at 2:57 AM, Victor Costan <pwn...@chromium.org> wrote:

> Dear blink-dev,
>
> I recently landed an almost-complete rewrite of the document that has
> best practices for writing layout tests.
> https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_layout_tests.md
>
> The landed version of the document is intended to be a draft that
> serves as a subject for public discussion. Please see this as an
> opportunity to reduce the number of nit comments you have to create in
> code review to help new project members get up to speed, and
> contribute your advice and fixes! The CL's code review was focused on
> removing glaring errors, so the community can focus on more subtle
> issues.

OK, let me start going through this from my POV, instead of just
responding to your email:

> [Reference tests] are only used when JavaScript tests are insufficient, such as when testing paint code

Do we mean to encourage testing layout things with CSSOM? (e.g.,
checking a box in positioned in the right place and right size?) This
is /mostly/ the status quo, but isn't the habit in csswg-test and on
the whole I would expect this to continue when that is merged into
web-platform-tests.

> Dump Render Tree (DRT) Tests output […]

I think it'd be kinda useful to mention *when* you'd want to use such
a test, given they test implementation detail. (i.e., you want to use
them essentially iff you're testing you get a specific render tree,
otherwise your goal is to test you get the right final result)

> The principles below are adapted from Test the Web Forward's Test Format Guidelines and WebKit's Wiki page on Writing good test cases.

FWIW, making the WPT documentation, which is currently on TTWF, saner
and easier to navigate, and likely doing copy editing of it, is on my
to-do list for, uh, this month; if there's anything you think that
should be there, file bugs on the w-p-t repo or email me, plz!

Also, the first bullet points seems not to be getting rendered
correctly, given there appear to be bullet points not rendered within!

> The desire to use modern features must be balanced with the desire for cross-platform tests. Avoid using features that haven't been shipped by other developed major rendering engines (WebKit, Gecko, Edge). When unsure, check caniuse.com.

As mentioned in my prior email, this should probably include avoiding
features we haven't shipped in a stable release to match the WPT
policy (obviously, this doesn't apply to the feature under test!).

> Furthermore, layout tests should include relevant metadata. The specification URL (in <link rel="help">) is almost always relevant, and is incredibly helpful to a developer who needs to understand the test quickly.

I think it's worthwhile to note we've been trying to *reduce* the
amount of metadata required in CSS tests (for one, almost all of it is
now per-resolution optional, though the implementation of making some
of it optional is yet to happen!).

The spec URL is IMO relatively rarely actually useful: in most cases
in web-platform-tests, the directory structure and filename can tell
you just as much as the spec URL can.

Requirement flags are almost entirely unused by anyone (their
practical uses are mostly superseded by -manual or -visual suffixes on
the filename).

Assertions are in the opinion of many better placed as comments close
to what is actually being tested and little value is got out of
putting them in a meta element, even if the CSS WG's official view is
that it's better to have it in a standard machine-readable format (I
think we should ignore the WG and do what *we* think is best, because
the policy is only a recommendation not a requirement, and potentially
in future lobby to change it).

> New reference tests should follow the WPT reftests guidelines.

I think another important point to include is: "Note that references
can be shared between tests; this is strongly encouraged since it
permits optimizations when running tests."

Some fairly simple references can be used by hundreds (or even
thousands!) of tests; one that occurs frequently in the CSS test
suites simply reads: "Test passes if there is a filled green square
and no red." with a 100px green square below—that's something that
most box-sizing tests can use.

Through the sharing of references, it's possible to cut out thousands
of page loads per test run by caching reference renderings.

HTH,

/gsnedders

[Philip Jägenstedt]

Again, maybe it's just me, but I'm not at all concerned about the potential for time wasted due to using ==, and having review nits about it doesn't seem like time well spent.

What I would agree to, is that if the reviewer can spot that a test depends on null==undefined or one of the other conversions, then they can ask for === instead.

>> Truthy values is a different thing and they should be used. Not using them
>> makes the code significantly longer for mostly no actual benefit.
>
>
> Glad we agree on that one :)

The draft recommendation says "prefer === over ==". I did mean to
imply anything about "if(document.doctype)". I think that the JS
truthiness rules are similar enough to other languages that truthiness
is easy to remember.

Do you think I should add wording that explicitly OKs the use of
non-boolean values in contexts that require booleans?

No, that's fine, I just meant that if() allows for certain surprises similar to ==, and that's OK.

[Philip Jägenstedt]

On Thu, Dec 1, 2016 at 5:30 AM Geoffrey Sneddon <m...@gsnedders.com> wrote:

I think another important point to include is: "Note that references
can be shared between tests; this is strongly encouraged since it
permits optimizations when running tests."

Some fairly simple references can be used by hundreds (or even
thousands!) of tests; one that occurs frequently in the CSS test
suites simply reads: "Test passes if there is a filled green square
and no red." with a 100px green square below—that's something that
most box-sizing tests can use.

Through the sharing of references, it's possible to cut out thousands
of page loads per test run by caching reference renderings.

That's pretty significant. Do you know if there's a bug for making that work when running LayoutTests? (I searched but did not find.)

[Dirk Pranke]

I posted https://codereview.chromium.org/2547463003/ which should improve things,

along the lines you and I discussed in #blink yesterday. Comments welcome.

-- Dirk

[Dirk Pranke]

I don't believe there is a bug on file for this. The reference pages tend to be dirt simple to render, so I expect the savings would not

be *that* significant (probably only a couple of percent of the total run time).

We previously also didn't support this because we really only supported references that lived next to the test (i.e., -expected.html)

but we now support references that live elsewhere (and can be shared between tests) as part of tkent's work to support manifest

files in web-platform-tests. We could (and probably should) switch to using manifests outside of web-platform-tests as well, in order

to have one fewer mechanism for doing things.

-- Dirk

[Geoffrey Sneddon]

Right, obviously this doesn't work with the old WebKit-style refs. For
the sake of reference, csswg-test has 11163 reftests and 3911
references, which means if my maths is right you cut out 1-in-3 loads
by sharing refs, which is significant. Nobody seems to remember quite
how big the perf gains others have got are, though!

/g

[Victor Costan]

On Thu, Dec 1, 2016 at 6:17 PM, Dirk Pranke <dpr...@chromium.org> wrote:
> We previously also didn't support this because we really only supported
> references that lived next to the test (i.e., -expected.html)
> but we now support references that live elsewhere (and can be shared between
> tests) as part of tkent's work to support manifest
> files in web-platform-tests. We could (and probably should) switch to using
> manifests outside of web-platform-tests as well, in order
> to have one fewer mechanism for doing things.

Is there a way to have manifests generated from the data in <link
rel="match">? I imagine that an ideal experience would be either to
have them generated by gn as a build step, or have them in the
repository with a presubmit check that makes sure they stay up to
date.

If this is the case, I'd be happy to document the manifest path, and
remove the recommendation to use -expectation naming.

Thank you,
Victor

[Dirk Pranke]

On Fri, Dec 2, 2016 at 9:49 AM, Victor Costan <pwn...@chromium.org> wrote:

On Thu, Dec 1, 2016 at 6:17 PM, Dirk Pranke <dpr...@chromium.org> wrote:
> We previously also didn't support this because we really only supported
> references that lived next to the test (i.e., -expected.html)
> but we now support references that live elsewhere (and can be shared between
> tests) as part of tkent's work to support manifest
> files in web-platform-tests. We could (and probably should) switch to using
> manifests outside of web-platform-tests as well, in order
> to have one fewer mechanism for doing things.

Is there a way to have manifests generated from the data in <link
rel="match">? I imagine that an ideal experience would be either to
have them generated by gn as a build step, or have them in the
repository with a presubmit check that makes sure they stay up to
date.

I don't think most of the non-wpt/csswg tests use <link rel="match"> but I could be wrong.

Regardless, it wouldn't be too hard to write a script to convert things. If we were to 

convert things, I'd just convert them and then drop support for the -expected.html

approach, rather than worry about keeping things in sync via presubmits or whatnot.

-- Dirk

[Ojan Vafai]

Hi Victor,

Thanks for improving our documentation!

That said, was there a discussion about these changes that I missed?

We should have rough agreement on guidelines before we update the documentation. I think a good way to do this would be to have a separate email thread about each change where you start the discussion with the pros/cons. That way, we're deliberate in changes we make and when someone asks why we have a given guideline, we have a place to point them to.

I don't think the changes you've made have broad agreement at the moment. Would you be willing to undo them and go through something like the process above?

Thanks,

Ojan

[Victor Costan]

On Wed, Dec 7, 2016 at 10:40 AM, Ojan Vafai <oj...@chromium.org> wrote:
> Hi Victor,
>
> Thanks for improving our documentation!
>
> That said, was there a discussion about these changes that I missed?
>
> We should have rough agreement on guidelines before we update the
> documentation. I think a good way to do this would be to have a separate
> email thread about each change where you start the discussion with the
> pros/cons. That way, we're deliberate in changes we make and when someone
> asks why we have a given guideline, we have a place to point them to.
>
> I don't think the changes you've made have broad agreement at the moment.
> Would you be willing to undo them and go through something like the process
> above?

This has been discussed off the list, and is now hopefully resolved.

The "Writing Layout Tests" document [1] now only describes the
mechanics involved in writing layout tests and rules for
distinguishing between test types that were inherited from WPT or
WebKit.

The style recommendations (some of which were controversial) now
reside in a "Layout Test Tips" document [2]. This document attempts to
describe bumps that usually occur when working on layout tests, and
suggests a set of defaults for style issues, including the
controversial ones. This document is intended to (also) help people
new to JavaScript or Blink, and I think that some defaults would help
reduce the intimidation associated with contributing to a new project.
Also, I tried to point out the issues where different teams have
different stands, and explain that looking at surrounding code can
help reduce review roundtrips.

In a similar manner, gracefully degrading to manual tests is now
described in the "Layout Tests with Manual Fallback" document [3] as a
technique that can help with test development and maintenance.

If you care about these topics, please take a look at the reviews
documents. Feedback is welcome here or at BlinkOn -- I'll be attending
and I hope to meet everyone there.

Thank you,
Victor

[1] https://chromium.googlesource.com/chromium/src/+/master/docs/testing/writing_layout_tests.md
[2] https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_tests_tips.md
[3] https://chromium.googlesource.com/chromium/src/+/master/docs/testing/layout_tests_with_manual_fallback.md