Issue 2492733003: New documentation on writing LayoutTests.

pwnall

Patchset #1 (id:1) has been deleted

4 years, 1 month ago (2016-11-10 10:16:37 UTC) #1

pwnall

pwnall@chromium.org changed reviewers: + stapark008@gmail.com

4 years, 1 month ago (2016-11-10 10:21:00 UTC) #2

spark

https://codereview.chromium.org/2492733003/diff/20001/docs/testing/layout_test_writing.md File docs/testing/layout_test_writing.md (right): https://codereview.chromium.org/2492733003/diff/20001/docs/testing/layout_test_writing.md#newcode30 docs/testing/layout_test_writing.md:30: should be avoided so it is clear what is ...

4 years, 1 month ago (2016-11-10 11:00:12 UTC) #4

pwnall

spark: Thank you very much for the quick turnaround, and for finding so many mistakes! ...

4 years, 1 month ago (2016-11-10 16:47:14 UTC) #5

jsbell

jsbell@chromium.org changed reviewers: + jsbell@chromium.org

4 years, 1 month ago (2016-11-10 19:32:25 UTC) #6

jsbell

drive-by... https://codereview.chromium.org/2492733003/diff/40001/docs/testing/layout_test_writing.md File docs/testing/layout_test_writing.md (right): https://codereview.chromium.org/2492733003/diff/40001/docs/testing/layout_test_writing.md#newcode5 docs/testing/layout_test_writing.md:5: and we use it to refer to every ...

4 years, 1 month ago (2016-11-10 19:32:26 UTC) #7

pwnall

jsbell: Thank you for the quick turnaround and for the thoughtful feedback! https://codereview.chromium.org/2492733003/diff/40001/docs/testing/layout_test_writing.md File docs/testing/layout_test_writing.md ...

4 years, 1 month ago (2016-11-11 20:51:07 UTC) #8

jsbell: Thank you for the quick turnaround and for the thoughtful feedback!

https://codereview.chromium.org/2492733003/diff/40001/docs/testing/layout_tes...
File docs/testing/layout_test_writing.md (right):

https://codereview.chromium.org/2492733003/diff/40001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:5: and we use it to refer to every test that
is written as an HTML page and lives
On 2016/11/10 19:32:26, jsbell wrote:
> We have some ref tests that are SVG documents, as well.
> 
> (Also, we may start running imported wpt ones that are e.g. test.worker.js
that
> gets an owning page for it auto-generated, but I guess that would be covered
by
> "HTML page" implicitly)

Done.

Added mentions to XHTML and SVG. Let's deal with the .js tests when we get there
:)

https://codereview.chromium.org/2492733003/diff/40001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:109: Below is a skeleton for a JavaScript
test. Note that, in order to follow the
On 2016/11/10 19:32:25, jsbell wrote:
> I think we should give an async_test and promise_test example as well.

Done.

https://codereview.chromium.org/2492733003/diff/40001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:140: Manual tests should minimize the chance
of user error. This implies keeping the
On 2016/11/10 19:32:26, jsbell wrote:
> We may want an inline example here of a "manual" test if we can come up with a
> short one that uses e.g. eventSender.

Done.

https://codereview.chromium.org/2492733003/diff/40001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:149: For example, the most popular
Blink-specific API is `testRunner`, which is
On 2016/11/10 19:32:26, jsbell wrote:
> We maybe want to mention (here or above) that testharness.js-based tests
running
> in Blink implicitly take advantage of testRunner via the Blink-specific
> testharnessreport.js file.

Done.
I added a note.

https://codereview.chromium.org/2492733003/diff/40001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:183: repository, so that we can be notified
when the failure modes change. In these
On 2016/11/10 19:32:26, jsbell wrote:
> maybe: (e.g. a test starts crashing rather than returning incorrect output)

Done.

https://codereview.chromium.org/2492733003/diff/40001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:215: ### Tests that use an HTTP Server
On 2016/11/10 19:32:25, jsbell wrote:
> Should probably have a section about imported tests here that links to the
> relevant docs - those are running w/ wptserve (or will be shortly)

There is a TODO at the end about documenting wptserve. This whole block is
imported as-is from old docs, and I was trying to avoid rewriting it for now.

WDYT? Is it OK to punt rewriting this section, or do I have to address it in
this CL?

https://codereview.chromium.org/2492733003/diff/40001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:218: require tests served via HTTP or HTTPS,
for example relative paths (`src=/foo`)
On 2016/11/10 19:32:26, jsbell wrote:
> absolute paths
> 
> (relative paths work file with file:)

Done.

pwnall

pwnall@chromium.org changed reviewers: + foolip@chromium.org, rbyers@google.com

4 years, 1 month ago (2016-11-11 21:02:17 UTC) #9

pwnall

rbyers@, foolip@: Can you please take a look? Everything except for the HTTP tests section ...

4 years, 1 month ago (2016-11-11 21:02:18 UTC) #10

jsbell

https://codereview.chromium.org/2492733003/diff/40001/docs/testing/layout_test_writing.md File docs/testing/layout_test_writing.md (right): https://codereview.chromium.org/2492733003/diff/40001/docs/testing/layout_test_writing.md#newcode215 docs/testing/layout_test_writing.md:215: ### Tests that use an HTTP Server On 2016/11/11 ...

4 years, 1 month ago (2016-11-14 19:12:22 UTC) #11

pwnall

Description was changed from ========== New documentation on writing LayoutTests. This CL pulls out the ...

4 years, 1 month ago (2016-11-15 17:58:34 UTC) #12

pwnall

rbyers@, foolip@: Ping? Can you please take a look and see if the testing instructions ...

4 years, 1 month ago (2016-11-15 18:12:54 UTC) #13

qyearsley

qyearsley@chromium.org changed reviewers: + qyearsley@chromium.org

4 years, 1 month ago (2016-11-15 18:40:55 UTC) #14

qyearsley

Thanks for writing this -- A few quick questions/comments: https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_test_writing.md File docs/testing/layout_test_writing.md (right): https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_test_writing.md#newcode1 docs/testing/layout_test_writing.md:1: ...

4 years, 1 month ago (2016-11-15 18:40:56 UTC) #15

jeffcarp

jeffcarp@chromium.org changed reviewers: + jeffcarp@chromium.org

4 years, 1 month ago (2016-11-15 19:08:16 UTC) #16

jeffcarp

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_test_writing.md File docs/testing/layout_test_writing.md (right): https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_test_writing.md#newcode208 docs/testing/layout_test_writing.md:208: <meta name="assert" content="Event.isTrusted value under certain situations" /> On ...

4 years, 1 month ago (2016-11-15 19:08:17 UTC) #17

pwnall

qyearsley: Thank you very much for taking a look at this CL! I think it's ...

4 years, 1 month ago (2016-11-16 01:50:39 UTC) #20

qyearsley: Thank you very much for taking a look at this CL! I think it's
significantly better thanks to your feedback!

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
File docs/testing/layout_test_writing.md (right):

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:1: # Writing Layout Tests
On 2016/11/15 18:40:56, qyearsley wrote:
> Is this file just called layout_test_writing.md so that it starts with
> "layout_test"? What about "writing_layout_tests.md"?

Done.
You guessed my intention correctly :) I thought having the files grouped beats
having (more) predictable names, but I'm ok with flipping the name.

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:5: and we use it to refer to every test that
is written as a Web page (HTML, SVG,
On 2016/11/15 18:40:56, qyearsley wrote:
> I notice that Web is capitalized in a few places here, although I think I
> usually see it uncapitalized (e.g. "web page", "the web", "the web platform")
-
> is there a reason why it's capitalized?

Web is capitalized on Wikipedia and NY Times. This makes sense to me, because
Web (the platform and distributed system) vs web (as in spider web) is quite
similar to Android (Google's mobile platform) vs android (as in robot). I'm
happy to change if I'm voted down :)

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:34: true.
On 2016/11/15 18:40:56, qyearsley wrote:
> Could note here that there are two different JS libraries: testharness.js and
> js-test.js, and there are more details below.

I'd rather not. js-test.js is part of our WebKit legacy, and developers only
need to be aware of it when reading existing tests. This document is intended to
help folks write new tests, and I think it only needs to reference js-test.js so
developers don't get confused and think it's an upcoming framework that's not
yet covered by docs.

WDYT?

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:76: the test scenario efficiently. Avoid
depending on edge case behavior of
On 2016/11/15 18:40:56, qyearsley wrote:
> Nit: "efficiently" may be unnecessary here.

I put it in there because I wanted to say that it's OK for a test to take
advantage of an extra feature (e.g., Promises) if that makes it easier to read /
write. I don't want this phrase to read as "don't use features", but as "don't
add dependencies on features gratuitously".

WDYT?

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:114: not apply.
On 2016/11/15 18:40:56, qyearsley wrote:
> Also note there's a doc about layout test style guidelines
> https://www.chromium.org/blink/coding-style/layout-test-style-guidelines,
> although it doesn't give many hard rules, and basically just says to try to be
> consistent.

Thank you for the link! I either completely forgot about that page, or
(unlikely) never saw it. Here is how the that page's content maps to this
document, which is intended to supersede it.

* the testing framework guidelines changed, js-test is strongly discouraged for
new tests
* I added a note about === 
* I added a note about the HTML5 DOCTYPE
* the omission <html>, <head>, <body> is already recommended
* the reference to Google's JavaScript style guide addresses indentation /
braces / quotation / comments
* I added a note to file naming describing snake-case
* I added a section on directory structure, which covers the special directories
* The TestRunner API is already explained in the guide
* I added a note about ServiceWorker style

I think that the proposal here is stricter than the previous style guidelines,
hence the need to get opinions about it. I think that a stricter style is
better, because it saves new contributors from having to guess preferences, and
it should reduce the amount of nits in code reviews.

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:208: <meta name="assert"
content="Event.isTrusted value under certain situations" />
On 2016/11/15 19:08:17, jeffcarp wrote:
> On 2016/11/15 at 18:40:56, qyearsley wrote:
> > I think there's no need to use /> to close meta and link elements if this is
> HTML5 and not XHTML.
> 
> Just to back this up, the Chromium style says not to close single tags:
> https://www.chromium.org/developers/web-development-style-guide#TOC-Body

Thanks for pointing me to that! I added a link to Google's HTML/CSS style guide,
which is referenced by Chromium style. The document already links the JavaScript
style guide.

I'm hesitant to reference the Chromium style guide, because a lot of the text
there refers to WebUI, and doesn't necessarily translate to LayoutTests.

Please let me know if there is anything that you think should be copied over.

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:208: <meta name="assert"
content="Event.isTrusted value under certain situations" />
On 2016/11/15 18:40:56, qyearsley wrote:
> I think there's no need to use /> to close meta and link elements if this is
> HTML5 and not XHTML.

Done.
Thanks so much for catching this! Having examples that disagree with the
guidelines undermined this document quite a bit :'(

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:326: 
On 2016/11/15 18:40:56, qyearsley wrote:
> I've been referring to all -expected.* files as "baselines", regardless of
> whether they're screenshots or JS test output, or render tree dumps. Could
this
> also be called "Text Baselines"?

Done.
Thank you very much for bringing this up! Consistent nomenclature helps keep
everyone in the project on the same page. I was a bit fuzzy on the concept of
baselines, and I understand it better now.

I used "baseline" throughout the write-up. Please let me know if I got anything
wrong.

FWIW, I think I got confused because we use the term "test expectations" (where
we document that a test is expected fail in a certain way), in a way that is
different from how most test frameworks use it. xUnit frameworks have assertions
APIs along the lines of "assert_predicate(expected, result)", and in that
context, "expectation" means the same thing that we mean by "baseline". However,
I'm fine documenting the current state of affairs. This would be hard to change.

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:341: behavior.
On 2016/11/15 18:40:56, qyearsley wrote:
> Although, web platform tests test conformance to a spec (desired behavior) and
> we add text files to document current behavior.

I think this is an exception. WPT tests are imported verbatim from the W3C
repository, so they don't go through our code review process, and they don't
necessarily follow our standards.

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:343: ### js-test tests
On 2016/11/15 18:40:56, qyearsley wrote:
> Possible alternate section title:
>   "JavaScript Tests Using js-test.js" or maybe
>   "Tests that use js-test.js"
> Not sure if either of those are better.

I changed the section heading. Thank you for highlighting it!

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:345: *** promo
On 2016/11/15 18:40:56, qyearsley wrote:
> What is "promo" here?

It's a gittiles-specific thing.
https://gerrit.googlesource.com/gitiles/+/master/Documentation/markdown.md

It is used elsewhere in the docs too.
https://cs.chromium.org/search/?q=%22***+promo%22+file:%5C.md&sq=package:chro...

I was taught about it in this code review: http://crrev.com/2476573006

I sent a CL your way that makes the Gittiles markdown documentation more
discoverable - http://crrev.com/2502163002

foolip

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_test_writing.md File docs/testing/layout_test_writing.md (right): https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_test_writing.md#newcode5 docs/testing/layout_test_writing.md:5: and we use it to refer to every test ...

4 years, 1 month ago (2016-11-16 11:59:53 UTC) #21

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
File docs/testing/layout_test_writing.md (right):

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:5: and we use it to refer to every test that
is written as a Web page (HTML, SVG,
On 2016/11/16 01:50:39, pwnall wrote:
> On 2016/11/15 18:40:56, qyearsley wrote:
> > I notice that Web is capitalized in a few places here, although I think I
> > usually see it uncapitalized (e.g. "web page", "the web", "the web
platform")
> -
> > is there a reason why it's capitalized?
> 
> Web is capitalized on Wikipedia and NY Times. This makes sense to me, because
> Web (the platform and distributed system) vs web (as in spider web) is quite
> similar to Android (Google's mobile platform) vs android (as in robot). I'm
> happy to change if I'm voted down :)

https://www.chromium.org/blink uses lowercase "web". I point this out only
because it's what I prefer, otherwise I'd have found a case that uses uppercase
"Web" :)

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:34: true.
On 2016/11/16 01:50:39, pwnall wrote:
> On 2016/11/15 18:40:56, qyearsley wrote:
> > Could note here that there are two different JS libraries: testharness.js
and
> > js-test.js, and there are more details below.
> 
> I'd rather not. js-test.js is part of our WebKit legacy, and developers only
> need to be aware of it when reading existing tests. This document is intended
to
> help folks write new tests, and I think it only needs to reference js-test.js
so
> developers don't get confused and think it's an upcoming framework that's not
> yet covered by docs.
> 
> WDYT?

This section doesn't mention specifics, but below there's a "All new JavaScript
tests should be written using the testharness.js testing framework" and that
seems sufficient to me.

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:76: the test scenario efficiently. Avoid
depending on edge case behavior of
On 2016/11/16 01:50:39, pwnall wrote:
> On 2016/11/15 18:40:56, qyearsley wrote:
> > Nit: "efficiently" may be unnecessary here.
> 
> I put it in there because I wanted to say that it's OK for a test to take
> advantage of an extra feature (e.g., Promises) if that makes it easier to read
/
> write. I don't want this phrase to read as "don't use features", but as "don't
> add dependencies on features gratuitously".
> 
> WDYT?

Makes sense to me. If you're waiting for many conditions to become true and use
Promise.all to do so, that could be more readable than other bookkeeping, and
would be fine I think.

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:341: behavior.
On 2016/11/16 01:50:39, pwnall wrote:
> On 2016/11/15 18:40:56, qyearsley wrote:
> > Although, web platform tests test conformance to a spec (desired behavior)
and
> > we add text files to document current behavior.
> 
> I think this is an exception. WPT tests are imported verbatim from the W3C
> repository, so they don't go through our code review process, and they don't
> necessarily follow our standards.

I think it is still worth pointing out somehow. As we drive more and more new
tests to web-platform-tests, having failing expectations as you test things not
yet implemented (by Blink, but perhaps others) is going to be common.

foolip

Very nice, thank you for working on this. I left very many comments, let's see ...

4 years, 1 month ago (2016-11-16 13:42:39 UTC) #22

Very nice, thank you for working on this. I left very many comments, let's see
if there's any real disagreement :)

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:18: (WPT). This helps us avoid regressions,
and gives us confidence that we match
To be picky, merely being in WPT doesn't imply much, some tests fail everywhere
or on 3/4 engines. I'd say "and shared tests with other browsers help identify
where interoperability is lacking" or some variation thereof.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:20: 2. When a Blink feature cannot be
tested using the Web Platform, and cannot be
s/Web Platform/WPT/? There will be many things that are definitely part of the
web platform, but which are currently hard to test using WPT. Input automation
is such a case.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:29: There are three broad types of layout
tests, listed in the order of preference.
There's also the dump render tree kind of test. Not sure where to put it in
order of preference, I guess between reftests and pixel tests?

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:66:
[window.onload](https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onload).
FWIW, when setTimeout is used it's often because there is no event, or you want
to check that an event doesn't fire. Maybe copy/move the "Writing reliable
layout tests" link here? It's a great document I think.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:90: * Tests should be written under the
assumption that they will be upstreamed
\o/

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:93: * Tests that use Blink-specific testing
APIs should feature-test for the
This isn't something I've ever asked for in code review and when writing tests
using testharness.js and internals I've just made them fail if run without
internals. To add manual steps might require making a sync test async, and I
personally think the "click here, things should be green" and similar is clutter
in what is actually an automated tests. Perhaps opinions differ here :)

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:105: use `snake-case`, but preserve the
case of any embedded Web Platform names.
s/Web Platform/API/ perhaps.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:109: * Tests must use the UTF-8 **character
encoding**, which should be declared by
Most tests are in ASCII, so I actually ask people to remove <meta charset=utf-8>
when it won't affect the test.

Looks like the HTML/CSS Style Guide below actually asks for <meta
charset="utf-8">. Could we say that it's optional for a self-contained test that
is obviously ASCII and doesn't include anything that's non-ASCII? At least
without a tool to complain about it, I'm not so keen on asking people to add it
in reviews.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:114: [Google's JavaScript Style
Guide](https://google.github.io/styleguide/javascriptguide.xml),
Is there a lint or format tool for this so that people don't have to learn it
nit-by-nit?

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:119: * Modern Web Platform and JavaScript
features should be preferred to legacy
Note that this is can be slightly at odds with upstreaming to WPT. We shouldn't
gratuitously use unrelated features that aren't yet in the stable versions of
all browsers, as that could obscure real problems.

Some judgement will be needed here. Asking test writers to always be in touch
with the state of interop is a bit much, but maybe "Use of new features
available in all major engines is OK. For example, ..."?

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:151: Below is a skeleton for an HTML5
JavaScript test. Note that, in order to follow
Drop "HTML5", just "JavaScript test" seems OK. (https://html.spec.whatwg.org/
has no version any longer.)

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:160:
href="http://www.ecma-international.org/ecma-262/6.0/#sec-terms-and-definitions-boolean-value">
https://tc39.github.io/ecma262/#sec-boolean-literals, although however you put
it this'll be a weird test, because testharness.js itself uses the true literal
to test it, so the tests amount to (true === true) === true, but I guess that's
OK.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:166: test(t => {
Suggest `() => { }` as the Test instance isn't generally used for sync tests.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:168: assert_true(truthy, 'true should be
truthy');
If it failed, this would say "FAIL JavaScript: the true literal assert_true:
true should be truthy expected true got undefined
" so if you rename the variable to value and say 'value' here, it'd be "FAIL
JavaScript: the true literal assert_true: value expected true got undefined"

In other words, if the description is simply a description of the actual value,
the errors make more sense.

However, for very small tests I'd just not have a description, it makes the test
bigger and doesn't help you track down which assertion failed.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:169: }, 'The literal true in a synchronous
test case');
This isn't spelled out above, but I think this is great. IMHO, test titles
should not include "Tests ..." or say anything about the pass condition, but
just say the thing that they are testing. If the spec changes to invert the pass
condition, you'd ideally not have to change the test title.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:174: const truthy = true;
Testing a true literal like this becomes rather strained. Maybe the test could
pretend that it's checking that the true literal doesn't change over time, by
setting a const originalTruth = true before the timeout, and assert_equals(true,
originalTruth) here?

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:177: t.done();
This test will always pass because this is run before the timeout runs, and the
first result is what counts. Suggest instead using t.step_func_done.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:181: promise_test(t => {
() here too since t isn't used.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:184: }).then(truthy => {
s/truthy/value/

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:200: Whenever possible, tests that rely on
Blink-specific testing APIs should also be
Maybe drop the similar bit above and have only this. I don't enjoy writing or
reviewing those manual steps, I guess opinions may differ though.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:216: <title>DOM Events: Event.isTrusted for
UI events</title>
s/DOM Events/DOM/

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:224: <button id="click-me"
type="button">Click Me!</button>
id="click-me" and type="button" is not needed, can use document.querySelector
when there's just one button.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:244: assert_equals(true,
clickEvent.isTrusted,
Expected and actual are reversed here, but just assert_true would work. And no
description at all would be better IMHO :)

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:248: }, 'Click generated by user
interaction');
Omit test description in single-test files with a <title> that works for the
test title.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:261: * It uses relative paths to point to
It's not possible to get this wrong outside of LayoutTests/imported/wpt/ is it?
And there you'll have to use absolute paths, so I'd suggest dropping this point
and figuring out how to make manual testing easier for that case.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:270: * The `<meta name="assert">` describes
the purpose of the entire file.
Can you throw on something like "Only use when it adds information that isn't
already in the title"? I've personally rarely found them useful.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:271: * The `assert_equals` string describes
the expected behavior, not the error.
The expected value (second argument) describes the expected behavior, so the
description need only name the actual value.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:281: * It uses
[Promises](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Promise)
This test I would argue would be smaller and clearer as an async_test:

async_test(t => {
const button = document.querySelector('button');
button.addEventListener('click', t.step_func_done(event => {
assert_true(event.isTrusted);
});
/* stuff with internals */
});

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:345: The baselines are generated
automatically when appropriate by
double space

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:349: Text baselines should be very rare. In
general, layout tests should use
As noted above, this'll be at odds with the desire to upstream more tests. If
writing with upstreaming in mind, I'd argue that we should test spec behavior
and live with the expected files, taking care to see that failures don't cascade
and hide other problems.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:414: WebKit-style reftests are deprecated.
Please use the WPT style for all new
Right now this isn't possible, and the WPT style is converted to *-expected.html
on import. So I guess a TODO to update this documentation when we have that
sorted.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:452: <script
src="/resources/ahem.js"></script>
Looks like this is always included using a relative path.

qyearsley

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_test_writing.md File docs/testing/layout_test_writing.md (right): https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_test_writing.md#newcode5 docs/testing/layout_test_writing.md:5: and we use it to refer to every test ...

4 years, 1 month ago (2016-11-16 18:35:45 UTC) #23

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
File docs/testing/layout_test_writing.md (right):

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:5: and we use it to refer to every test that
is written as a Web page (HTML, SVG,
On 2016/11/16 at 11:59:53, foolip wrote:
> On 2016/11/16 01:50:39, pwnall wrote:
> > On 2016/11/15 18:40:56, qyearsley wrote:
> > > I notice that Web is capitalized in a few places here, although I think I
> > > usually see it uncapitalized (e.g. "web page", "the web", "the web
platform")
> > -
> > > is there a reason why it's capitalized?
> > 
> > Web is capitalized on Wikipedia and NY Times. This makes sense to me,
because
> > Web (the platform and distributed system) vs web (as in spider web) is quite
> > similar to Android (Google's mobile platform) vs android (as in robot). I'm
> > happy to change if I'm voted down :)
> 
> https://www.chromium.org/blink uses lowercase "web". I point this out only
because it's what I prefer, otherwise I'd have found a case that uses uppercase
"Web" :)

For some reason I never realized that it was sometimes capitalized when
referring to "the Web", even though I know that "the Internet" is often
capitalized (and "the Internet" is different from "an internet"). Anyway, both
are fine with me.

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:34: true.
On 2016/11/16 at 11:59:53, foolip wrote:
> On 2016/11/16 01:50:39, pwnall wrote:
> > On 2016/11/15 18:40:56, qyearsley wrote:
> > > Could note here that there are two different JS libraries: testharness.js
and
> > > js-test.js, and there are more details below.
> > 
> > I'd rather not. js-test.js is part of our WebKit legacy, and developers only
> > need to be aware of it when reading existing tests. This document is
intended to
> > help folks write new tests, and I think it only needs to reference
js-test.js so
> > developers don't get confused and think it's an upcoming framework that's
not
> > yet covered by docs.
> > 
> > WDYT?
> 
> This section doesn't mention specifics, but below there's a "All new
JavaScript tests should be written using the testharness.js testing framework"
and that seems sufficient to me.

Yep, sounds good to me.

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:76: the test scenario efficiently. Avoid
depending on edge case behavior of
On 2016/11/16 at 01:50:39, pwnall wrote:
> On 2016/11/15 18:40:56, qyearsley wrote:
> > Nit: "efficiently" may be unnecessary here.
> 
> I put it in there because I wanted to say that it's OK for a test to take
advantage of an extra feature (e.g., Promises) if that makes it easier to read /
write. I don't want this phrase to read as "don't use features", but as "don't
add dependencies on features gratuitously".
> 
> WDYT?

Sounds good. This is nitpicky, but I'm still not sure whether there's a better
word. "Efficiently" maybe sounds like it implies "using as little time or space
as possible", although maybe what's meant is more like "concisely"?

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:326: 
On 2016/11/16 at 01:50:39, pwnall wrote:
> On 2016/11/15 18:40:56, qyearsley wrote:
> > I've been referring to all -expected.* files as "baselines", regardless of
> > whether they're screenshots or JS test output, or render tree dumps. Could
this
> > also be called "Text Baselines"?
> 
> Done.
> Thank you very much for bringing this up! Consistent nomenclature helps keep
everyone in the project on the same page. I was a bit fuzzy on the concept of
baselines, and I understand it better now.
> 
> I used "baseline" throughout the write-up. Please let me know if I got
anything wrong.
> 
> FWIW, I think I got confused because we use the term "test expectations"
(where we document that a test is expected fail in a certain way), in a way that
is different from how most test frameworks use it. xUnit frameworks have
assertions APIs along the lines of "assert_predicate(expected, result)", and in
that context, "expectation" means the same thing that we mean by "baseline".
However, I'm fine documenting the current state of affairs. This would be hard
to change.

Yeah, I think that the current state of affairs is: "test expectations" is
ambiguous and usually refers to either -expected.* files or entries in
TestExpectations; both of these things work together to define what the
"expected" result of the test is. The less ambiguous way to refer to these is
"baseline files" and "test expectations lines" respectively, I think.

Specifically, if it's expected to crash or timeout, then that's put in
TestExpectations -- and the expected actual result is specified by the baseline.

This just starts to get confusing in a couple cases:
 - For lines with [ Failure ] in TestExpectations, the test "passes" (exit code
0) if it "fails" (mismatch with baseline or reference).
 - For baselines with "FAIL", the test "passes" if one of the individual asserts
fails (as long as the test doesn't have a [ Failure ] expectation in
TestExpectations...)

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:345: *** promo
On 2016/11/16 at 01:50:39, pwnall wrote:
> On 2016/11/15 18:40:56, qyearsley wrote:
> > What is "promo" here?
> 
> It's a gittiles-specific thing.
> https://gerrit.googlesource.com/gitiles/+/master/Documentation/markdown.md
> 
> It is used elsewhere in the docs too.
>
https://cs.chromium.org/search/?q=%22***+promo%22+file:%5C.md&sq=package:chro...
> 
> I was taught about it in this code review: http://crrev.com/2476573006
> 
> I sent a CL your way that makes the Gittiles markdown documentation more
discoverable - http://crrev.com/2502163002

Ah, thanks

pwnall

foolip@: Thank you very much for the thorough review! I hope I've addressed most of ...

4 years, 1 month ago (2016-11-18 05:40:44 UTC) #24

foolip@: Thank you very much for the thorough review! I hope I've addressed most
of your feedback, and I have a few questions left.

Can you please take another look?

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:18: (WPT). This helps us avoid regressions,
and gives us confidence that we match
On 2016/11/16 13:42:38, foolip wrote:
> To be picky, merely being in WPT doesn't imply much, some tests fail
everywhere
> or on 3/4 engines. I'd say "and shared tests with other browsers help identify
> where interoperability is lacking" or some variation thereof.

Done.
I tried to rephrase. Does this work for you?

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:20: 2. When a Blink feature cannot be
tested using the Web Platform, and cannot be
On 2016/11/16 13:42:39, foolip wrote:
> s/Web Platform/WPT/? There will be many things that are definitely part of the
> web platform, but which are currently hard to test using WPT. Input automation
> is such a case.

Wouldn't you say these are cases where the Web Platform doesn't provide ways to
test the APIs? For things like input automation, I imagine that WPT might one
day have a WebDriver interface (JS that XHRs to the WPT server, which controls a
WebDriver client running the browser that has the test page open) that resembles
our eventSender, and I imagine that we'll still want to encourage folks to write
tests that stick to Web Platform features (and don't use the Web Driver stuff)
whenever possible.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:29: There are three broad types of layout
tests, listed in the order of preference.
On 2016/11/16 13:42:38, foolip wrote:
> There's also the dump render tree kind of test. Not sure where to put it in
> order of preference, I guess between reftests and pixel tests?

I have no idea what I'm talking about, because I haven't worked on layout yet...
but isn't it possible to use JavaScript to get (and validate) information that
is equivalent to the DRT output?

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:66:
[window.onload](https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onload).
On 2016/11/16 13:42:39, foolip wrote:
> FWIW, when setTimeout is used it's often because there is no event, or you
want
> to check that an event doesn't fire. Maybe copy/move the "Writing reliable
> layout tests" link here? It's a great document I think.

Done.
Ideally, the document should be turned into a Markdown file in the docs/ folder,
or should be absorbed in here. The Google Doc is more difficult to update in a
collaborative fashion. That is definitely future work.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:90: * Tests should be written under the
assumption that they will be upstreamed
On 2016/11/16 13:42:39, foolip wrote:
> \o/

Agreed!

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:93: * Tests that use Blink-specific testing
APIs should feature-test for the
On 2016/11/16 13:42:38, foolip wrote:
> This isn't something I've ever asked for in code review and when writing tests
> using testharness.js and internals I've just made them fail if run without
> internals. To add manual steps might require making a sync test async, and I
> personally think the "click here, things should be green" and similar is
clutter
> in what is actually an automated tests. Perhaps opinions differ here :)

I was definitely asked to do this in (perhaps too old?) code reviews... that's
how I learned the technique :)

I think that the manual test aspect is helpful for debugging and for quickly
checking if we match other browsers. Given my past experience, I think we should
document whatever the outcome of this decision is -- if we decide we don't want
manual tests, I'd like to include notes/promos in the relevant places stating
that manual tests used to be a thing but are no longer mandatory.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:105: use `snake-case`, but preserve the
case of any embedded Web Platform names.
On 2016/11/16 13:42:39, foolip wrote:
> s/Web Platform/API/ perhaps.

Done.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:109: * Tests must use the UTF-8 **character
encoding**, which should be declared by
On 2016/11/16 13:42:38, foolip wrote:
> Most tests are in ASCII, so I actually ask people to remove <meta
charset=utf-8>
> when it won't affect the test.
> 
> Looks like the HTML/CSS Style Guide below actually asks for <meta
> charset="utf-8">. Could we say that it's optional for a self-contained test
that
> is obviously ASCII and doesn't include anything that's non-ASCII? At least
> without a tool to complain about it, I'm not so keen on asking people to add
it
> in reviews.

Firefox Nightly now issues a warning when a page doesn't have the charset
specified. This was my motivation for removing the pure ASCII exception that was
in a previous version of this CL.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:114: [Google's JavaScript Style
Guide](https://google.github.io/styleguide/javascriptguide.xml),
On 2016/11/16 13:42:38, foolip wrote:
> Is there a lint or format tool for this so that people don't have to learn it
> nit-by-nit?

AFAIK that is not yet the case, but I hear that clang-format is getting better
at JavaScript?

I still think there's value to us agreeing on a format before we have the tools
to enforce it. I'd much rather have a guide I can read (and people to help me),
as opposed to loose rules that leave me guessing what my reviewer would like to
see.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:119: * Modern Web Platform and JavaScript
features should be preferred to legacy
On 2016/11/16 13:42:39, foolip wrote:
> Note that this is can be slightly at odds with upstreaming to WPT. We
shouldn't
> gratuitously use unrelated features that aren't yet in the stable versions of
> all browsers, as that could obscure real problems.
> 
> Some judgement will be needed here. Asking test writers to always be in touch
> with the state of interop is a bit much, but maybe "Use of new features
> available in all major engines is OK. For example, ..."?

Done.
Thank you for reminding me about this balancing act! I pulled the idea to its
own principle and populated it a bit more.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:151: Below is a skeleton for an HTML5
JavaScript test. Note that, in order to follow
On 2016/11/16 13:42:39, foolip wrote:
> Drop "HTML5", just "JavaScript test" seems OK. (https://html.spec.whatwg.org/
> has no version any longer.)

Done.
I rephrased to JavaScript test embedded in HTML page. I wanted to make it clear
(for readers who care) that this isn't a valid skeleton for an XHTML page.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:160:
href="http://www.ecma-international.org/ecma-262/6.0/#sec-terms-and-definitions-boolean-value">
On 2016/11/16 13:42:39, foolip wrote:
> https://tc39.github.io/ecma262/#sec-boolean-literals, although however you put
> it this'll be a weird test, because testharness.js itself uses the true
literal
> to test it, so the tests amount to (true === true) === true, but I guess
that's
> OK.

Done.
Thank you very much for the shorter link!

I agree with the weirdness. FWIW, I upgraded an old example, because it seemed
minimal. Do you have any idea for what other concept would be better here?
Something like 1 + 1 === 2?

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:166: test(t => {
On 2016/11/16 13:42:39, foolip wrote:
> Suggest `() => { }` as the Test instance isn't generally used for sync tests.

Done.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:168: assert_true(truthy, 'true should be
truthy');
On 2016/11/16 13:42:39, foolip wrote:
> If it failed, this would say "FAIL JavaScript: the true literal assert_true:
> true should be truthy expected true got undefined
> " so if you rename the variable to value and say 'value' here, it'd be "FAIL
> JavaScript: the true literal assert_true: value expected true got undefined"
> 
> In other words, if the description is simply a description of the actual
value,
> the errors make more sense.
> 
> However, for very small tests I'd just not have a description, it makes the
test
> bigger and doesn't help you track down which assertion failed.

Done.
I changed the tests to demonstrate the rule you suggested (no description for
single-assertion tests, description for multi-assertion tests).

I also added an assert_equal(!value, false) because (1) I wanted to show the
actual/expected argument order, and (2) I wanted to have a test where it makes
sense to have assertion descriptions.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:169: }, 'The literal true in a synchronous
test case');
On 2016/11/16 13:42:39, foolip wrote:
> This isn't spelled out above, but I think this is great. IMHO, test titles
> should not include "Tests ..." or say anything about the pass condition, but
> just say the thing that they are testing. If the spec changes to invert the
pass
> condition, you'd ideally not have to change the test title.

I created a bulleted list below this block of code, where I tried to spell out
this idea, as well as other things that I think aren't obvious on a first read,
but need to be learned to write tests effectively.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:174: const truthy = true;
On 2016/11/16 13:42:38, foolip wrote:
> Testing a true literal like this becomes rather strained. Maybe the test could
> pretend that it's checking that the true literal doesn't change over time, by
> setting a const originalTruth = true before the timeout, and
assert_equals(true,
> originalTruth) here?

Done.
This does look better. Thank you!

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:177: t.done();
On 2016/11/16 13:42:39, foolip wrote:
> This test will always pass because this is run before the timeout runs, and
the
> first result is what counts. Suggest instead using t.step_func_done.

Done. (pun not intended)
Wow, that error was pretty terrible! Thanks so much for catching it! I know how
to JS, I promise :)
I added a comment that introduces step_func() and done(), so folks don't blindly
build on the example and end up with multiple step_func_done() in a test.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:181: promise_test(t => {
On 2016/11/16 13:42:39, foolip wrote:
> () here too since t isn't used.

Done.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:184: }).then(truthy => {
On 2016/11/16 13:42:39, foolip wrote:
> s/truthy/value/

Done.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:200: Whenever possible, tests that rely on
Blink-specific testing APIs should also be
On 2016/11/16 13:42:38, foolip wrote:
> Maybe drop the similar bit above and have only this. I don't enjoy writing or
> reviewing those manual steps, I guess opinions may differ though.

Ack.
I will watch out for more opinions on the manual tests requirement.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:216: <title>DOM Events: Event.isTrusted for
UI events</title>
On 2016/11/16 13:42:39, foolip wrote:
> s/DOM Events/DOM/

Done.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:224: <button id="click-me"
type="button">Click Me!</button>
On 2016/11/16 13:42:39, foolip wrote:
> id="click-me" and type="button" is not needed, can use document.querySelector
> when there's just one button.

Done.
FWIW, I'm a bit wary of using querySelector, because I got super-excited when it
shipped and I'm afraid that I'm biased and querySelector ALL THE THINGS :)

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:244: assert_equals(true,
clickEvent.isTrusted,
On 2016/11/16 13:42:39, foolip wrote:
> Expected and actual are reversed here, but just assert_true would work. And no
> description at all would be better IMHO :)

Done.
Wow, I failed at a point that I was trying to make. I now use the correct order
(I hope) above, and document the order explicitly in a bullet point.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:248: }, 'Click generated by user
interaction');
On 2016/11/16 13:42:39, foolip wrote:
> Omit test description in single-test files with a <title> that works for the
> test title.

I also made this explicit in a new bulleted list above.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:261: * It uses relative paths to point to
On 2016/11/16 13:42:39, foolip wrote:
> It's not possible to get this wrong outside of LayoutTests/imported/wpt/ is
it?
> And there you'll have to use absolute paths, so I'd suggest dropping this
point
> and figuring out how to make manual testing easier for that case.

Done.
I described relative paths as a limitation of our test runner. I don't have
enough cycles to work on infrastructure improvements, I can only document the
current state. I was advised to write this from the perspective of where we'd
want to be in a year, and I think I overshot by a bit :)

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:270: * The `<meta name="assert">` describes
the purpose of the entire file.
On 2016/11/16 13:42:39, foolip wrote:
> Can you throw on something like "Only use when it adds information that isn't
> already in the title"? I've personally rarely found them useful.

Done.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:271: * The `assert_equals` string describes
the expected behavior, not the error.
On 2016/11/16 13:42:39, foolip wrote:
> The expected value (second argument) describes the expected behavior, so the
> description need only name the actual value.

Done.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:281: * It uses
[Promises](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Promise)
On 2016/11/16 13:42:39, foolip wrote:
> This test I would argue would be smaller and clearer as an async_test:
> 
> async_test(t => {
>   const button = document.querySelector('button');
>   button.addEventListener('click', t.step_func_done(event => {
>     assert_true(event.isTrusted);
>   });
>   /* stuff with internals */
> });

I agree. I still think that it's more valuable to teach how this works with
Promises, because they're easier to compose and reason about.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:345: The baselines  are generated
automatically when appropriate by
On 2016/11/16 13:42:39, foolip wrote:
> double space

Done.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:349: Text baselines should be very rare. In
general, layout tests should use
On 2016/11/16 13:42:38, foolip wrote:
> As noted above, this'll be at odds with the desire to upstream more tests. If
> writing with upstreaming in mind, I'd argue that we should test spec behavior
> and live with the expected files, taking care to see that failures don't
cascade
> and hide other problems.

Done.
I rewrote this paragraph. It now advises for text baselines (+crbug.com issues
tracking their removal) for WPT tests, and current behavior tests for non-WPT
tests.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:414: WebKit-style reftests are deprecated.
Please use the WPT style for all new
On 2016/11/16 13:42:39, foolip wrote:
> Right now this isn't possible, and the WPT style is converted to
*-expected.html
> on import. So I guess a TODO to update this documentation when we have that
> sorted.

Done.
I have been trying to avoid writing this section, because I have zero experience
with reference tests. I rephrased the section to reflect the current reality and
state where we aim to be in the near (< 1 year?) future.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:452: <script
src="/resources/ahem.js"></script>
On 2016/11/16 13:42:38, foolip wrote:
> Looks like this is always included using a relative path.

Done.
I added a note about this.

foolip

LGTM % a few more nits, and there are a few points which I think ...

4 years, 1 month ago (2016-11-18 11:30:40 UTC) #25

LGTM % a few more nits, and there are a few points which I think warrant some
wider input. This is really great documentation, thanks for taking the time!

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:18: (WPT). This helps us avoid regressions,
and gives us confidence that we match
On 2016/11/18 05:40:43, pwnall wrote:
> On 2016/11/16 13:42:38, foolip wrote:
> > To be picky, merely being in WPT doesn't imply much, some tests fail
> everywhere
> > or on 3/4 engines. I'd say "and shared tests with other browsers help
identify
> > where interoperability is lacking" or some variation thereof.
> 
> Done.
> I tried to rephrase. Does this work for you?

Wonderful!

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:20: 2. When a Blink feature cannot be
tested using the Web Platform, and cannot be
On 2016/11/18 05:40:43, pwnall wrote:
> On 2016/11/16 13:42:39, foolip wrote:
> > s/Web Platform/WPT/? There will be many things that are definitely part of
the
> > web platform, but which are currently hard to test using WPT. Input
automation
> > is such a case.
> 
> Wouldn't you say these are cases where the Web Platform doesn't provide ways
to
> test the APIs? For things like input automation, I imagine that WPT might one
> day have a WebDriver interface (JS that XHRs to the WPT server, which controls
a
> WebDriver client running the browser that has the test page open) that
resembles
> our eventSender, and I imagine that we'll still want to encourage folks to
write
> tests that stick to Web Platform features (and don't use the Web Driver stuff)
> whenever possible.

Yes, that's precisely where I think we must go, to standardize enough testing
APIs (as part of WebDriver or whatever makes sense) so that more and more things
can be part of web-platform-tests.

By s/Web Platform/WPT/ I mean that being in WPT isn't synonymous with being
automatable using only on web exposed APIs. The question to ask, I think, is
"can I put this in WPT", and the answer is already yes in a few cases using
wpt_automation.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:29: There are three broad types of layout
tests, listed in the order of preference.
On 2016/11/18 05:40:43, pwnall wrote:
> On 2016/11/16 13:42:38, foolip wrote:
> > There's also the dump render tree kind of test. Not sure where to put it in
> > order of preference, I guess between reftests and pixel tests?
> 
> I have no idea what I'm talking about, because I haven't worked on layout
yet...
> but isn't it possible to use JavaScript to get (and validate) information that
> is equivalent to the DRT output?

Not entirely I think, DRT also includes pseudo elements and maybe other things
that aren't web exposed. Whatever the case, there's a lot of these tests. Maybe
ask eae@ for a blurb describing them?

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:66:
[window.onload](https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onload).
On 2016/11/18 05:40:43, pwnall wrote:
> On 2016/11/16 13:42:39, foolip wrote:
> > FWIW, when setTimeout is used it's often because there is no event, or you
> want
> > to check that an event doesn't fire. Maybe copy/move the "Writing reliable
> > layout tests" link here? It's a great document I think.
> 
> Done.
> Ideally, the document should be turned into a Markdown file in the docs/
folder,
> or should be absorbed in here. The Google Doc is more difficult to update in a
> collaborative fashion. That is definitely future work.

Yep, that'd be great, some day.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:93: * Tests that use Blink-specific testing
APIs should feature-test for the
On 2016/11/18 05:40:44, pwnall wrote:
> On 2016/11/16 13:42:38, foolip wrote:
> > This isn't something I've ever asked for in code review and when writing
tests
> > using testharness.js and internals I've just made them fail if run without
> > internals. To add manual steps might require making a sync test async, and I
> > personally think the "click here, things should be green" and similar is
> clutter
> > in what is actually an automated tests. Perhaps opinions differ here :)
> 
> I was definitely asked to do this in (perhaps too old?) code reviews... that's
> how I learned the technique :)
> 
> I think that the manual test aspect is helpful for debugging and for quickly
> checking if we match other browsers. Given my past experience, I think we
should
> document whatever the outcome of this decision is -- if we decide we don't
want
> manual tests, I'd like to include notes/promos in the relevant places stating
> that manual tests used to be a thing but are no longer mandatory.

I guess consult blink-dev on this question? Leaving it to CL author and reviewer
judgement seems like it might work.

Note that there are plenty APIs in Internals.idl where no user action could do
the same thing, input automation might be the exception rather than the rule.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:109: * Tests must use the UTF-8 **character
encoding**, which should be declared by
On 2016/11/18 05:40:43, pwnall wrote:
> On 2016/11/16 13:42:38, foolip wrote:
> > Most tests are in ASCII, so I actually ask people to remove <meta
> charset=utf-8>
> > when it won't affect the test.
> > 
> > Looks like the HTML/CSS Style Guide below actually asks for <meta
> > charset="utf-8">. Could we say that it's optional for a self-contained test
> that
> > is obviously ASCII and doesn't include anything that's non-ASCII? At least
> > without a tool to complain about it, I'm not so keen on asking people to add
> it
> > in reviews.
> 
> Firefox Nightly now issues a warning when a page doesn't have the charset
> specified. This was my motivation for removing the pure ASCII exception that
was
> in a previous version of this CL.

I think I'd still like the exception, but again perhaps seek wider feedback.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:114: [Google's JavaScript Style
Guide](https://google.github.io/styleguide/javascriptguide.xml),
On 2016/11/18 05:40:44, pwnall wrote:
> On 2016/11/16 13:42:38, foolip wrote:
> > Is there a lint or format tool for this so that people don't have to learn
it
> > nit-by-nit?
> 
> AFAIK that is not yet the case, but I hear that clang-format is getting better
> at JavaScript?
> 
> I still think there's value to us agreeing on a format before we have the
tools
> to enforce it. I'd much rather have a guide I can read (and people to help
me),
> as opposed to loose rules that leave me guessing what my reviewer would like
to
> see.

Acknowledged.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:119: * Modern Web Platform and JavaScript
features should be preferred to legacy
On 2016/11/18 05:40:43, pwnall wrote:
> On 2016/11/16 13:42:39, foolip wrote:
> > Note that this is can be slightly at odds with upstreaming to WPT. We
> shouldn't
> > gratuitously use unrelated features that aren't yet in the stable versions
of
> > all browsers, as that could obscure real problems.
> > 
> > Some judgement will be needed here. Asking test writers to always be in
touch
> > with the state of interop is a bit much, but maybe "Use of new features
> > available in all major engines is OK. For example, ..."?
> 
> Done.
> Thank you for reminding me about this balancing act! I pulled the idea to its
> own principle and populated it a bit more.

Great, these and the above changes get the message across clearly I think.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:160:
href="http://www.ecma-international.org/ecma-262/6.0/#sec-terms-and-definitions-boolean-value">
On 2016/11/18 05:40:43, pwnall wrote:
> On 2016/11/16 13:42:39, foolip wrote:
> > https://tc39.github.io/ecma262/#sec-boolean-literals, although however you
put
> > it this'll be a weird test, because testharness.js itself uses the true
> literal
> > to test it, so the tests amount to (true === true) === true, but I guess
> that's
> > OK.
> 
> Done.
> Thank you very much for the shorter link!
> 
> I agree with the weirdness. FWIW, I upgraded an old example, because it seemed
> minimal. Do you have any idea for what other concept would be better here?
> Something like 1 + 1 === 2?

Let's stick with this, presumably literals are already tested somehow, so people
won't copy this to create real tests for the true literal :)

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:177: t.done();
On 2016/11/18 05:40:43, pwnall wrote:
> On 2016/11/16 13:42:39, foolip wrote:
> > This test will always pass because this is run before the timeout runs, and
> the
> > first result is what counts. Suggest instead using t.step_func_done.
> 
> Done. (pun not intended)
> Wow, that error was pretty terrible! Thanks so much for catching it! I know
how
> to JS, I promise :)
> I added a comment that introduces step_func() and done(), so folks don't
blindly
> build on the example and end up with multiple step_func_done() in a test.

I totally believe that your promise resolves to true!

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:224: <button id="click-me"
type="button">Click Me!</button>
On 2016/11/18 05:40:43, pwnall wrote:
> On 2016/11/16 13:42:39, foolip wrote:
> > id="click-me" and type="button" is not needed, can use
document.querySelector
> > when there's just one button.
> 
> Done.
> FWIW, I'm a bit wary of using querySelector, because I got super-excited when
it
> shipped and I'm afraid that I'm biased and querySelector ALL THE THINGS :)

Well, I have that bias too, I even use querySelectorAll if it means I can avoid
id="button1" and id="button2" :)

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:244: assert_equals(true,
clickEvent.isTrusted,
On 2016/11/18 05:40:43, pwnall wrote:
> On 2016/11/16 13:42:39, foolip wrote:
> > Expected and actual are reversed here, but just assert_true would work. And
no
> > description at all would be better IMHO :)
> 
> Done.
> Wow, I failed at a point that I was trying to make. I now use the correct
order
> (I hope) above, and document the order explicitly in a bullet point.

It's very good to have clear documentation on this, the historical note at the
end of
https://github.com/google/googletest/blob/master/googletest/docs/Primer.md#bi...
is probably why the other order comes naturally to many Chromium developers.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:261: * It uses relative paths to point to
On 2016/11/18 05:40:43, pwnall wrote:
> On 2016/11/16 13:42:39, foolip wrote:
> > It's not possible to get this wrong outside of LayoutTests/imported/wpt/ is
> it?
> > And there you'll have to use absolute paths, so I'd suggest dropping this
> point
> > and figuring out how to make manual testing easier for that case.
> 
> Done.
> I described relative paths as a limitation of our test runner. I don't have
> enough cycles to work on infrastructure improvements, I can only document the
> current state. I was advised to write this from the perspective of where we'd
> want to be in a year, and I think I overshot by a bit :)

:)

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:281: * It uses
[Promises](https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Promise)
On 2016/11/18 05:40:44, pwnall wrote:
> On 2016/11/16 13:42:39, foolip wrote:
> > This test I would argue would be smaller and clearer as an async_test:
> > 
> > async_test(t => {
> >   const button = document.querySelector('button');
> >   button.addEventListener('click', t.step_func_done(event => {
> >     assert_true(event.isTrusted);
> >   });
> >   /* stuff with internals */
> > });
> 
> I agree. I still think that it's more valuable to teach how this works with
> Promises, because they're easier to compose and reason about.

OK, I'll take both styles! One benefit of promise_test is a lot less need for
t.step_func wrapping.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:349: Text baselines should be very rare. In
general, layout tests should use
On 2016/11/18 05:40:43, pwnall wrote:
> On 2016/11/16 13:42:38, foolip wrote:
> > As noted above, this'll be at odds with the desire to upstream more tests.
If
> > writing with upstreaming in mind, I'd argue that we should test spec
behavior
> > and live with the expected files, taking care to see that failures don't
> cascade
> > and hide other problems.
> 
> Done.
> I rewrote this paragraph. It now advises for text baselines (+crbug.com issues
> tracking their removal) for WPT tests, and current behavior tests for non-WPT
> tests. 

Acknowledged.

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:199: assert_equal(!value, false, 'the
logical negtion of true');
I guess assert_false here, since you'll have a chance to show off assert_equals
below.

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:207: assert_true(originallyTrue);
assert_equals(value, orginallyTrue), or maybe the reverse, but something
actually using the value const :)

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:241: * Do not start test case descriptions
with redundant terms like "Testing "
This whole block is great! Nit here is extra space in "Testing " (my fault
probably)

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:252: * Test case assertions must be wrapped
in `t.step_func()` calls, so that
Callbacks that don't have assertions but could plausibly throw exceptions must
also be wrapped, or it results in an harness error I believe.

There's also some magic around a single async test in a file when t.step_func
wrapping isn't needed. I've never depended on it, and I guess wouldn't encourage
it, but accept it in review.

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:344: * The `assert_equals` string describes
the way the actual value was
The above test doesn't have assert_equals any more. This is quite well described
above so maybe this bit isn't needed?

foolip

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_layout_tests.md File docs/testing/writing_layout_tests.md (right): https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_layout_tests.md#newcode496 docs/testing/writing_layout_tests.md:496: Chromium's testing infrastructure does not support WPT reftests. In ...

4 years, 1 month ago (2016-11-18 11:31:42 UTC) #26

qyearsley

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_layout_tests.md File docs/testing/writing_layout_tests.md (right): https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_layout_tests.md#newcode496 docs/testing/writing_layout_tests.md:496: Chromium's testing infrastructure does not support WPT reftests. In ...

4 years, 1 month ago (2016-11-18 18:02:08 UTC) #27

foolip

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_layout_tests.md File docs/testing/writing_layout_tests.md (right): https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_layout_tests.md#newcode496 docs/testing/writing_layout_tests.md:496: Chromium's testing infrastructure does not support WPT reftests. In ...

4 years, 1 month ago (2016-11-21 09:45:02 UTC) #29

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:496: Chromium's testing infrastructure does
not support WPT reftests. In the
On 2016/11/18 18:02:08, qyearsley wrote:
> On 2016/11/18 at 11:31:42, foolip wrote:
> > qyearsley@, I think this maybe doesn't quite capture it, can you suggest
> wording?
> 
> Yeah, rather than saying that we "don't support WPT reftests", it's more
> accurate to say that we currently do extra work when importing to rename
> reference files to fit our naming convention. This is only needed so that the
> test runner finds the reference files.
> 
> There was just some related discussion of this in
> https://bugs.chromium.org/p/chromium/issues/detail?id=268729#c19: It sounds
like
> the preferred plan for supporting WPT reftests is to find the reference files
by
> reading from a generated manifest, but that manifest is only going to be in
the
> web-platform-tests directory.
> 
> So, maybe for the foreseeable future we still want to use the "legacy" naming
> convention -expected.html for new reference tests that are not part of WPT,
and
> the rule for using <link rel="match" href="..."> only applies to new tests
that
> will be part of WPT?

Keeping -expected.html outside of WPT sounds good to me, presumably it'd be
hopeless to convert all of them to the other style, and at least -expected.html
is easy to understand.

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:509: and an `-expected.html` suffix.
On 2016/11/18 18:02:08, qyearsley wrote:
> Not sure if it's worth mentioning that it's also possible to name files as
> -expected-mismatch.html, and then the test will pass if there's a mismatch.
> Also, the extension doesn't have to be .html; it can also be htm, svg, xht, or
> xhtml.

+1, this can be powerful, and the first time I wanted to use it I had a bit of
trouble figuring out how, basically I went searching for file name patterns and
found a case.

Rick Byers

rbyers@chromium.org changed reviewers: + rbyers@chromium.org - rbyers@google.com

4 years, 1 month ago (2016-11-21 19:14:22 UTC) #30

Rick Byers

Sorry for the long delay in reviewing this! I missed it in my daily codereview ...

4 years, 1 month ago (2016-11-21 22:37:15 UTC) #31

Sorry for the long delay in reviewing this!  I missed it in my daily codereview
checks because you had rbyers@google.com as the reviewer (instead of my
chromium.org account).  I thought I had a nickname specified to make that
obvious but looks like that got reset or I deleted it - fixed now.

Thanks a ton for working on this!  It looks great overall to me, just a couple
small suggestions.

If you want a wider audience before landing / posting to blink-dev then you
might want to reach out to someone (eg. haraken, jbroman, esprehn) on the blink
architecture team as the stewards of overall blink code health.  I suspect
they'd say they want some docs encouraging more unit testing, but that's
presumably orthogonal.  Maybe they'd at least have some guidance/docs for you to
link to from here?

https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:17: [Web Platform Tests
Project](https://github.com/w3c/web-platform-tests)
nit: there's technically more than just WPT (eg. test262 for things defined in
the Ecmascript specification, and csswg-test for CSS stuff - though that's
hopefully going to be merged into web-platform-test before too long).  Maybe
just say "covered by cross-browser tests such as the Web Platform Tests
Project"?

https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:19: areas where the major browsers don't
have interoperable implementations.
also mention that this lowers the cost of testing by sharing work across all the
major engines?

https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:39: such as when testing layout code.
why "layout code"?  Most of layout can be tested in JS IMHO by looking at all
the getClientRects.  Maybe "painting code" would be better?  You could ask eae@
or someone else on layout-dev for their wording.

https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:358: `setup({ explicit_timeout: true });`
What's the implication of this on our bots?  Eg. say we had a bug that caused
eventSender.mouseUp to occasionally be a no-op.  Would the test never time out? 
Presumably our build-bot task timeout would kick in and kill running
webkit_tests, but I think that would be pretty bad.  Perhaps we should only
disable the timeout when eventSender isn't present?

Rick Byers

Oh and LGTM if you want to go ahead and land this now - seems ...

4 years, 1 month ago (2016-11-21 22:39:03 UTC) #32

Rick Byers

On 2016/11/21 22:37:15, Rick Byers wrote: > Sorry for the long delay in reviewing this! ...

4 years, 1 month ago (2016-11-21 22:40:05 UTC) #33

On 2016/11/21 22:37:15, Rick Byers wrote:
> Sorry for the long delay in reviewing this!  I missed it in my daily
codereview
> checks because you had mailto:rbyers@google.com as the reviewer (instead of my
> http://chromium.org account).  I thought I had a nickname specified to make
that
> obvious but looks like that got reset or I deleted it - fixed now.

I should admit this excuse isn't entirely valid.  When I saw Josh last week he
told me about this CL, then I just forgot :-(

> Thanks a ton for working on this!  It looks great overall to me, just a couple
> small suggestions.
> 
> If you want a wider audience before landing / posting to blink-dev then you
> might want to reach out to someone (eg. haraken, jbroman, esprehn) on the
blink
> architecture team as the stewards of overall blink code health.  I suspect
> they'd say they want some docs encouraging more unit testing, but that's
> presumably orthogonal.  Maybe they'd at least have some guidance/docs for you
to
> link to from here?
> 
>
https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
> File docs/testing/writing_layout_tests.md (right):
> 
>
https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:17: [Web Platform Tests
> Project](https://github.com/w3c/web-platform-tests)
> nit: there's technically more than just WPT (eg. test262 for things defined in
> the Ecmascript specification, and csswg-test for CSS stuff - though that's
> hopefully going to be merged into web-platform-test before too long).  Maybe
> just say "covered by cross-browser tests such as the Web Platform Tests
> Project"?
> 
>
https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:19: areas where the major browsers don't
> have interoperable implementations.
> also mention that this lowers the cost of testing by sharing work across all
the
> major engines?
> 
>
https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:39: such as when testing layout code.
> why "layout code"?  Most of layout can be tested in JS IMHO by looking at all
> the getClientRects.  Maybe "painting code" would be better?  You could ask
eae@
> or someone else on layout-dev for their wording.
> 
>
https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:358: `setup({ explicit_timeout: true });`
> What's the implication of this on our bots?  Eg. say we had a bug that caused
> eventSender.mouseUp to occasionally be a no-op.  Would the test never time
out? 
> Presumably our build-bot task timeout would kick in and kill running
> webkit_tests, but I think that would be pretty bad.  Perhaps we should only
> disable the timeout when eventSender isn't present?

pwnall

foolip, qyearsley, rbyers: Thank you very much for your feedback! I learned a lot, and ...

4 years, 1 month ago (2016-11-22 20:32:55 UTC) #38

foolip, qyearsley, rbyers: Thank you very much for your feedback! I learned a
lot, and this writeup is a lot more solid because of it.

I plan to get another grammar review, and hope to land this CL soon, as it's
getting large and unwieldy. If you have time to do another sanity check, that
would be appreciated! FWIW, I did run all the code samples and made sure that
they work.

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
File docs/testing/layout_test_writing.md (right):

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:76: the test scenario efficiently. Avoid
depending on edge case behavior of
On 2016/11/16 18:35:44, qyearsley wrote:
> On 2016/11/16 at 01:50:39, pwnall wrote:
> > On 2016/11/15 18:40:56, qyearsley wrote:
> > > Nit: "efficiently" may be unnecessary here.
> > 
> > I put it in there because I wanted to say that it's OK for a test to take
> advantage of an extra feature (e.g., Promises) if that makes it easier to read
/
> write. I don't want this phrase to read as "don't use features", but as "don't
> add dependencies on features gratuitously".
> > 
> > WDYT?
> 
> Sounds good. This is nitpicky, but I'm still not sure whether there's a better
> word. "Efficiently" maybe sounds like it implies "using as little time or
space
> as possible", although maybe what's meant is more like "concisely"?

Done.
I like concisely. Thank you!

FWIW, "efficiently" is a bit closer to what I have in mind, because when I write
code I balance between being concise and minimizing the mental power required to
read the test. But I agree that a reader would interpret "efficiently" as "fast"
here, and I hope that concisely sends the right message.

https://codereview.chromium.org/2492733003/diff/80001/docs/testing/layout_tes...
docs/testing/layout_test_writing.md:341: behavior.
On 2016/11/16 11:59:53, foolip wrote:
> On 2016/11/16 01:50:39, pwnall wrote:
> > On 2016/11/15 18:40:56, qyearsley wrote:
> > > Although, web platform tests test conformance to a spec (desired behavior)
> and
> > > we add text files to document current behavior.
> > 
> > I think this is an exception. WPT tests are imported verbatim from the W3C
> > repository, so they don't go through our code review process, and they don't
> > necessarily follow our standards.
> 
> I think it is still worth pointing out somehow. As we drive more and more new
> tests to web-platform-tests, having failing expectations as you test things
not
> yet implemented (by Blink, but perhaps others) is going to be common.

Done.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:20: 2. When a Blink feature cannot be
tested using the Web Platform, and cannot be
On 2016/11/18 11:30:40, foolip wrote:
> On 2016/11/18 05:40:43, pwnall wrote:
> > On 2016/11/16 13:42:39, foolip wrote:
> > > s/Web Platform/WPT/? There will be many things that are definitely part of
> the
> > > web platform, but which are currently hard to test using WPT. Input
> automation
> > > is such a case.
> > 
> > Wouldn't you say these are cases where the Web Platform doesn't provide ways
> to
> > test the APIs? For things like input automation, I imagine that WPT might
one
> > day have a WebDriver interface (JS that XHRs to the WPT server, which
controls
> a
> > WebDriver client running the browser that has the test page open) that
> resembles
> > our eventSender, and I imagine that we'll still want to encourage folks to
> write
> > tests that stick to Web Platform features (and don't use the Web Driver
stuff)
> > whenever possible.
> 
> Yes, that's precisely where I think we must go, to standardize enough testing
> APIs (as part of WebDriver or whatever makes sense) so that more and more
things
> can be part of web-platform-tests.
> 
> By s/Web Platform/WPT/ I mean that being in WPT isn't synonymous with being
> automatable using only on web exposed APIs. The question to ask, I think, is
> "can I put this in WPT", and the answer is already yes in a few cases using
> wpt_automation.

I was completely unaware of wpt_automation. I added some wording prescribing (1)
Web Platform APIs, (2) WPT testing APIs, (3) Blink-specific APIs. I also added a
stub section with a TODO for WPT APIs, because I couldn't find any
wpt_automation documentation to summarize and link to.

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:29: There are three broad types of layout
tests, listed in the order of preference.
On 2016/11/18 11:30:39, foolip wrote:
> On 2016/11/18 05:40:43, pwnall wrote:
> > On 2016/11/16 13:42:38, foolip wrote:
> > > There's also the dump render tree kind of test. Not sure where to put it
in
> > > order of preference, I guess between reftests and pixel tests?
> > 
> > I have no idea what I'm talking about, because I haven't worked on layout
> yet...
> > but isn't it possible to use JavaScript to get (and validate) information
that
> > is equivalent to the DRT output?
> 
> Not entirely I think, DRT also includes pseudo elements and maybe other things
> that aren't web exposed. Whatever the case, there's a lot of these tests.
Maybe
> ask eae@ for a blurb describing them?

Done.
I did some reading, and wrote a section on DRT tests. Based on my reading (and
experiment, see the code sample), DRT tests produce both an image and a text
render tree dump. This made me decide they're the least desirable tests. 

FWIW, I think that tests that rely on an internal structure would be worse than
pixel tests, even if we could suppress the image output. But we don't need to
have that argument, as it seems like the image output can't be supressed :D

https://codereview.chromium.org/2492733003/diff/140001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:109: * Tests must use the UTF-8 **character
encoding**, which should be declared by
On 2016/11/18 11:30:40, foolip wrote:
> On 2016/11/18 05:40:43, pwnall wrote:
> > On 2016/11/16 13:42:38, foolip wrote:
> > > Most tests are in ASCII, so I actually ask people to remove <meta
> > charset=utf-8>
> > > when it won't affect the test.
> > > 
> > > Looks like the HTML/CSS Style Guide below actually asks for <meta
> > > charset="utf-8">. Could we say that it's optional for a self-contained
test
> > that
> > > is obviously ASCII and doesn't include anything that's non-ASCII? At least
> > > without a tool to complain about it, I'm not so keen on asking people to
add
> > it
> > > in reviews.
> > 
> > Firefox Nightly now issues a warning when a page doesn't have the charset
> > specified. This was my motivation for removing the pure ASCII exception that
> was
> > in a previous version of this CL.
> 
> I think I'd still like the exception, but again perhaps seek wider feedback.

Done.
I put the exception in for now, while I seek wider feedback.

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:199: assert_equal(!value, false, 'the
logical negtion of true');
On 2016/11/18 11:30:40, foolip wrote:
> I guess assert_false here, since you'll have a chance to show off
assert_equals
> below.

I changed the assertion to assert_equals(value.toString(), 'true') so a
predefined assert wouldn't work :D I think this example is better at showing the
argument ordering.

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:207: assert_true(originallyTrue);
On 2016/11/18 11:30:40, foolip wrote:
> assert_equals(value, orginallyTrue), or maybe the reverse, but something
> actually using the value const :)

Thanks for noticing! I remove the value variable and used an assert_true here.

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:241: * Do not start test case descriptions
with redundant terms like "Testing "
On 2016/11/18 11:30:40, foolip wrote:
> This whole block is great! Nit here is extra space in "Testing " (my fault
> probably)

Done.
Thank you for catching this!
Totally my fault, I was initially intending to do "Testing ..." and figured that
the ellipses are unnecessary.

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:252: * Test case assertions must be wrapped
in `t.step_func()` calls, so that
On 2016/11/18 11:30:40, foolip wrote:
> Callbacks that don't have assertions but could plausibly throw exceptions must
> also be wrapped, or it results in an harness error I believe.

Good point! I amended the description.

> There's also some magic around a single async test in a file when t.step_func
> wrapping isn't needed. I've never depended on it, and I guess wouldn't
encourage
> it, but accept it in review.
I vaguely remember reading about that. I think the savings aren't worth the
mental burden of remembering the special case, and I'd rather not document it.

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:344: * The `assert_equals` string describes
the way the actual value was
On 2016/11/18 11:30:40, foolip wrote:
> The above test doesn't have assert_equals any more. This is quite well
described
> above so maybe this bit isn't needed?

Done.
This block was duplicated (and then edited) above, so I removed it. Thanks for
catching!

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:496: Chromium's testing infrastructure does
not support WPT reftests. In the
On 2016/11/21 09:45:02, foolip wrote:
> On 2016/11/18 18:02:08, qyearsley wrote:
> > On 2016/11/18 at 11:31:42, foolip wrote:
> > > qyearsley@, I think this maybe doesn't quite capture it, can you suggest
> > wording?
> > 
> > Yeah, rather than saying that we "don't support WPT reftests", it's more
> > accurate to say that we currently do extra work when importing to rename
> > reference files to fit our naming convention. This is only needed so that
the
> > test runner finds the reference files.
> > 
> > There was just some related discussion of this in
> > https://bugs.chromium.org/p/chromium/issues/detail?id=268729#c19: It sounds
> like
> > the preferred plan for supporting WPT reftests is to find the reference
files
> by
> > reading from a generated manifest, but that manifest is only going to be in
> the
> > web-platform-tests directory.
> > 
> > So, maybe for the foreseeable future we still want to use the "legacy"
naming
> > convention -expected.html for new reference tests that are not part of WPT,
> and
> > the rule for using <link rel="match" href="..."> only applies to new tests
> that
> > will be part of WPT?
> 
> Keeping -expected.html outside of WPT sounds good to me, presumably it'd be
> hopeless to convert all of them to the other style, and at least
-expected.html
> is easy to understand.

In the spirit of writing tests that can be upstreamed to WPT, how about asking
authors to satisfy both requirements for reference pages ("-expected.html"
naming, and <link rel="match">)? I drafted a new version of this section that
states that.

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:509: and an `-expected.html` suffix.
On 2016/11/18 18:02:08, qyearsley wrote:
> Not sure if it's worth mentioning that it's also possible to name files as
> -expected-mismatch.html, and then the test will pass if there's a mismatch.
> Also, the extension doesn't have to be .html; it can also be htm, svg, xht, or
> xhtml.

Thank you very much for explaining all this! It is very useful. I tried to
incorporate the information in my writeup.

https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:17: [Web Platform Tests
Project](https://github.com/w3c/web-platform-tests)
On 2016/11/21 22:37:15, Rick Byers wrote:
> nit: there's technically more than just WPT (eg. test262 for things defined in
> the Ecmascript specification, and csswg-test for CSS stuff - though that's
> hopefully going to be merged into web-platform-test before too long).  Maybe
> just say "covered by cross-browser tests such as the Web Platform Tests
> Project"?

I would rather have specific projects and clear links to them. I think that
while more general wording would be closer to the truth, it would also be less
useful.

csswg-test: I assume that csswg-test is logically a part of WPT. My assumption
is based on the facts that the top of the  WPT README states that the CSS tests
are in a different repository for historic reasons, and that the WPT
documentation covers writing CSS tests. I also assume that folks who follow the
links here will easily arrive to the correct conclusion about the csswg-test /
web-platform-tests split.

test262: Would layout tests fall under test262? I imagined that ECMA262 issues
would be tested in the v8 repository.

I did add a promo early on that begs readers to get in touch and update this
document if we upstream tests to other communal repositories.

https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:19: areas where the major browsers don't
have interoperable implementations.
On 2016/11/21 22:37:14, Rick Byers wrote:
> also mention that this lowers the cost of testing by sharing work across all
the
> major engines?

Done.

https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:39: such as when testing layout code.
On 2016/11/21 22:37:15, Rick Byers wrote:
> why "layout code"?  Most of layout can be tested in JS IMHO by looking at all
> the getClientRects.  Maybe "painting code" would be better?  You could ask
eae@
> or someone else on layout-dev for their wording.

Done.
I replaced "layout" with paint, and will seek eae's opinion.

I don't really know what I'm doing here :) My goal is to write something that's
close enough to the real thing that knowledgeable people will be willing to fix
what's broken. My rationale is that without this doc, new developers have to
Google around and learn from reviews. Given that we're all time constrained, the
knowledge is going to be less complete than what's summarized here.

https://codereview.chromium.org/2492733003/diff/200001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:358: `setup({ explicit_timeout: true });`
On 2016/11/21 22:37:14, Rick Byers wrote:
> What's the implication of this on our bots?  Eg. say we had a bug that caused
> eventSender.mouseUp to occasionally be a no-op.  Would the test never time
out? 
> Presumably our build-bot task timeout would kick in and kill running
> webkit_tests, but I think that would be pretty bad.  Perhaps we should only
> disable the timeout when eventSender isn't present?

My experiments suggest that we have an external timeout mechanism that
testharness.js can't modify. When my test had a similar bug (I was clicking
outside the button), it still timed out.

pwnall

eae@: Can you please make a quick pass and point out egregious mistakes? Please focus ...

4 years, 1 month ago (2016-11-22 20:34:45 UTC) #40

eae

Overall this looks great but there are a number of things that struck me as ...

4 years, 1 month ago (2016-11-22 21:53:12 UTC) #41

Overall this looks great but there are a number of things that struck me as
confusing and out of scope. Paid special attention to the sections requested but
this skimmed the entire document and added a few comments there as well.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:13: Layout tests should be used to
accomplish one of the following goals:
(optional suggestion) I would change this to say that layout tests should
accomplish the goals outlined in 1 and then have another paragraph saying that
they're currently also used for 2. We shouldn't be adding more tests of type 2
now that we have better unit tests support and a mandate to use web-platform
tests for layout tests.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:66: 
I'd add web platform tests as a distinct fifth category. They're distinct from
JavaScript tests in numerous and important ways.

It's also probably worth mentioning that a lot of tests belong to two or more
categories. For instance a lot of our DRT tests are also Javascript tests (have
asser-like behavior) and many of the pixel tests are also DRT tests.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:75: elements that are necessary and
relevant to what is being tested.
I know it comes from the TTWF guidelines but I don't like this section at all.
It is direct conflict with the self-describing one below which, imho, is *much*
more important.

Clarity over brevity.

We have a number of tests today that intentionally includes extra information to
show what and how something is broken when there is a failure rather than just
showing a FAIL line. This has proven incredibly helpful on countless occasions
and I would hate to loose that to save a few bytes.

Can we please reword this to say that tests should only include content relevant
to what is being tested without enforcing it to be as short as possible or with
as few elements as possible?

Actually, the *minimal* section below covers that pretty well. I'd drop this
section entirely.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:76: 
(optional) I'd also include the window size guidelines from the TTWF guidelines
as we limit the size of the window.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:107: * Prefer JavaScript's
Why? The google3 js style guide makes the exact *opposite* recommendation.

The majority of comparisons in layout tests are for pixel values where this is
bad advice given that blink uses integers or layout units internally and js uses
floats.

More importantly, stylistic rules should be governed by the style guide, not
this document.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:145: * Tests should prefer **modern
features** in JavaScript and in the Web Platform.
These guidelines are in conflict with the *cross-platform* and *minimal*
sections and doesn't match the existing tests.

var is used over 50 *fifty* times more than let. Are we also going to rewrite
all our existing tests? If not I'd argue that consistency is way more important.
We can't have 90% of our tests fail the guidelines.

It is also worth pointing out that these guidelines do *not* match the web
platform tests style guide, recommendations or even examples. Given that we plan
to share these tests with other vendors it doesn't make sense to me to enforce
rules outside those that other vendors enforce and outside the agreed on
guidelines.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:173: * Tests must aim to have a **coding
style** that is consistent with
Shouldn't this be http://testthewebforward.org/docs/test-style-guidelines.html
instead?

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:174: [Google's JavaScript Style
Guide](https://google.github.io/styleguide/javascriptguide.xml),
This has moved.
https://google.github.io/styleguide/jsguide.html

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:179: * Modern Web Platform and JavaScript
features should be preferred to legacy
Again, why?

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:274: * Test files with a single test case
should omit the test case description.
This would require tooling changes on our side.

The test case description is shown when a test is failing. The title is not.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:628: * use the [Ahem
font](https://www.w3.org/Style/CSS/Test/Fonts/Ahem/README) to
Unless text, text flow, font selection, font fallback, font features or other
typographic information is part of the test.

Also it doesn't really do much to minimize platform differences, at least not
for us, so I'm not sure this is still good advice.

It trades readable self-describing *fast* tests into hard-to-reason-about test
that have to synchronously load a font from an array buffer using js.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:630: 
* The viewport for pixel tests is 800x600, content outside that viewport will
*not* be compared.
* If identifying a failing/passing test isn't immediately obvious include a
textual description of the desired (passing) outcome.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:659: A Dump Render Tree test renders a web
page and produces two results, which are
A DRT test can produce either text result, pixel results, or both.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:669: DRT tests are slow, and their
baselines depend on platform-specific mechanisms,
Actually they are much faster than reference tests. There are *many* reasons not
to use DRT tests but speed isn't really one of them.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:694: <p>This paragraph must be green.</p>
This is a bad example. This will render as a series of green boxes and lines.
Not as a continuous green rectangle as suggested by the TTWF guidelines. Either
change it to render as a rectangle and/or include a description.

<style>
div { font: 10px Ahem; color: green; }
</style>

<div>aaaaa</div>
<p>There must be a green box above.</p>

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:715: baseline would have depended on the
fonts installed on the testing computer, and
Using Ahem does *not* guarantee that there won't be platform differences. It
helps *minimize* them for specific runs of text.

In this case the test should disable the text output and only compare the pixel
results as the render-tree dump is irrelevant. There are very few cases where
having a render-tree text dump is relevant for pixel tests. I'd like to see the
guideline discourage it.

eae

Also, thank you so much for working on this! This is something that we all ...

4 years, 1 month ago (2016-11-22 22:01:58 UTC) #42

foolip

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_layout_tests.md File docs/testing/writing_layout_tests.md (right): https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_layout_tests.md#newcode199 docs/testing/writing_layout_tests.md:199: assert_equal(!value, false, 'the logical negtion of true'); On 2016/11/22 ...

4 years, 1 month ago (2016-11-23 09:23:59 UTC) #43

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:199: assert_equal(!value, false, 'the
logical negtion of true');
On 2016/11/22 20:32:55, pwnall wrote:
> On 2016/11/18 11:30:40, foolip wrote:
> > I guess assert_false here, since you'll have a chance to show off
> assert_equals
> > below.
> 
> I changed the assertion to assert_equals(value.toString(), 'true') so a
> predefined assert wouldn't work :D I think this example is better at showing
the
> argument ordering.

Thanks, that looks good!

https://codereview.chromium.org/2492733003/diff/160001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:496: Chromium's testing infrastructure does
not support WPT reftests. In the
On 2016/11/22 20:32:55, pwnall wrote:
> On 2016/11/21 09:45:02, foolip wrote:
> > On 2016/11/18 18:02:08, qyearsley wrote:
> > > On 2016/11/18 at 11:31:42, foolip wrote:
> > > > qyearsley@, I think this maybe doesn't quite capture it, can you suggest
> > > wording?
> > > 
> > > Yeah, rather than saying that we "don't support WPT reftests", it's more
> > > accurate to say that we currently do extra work when importing to rename
> > > reference files to fit our naming convention. This is only needed so that
> the
> > > test runner finds the reference files.
> > > 
> > > There was just some related discussion of this in
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=268729#c19: It
sounds
> > like
> > > the preferred plan for supporting WPT reftests is to find the reference
> files
> > by
> > > reading from a generated manifest, but that manifest is only going to be
in
> > the
> > > web-platform-tests directory.
> > > 
> > > So, maybe for the foreseeable future we still want to use the "legacy"
> naming
> > > convention -expected.html for new reference tests that are not part of
WPT,
> > and
> > > the rule for using <link rel="match" href="..."> only applies to new tests
> > that
> > > will be part of WPT?
> > 
> > Keeping -expected.html outside of WPT sounds good to me, presumably it'd be
> > hopeless to convert all of them to the other style, and at least
> -expected.html
> > is easy to understand.
> 
> In the spirit of writing tests that can be upstreamed to WPT, how about asking
> authors to satisfy both requirements for reference pages ("-expected.html"
> naming, and <link rel="match">)? I drafted a new version of this section that
> states that.

I think using just one model at a time is preferable, otherwise people have to
wonder which it is that's actually doing the work, and why <link rel="match"
href="ol-reversed-ref.html"> or similar won't work.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:274: * Test files with a single test case
should omit the test case description.
On 2016/11/22 21:53:11, eae wrote:
> This would require tooling changes on our side.
> 
> The test case description is shown when a test is failing. The title is not.

testharness.js uses document.title as the default description, see
WindowTestEnvironment.prototype.next_default_test_name

foolip

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_layout_tests.md File docs/testing/writing_layout_tests.md (right): https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_layout_tests.md#newcode145 docs/testing/writing_layout_tests.md:145: * Tests should prefer **modern features** in JavaScript and ...

4 years, 1 month ago (2016-11-23 09:39:23 UTC) #44

eae

On Wed, Nov 23, 2016 at 1:39 AM, <foolip@chromium.org> wrote: > > https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_layout_tests.md > File ...

4 years ago (2016-11-23 19:11:01 UTC) #45

On Wed, Nov 23, 2016 at 1:39 AM,  <foolip@chromium.org> wrote:
>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> File docs/testing/writing_layout_tests.md (right):
>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:145: * Tests should prefer **modern
> features** in JavaScript and in the Web Platform.
> On 2016/11/22 21:53:12, eae wrote:
>> These guidelines are in conflict with the *cross-platform* and
> *minimal*
>> sections and doesn't match the existing tests.
>>
>> var is used over 50 *fifty* times more than let. Are we also going to
> rewrite
>> all our existing tests? If not I'd argue that consistency is way more
> important.
>> We can't have 90% of our tests fail the guidelines.
>>
>> It is also worth pointing out that these guidelines do *not* match the
> web
>> platform tests style guide, recommendations or even examples. Given
> that we plan
>> to share these tests with other vendors it doesn't make sense to me to
> enforce
>> rules outside those that other vendors enforce and outside the agreed
> on
>> guidelines.
>
> Some folks, including me, have recently written some upstream
> web-platform-tests that use let/const or arrow functions where not
> necessary:
> https://github.com/w3c/web-platform-tests/pull/4054
> https://github.com/w3c/web-platform-tests/pull/4062
> https://github.com/w3c/web-platform-tests/pull/4092
> https://github.com/w3c/web-platform-tests/pull/4134
> https://github.com/w3c/web-platform-tests/pull/4146
>
> So I guess the situation upstream is that new features are accepted, but
> not required.
>
> (What would really help here is if we had a way of running the tests on
> other browsers before landing. Then we could ask test authors to ensure
> that failures on other browsers aren't because of completely unrelated
> things.)

I still don't understand why we would mandate the use of features that
are completely irrelevant for the test in question and that violates
the simplicity and interop guidelines.
Can we please let this be a guideline and let developer use their own
judgement rather than mandating personal preferences?

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

foolip

On 2016/11/23 19:11:01, eae wrote: > On Wed, Nov 23, 2016 at 1:39 AM, <mailto:foolip@chromium.org> ...

4 years ago (2016-11-23 19:26:24 UTC) #46

On 2016/11/23 19:11:01, eae wrote:
> On Wed, Nov 23, 2016 at 1:39 AM,  <mailto:foolip@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> > File docs/testing/writing_layout_tests.md (right):
> >
> >
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> > docs/testing/writing_layout_tests.md:145: * Tests should prefer **modern
> > features** in JavaScript and in the Web Platform.
> > On 2016/11/22 21:53:12, eae wrote:
> >> These guidelines are in conflict with the *cross-platform* and
> > *minimal*
> >> sections and doesn't match the existing tests.
> >>
> >> var is used over 50 *fifty* times more than let. Are we also going to
> > rewrite
> >> all our existing tests? If not I'd argue that consistency is way more
> > important.
> >> We can't have 90% of our tests fail the guidelines.
> >>
> >> It is also worth pointing out that these guidelines do *not* match the
> > web
> >> platform tests style guide, recommendations or even examples. Given
> > that we plan
> >> to share these tests with other vendors it doesn't make sense to me to
> > enforce
> >> rules outside those that other vendors enforce and outside the agreed
> > on
> >> guidelines.
> >
> > Some folks, including me, have recently written some upstream
> > web-platform-tests that use let/const or arrow functions where not
> > necessary:
> > https://github.com/w3c/web-platform-tests/pull/4054
> > https://github.com/w3c/web-platform-tests/pull/4062
> > https://github.com/w3c/web-platform-tests/pull/4092
> > https://github.com/w3c/web-platform-tests/pull/4134
> > https://github.com/w3c/web-platform-tests/pull/4146
> >
> > So I guess the situation upstream is that new features are accepted, but
> > not required.
> >
> > (What would really help here is if we had a way of running the tests on
> > other browsers before landing. Then we could ask test authors to ensure
> > that failures on other browsers aren't because of completely unrelated
> > things.)
> 
> I still don't understand why we would mandate the use of features that
> are completely irrelevant for the test in question and that violates
> the simplicity and interop guidelines.
> Can we please let this be a guideline and let developer use their own
> judgement rather than mandating personal preferences?

Leaving it to author/reviewer discretion while discouraging gratuitous use of
shiny new things WFM.

pwnall

eae: Thank you very much for the feedback, and for your expertise around layout tests! ...

4 years ago (2016-11-24 03:39:22 UTC) #48

eae: Thank you very much for the feedback, and for your expertise around layout
tests!

Some of the issues seem to warrant wider input. After landing this CL, I intend
to bring up the document on blink-dev, and we can hash out the issues there. I
think it'll be easier to contribute if this is rendered markdown rather than a
CL. Is that OK?

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
File docs/testing/writing_layout_tests.md (right):

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:13: Layout tests should be used to
accomplish one of the following goals:
On 2016/11/22 21:53:11, eae wrote:
> (optional suggestion) I would change this to say that layout tests should
> accomplish the goals outlined in 1 and then have another paragraph saying that
> they're currently also used for 2. We shouldn't be adding more tests of type 2
> now that we have better unit tests support and a mandate to use web-platform
> tests for layout tests.

It might be the case that I haven't read the right documentation on C++ unit
tests, but I still find it a lot easier to write layout tests for most of the
functionality I'm testing, e.g. drag and drop.

If there is some good documentation for C++ unit tests, can you please point me
to it, so I can link it into this guide?

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:66: 
On 2016/11/22 21:53:11, eae wrote:
> I'd add web platform tests as a distinct fifth category. They're distinct from
> JavaScript tests in numerous and important ways.
> 
> It's also probably worth mentioning that a lot of tests belong to two or more
> categories. For instance a lot of our DRT tests are also Javascript tests
(have
> asser-like behavior) and many of the pixel tests are also DRT tests.

I agree with your assessment of the current situation. It seems to me that, for
new tests, it makes sense to separate tests that use assertions from tests that
produce a render tree dump. It'd be easier to debug such tests, as the render
tree would be smaller. Duplication can be avoided by moving shared code into a
resources/ file.

WDYT?

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:75: elements that are necessary and
relevant to what is being tested.
On 2016/11/22 21:53:11, eae wrote:
> I know it comes from the TTWF guidelines but I don't like this section at all.
> It is direct conflict with the self-describing one below which, imho, is
*much*
> more important.
> 
> Clarity over brevity.
> 
> We have a number of tests today that intentionally includes extra information
to
> show what and how something is broken when there is a failure rather than just
> showing a FAIL line. This has proven incredibly helpful on countless occasions
> and I would hate to loose that to save a few bytes.
> 
> Can we please reword this to say that tests should only include content
relevant
> to what is being tested without enforcing it to be as short as possible or
with
> as few elements as possible?
> 
> Actually, the *minimal* section below covers that pretty well. I'd drop this
> section entirely.

I modified the description to clarify that "short" (reworded as concise, which I
think is clearer) does not trump the other principles, and added the statement
regarding making failures easy to debug.

I think that "short" translates to concise, and "minimal" translates to no
unnecessary complexity on the reader/maintainer. I changed the writeup to
reflect this.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:76: 
On 2016/11/22 21:53:11, eae wrote:
> (optional) I'd also include the window size guidelines from the TTWF
guidelines
> as we limit the size of the window.

I added some wording for the window size. Thank you very much for pointing this
out!

I assumed (perhaps incorrectly) that "window size" actually means "viewport
size", and does not include decorations. Is this correct? I imagine we'd have
chaos if we'd include platform-dependent window decorations in the 800x600 size.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:107: * Prefer JavaScript's
On 2016/11/22 21:53:11, eae wrote:
> Why? The google3 js style guide makes the exact *opposite* recommendation.

FWIW, the google3 style guide changed completely since last time I read it. It
might have changed today, so perhaps you're referring to an old version? The new
guide doesn't seem to take any position on ==.

> The majority of comparisons in layout tests are for pixel values where this is
> bad advice given that blink uses integers or layout units internally and js
uses
> floats.
> 
> More importantly, stylistic rules should be governed by the style guide, not
> this document.

This guide uses "layout tests" to refer to everything under WebKit/LayoutTests.
The tests I've written (File API, IndexedDB, drag and drop) compare strings /
booleans / null roughly as much as they compare numbers.

That being said, I'm not sure why == differs from === when both sides of the
comparison are numbers. Can you please explain?

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:145: * Tests should prefer **modern
features** in JavaScript and in the Web Platform.
On 2016/11/23 09:39:23, foolip wrote:
> (What would really help here is if we had a way of running the tests on other
> browsers before landing. Then we could ask test authors to ensure that
failures
> on other browsers aren't because of completely unrelated things.)

There is a recommendation below for using caniuse.com and avoiding features that
aren't shipped in all major engines.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:145: * Tests should prefer **modern
features** in JavaScript and in the Web Platform.
On 2016/11/22 21:53:12, eae wrote:
> These guidelines are in conflict with the *cross-platform* and *minimal*
> sections and doesn't match the existing tests.

These guidelines are for new tests.

cross-platform: The features that I mentioned here  are shipped in all major
browsers.
minimal: Based on your feedback, clarified *minimal* to mean minimal burden on
reader / maintainer. Based on my Web development experience, all the features I
cited here (use strict, let / const, classes, promises) translate into code that
is easier to review and maintain.

> var is used over 50 *fifty* times more than let. Are we also going to rewrite
> all our existing tests? If not I'd argue that consistency is way more
important.
> We can't have 90% of our tests fail the guidelines.

We don't have any rules currently, so we have a mix of styles. I think that the
path towards consistency starts with setting guidelines for new tests, and
continues with gradually upgrading old tests. I believe this is how Google3's
style guides started too, as the C++ guide specifically mentions the existence
of old code that doesn't obey the rules.

I think const and let are the right thing to do for new tests, as their
semantics enhance readability in at least two ways - (1) the variables are
defined at block-level, and are only valid after they are declared (no
hoisting), and (2) const vs let communicates mutability intent and makes it
easier to reason about the values that a variable can take.

> It is also worth pointing out that these guidelines do *not* match the web
> platform tests style guide, recommendations or even examples. Given that we
plan
> to share these tests with other vendors it doesn't make sense to me to enforce
> rules outside those that other vendors enforce and outside the agreed on
> guidelines.

Tests that we import from WPT (or other projects) do not go through code review,
so we don't need to worry about them meeting our rules. I think they should be
regarded as third_party/ code.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:173: * Tests must aim to have a **coding
style** that is consistent with
On 2016/11/22 21:53:11, eae wrote:
> Shouldn't this be http://testthewebforward.org/docs/test-style-guidelines.html
> instead?

The WPT page describes test construction, not coding style. I haven't found any
coding style notes on testthewebforward.org, so I'm following the Chromium
guideline that we use Google style unless we have a good reason to deviate from
it.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:174: [Google's JavaScript Style
Guide](https://google.github.io/styleguide/javascriptguide.xml),
On 2016/11/22 21:53:11, eae wrote:
> This has moved.
> https://google.github.io/styleguide/jsguide.html

Done.
Thanks so much! FWIW, this wasn't the case when the CL started :D

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:179: * Modern Web Platform and JavaScript
features should be preferred to legacy
On 2016/11/22 21:53:11, eae wrote:
> Again, why?

Please see above :)

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:628: * use the [Ahem
font](https://www.w3.org/Style/CSS/Test/Fonts/Ahem/README) to
On 2016/11/22 21:53:12, eae wrote:
> Unless text, text flow, font selection, font fallback, font features or other
> typographic information is part of the test.

Good point. I added this to the writeup. Thank you very much!

> Also it doesn't really do much to minimize platform differences, at least not
> for us, so I'm not sure this is still good advice.

FWIW, I recently wrote a test that generates a pixel result. The result was the
same for all platforms when I could use Ahem, and I had to use per-platform
expectations when I couldn't use Ahem. So, I have data showing that it still
adds value.

> 
> It trades readable self-describing *fast* tests into hard-to-reason-about test
> that have to synchronously load a font from an array buffer using js.

This is a very good point! Thank you very much for bringing it up!

How about (me submitting a CL for) modifying ahem.js to only load the Ahem font
when window.testRunner is detected? I effectively did this (manually, via CSS
classes) in a test I wrote.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:630: 
On 2016/11/22 21:53:11, eae wrote:
> * The viewport for pixel tests is 800x600, content outside that viewport will
> *not* be compared.
> * If identifying a failing/passing test isn't immediately obvious include a
> textual description of the desired (passing) outcome.

Done.
This is super-important. Thank you for bringing it up!

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:659: A Dump Render Tree test renders a web
page and produces two results, which are
On 2016/11/22 21:53:11, eae wrote:
> A DRT test can produce either text result, pixel results, or both.

I've been trying to use the DRT term to only label tests that produce a render
tree dump. Can we use the term pixel tests for the tests that only produce pixel
results? It seems clearer.

I think I was misguided by a WebKit page.
https://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

It says "How to disable the pixel dumps but keep the render tree dump
There is currently no way for you to do that."

If there's a way to produce a render tree but not a pixel result, can you please
tell me how it works? OR, if you prefer, I can leave a TODO in the file, and you
can update it.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:669: DRT tests are slow, and their
baselines depend on platform-specific mechanisms,
On 2016/11/22 21:53:12, eae wrote:
> Actually they are much faster than reference tests. There are *many* reasons
not
> to use DRT tests but speed isn't really one of them.

Done.
I corrected the text. Thank you very much for pointing this out!

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:694: <p>This paragraph must be green.</p>
On 2016/11/22 21:53:11, eae wrote:
> This is a bad example. This will render as a series of green boxes and lines.
> Not as a continuous green rectangle as suggested by the TTWF guidelines.
Either
> change it to render as a rectangle and/or include a description.
> 
> <style>
> div { font: 10px Ahem; color: green; }
> </style>
> 
> <div>aaaaa</div>
> <p>There must be a green box above.</p>

I tried to put together a better example. This is self-describing (when the
browser doesn't have the Ahem font installed), and tests some functionality that
couldn't be covered using Web APIs. One could argue that most of the
functionality could be covered with reftests, but perhaps we care about the
render tree.

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
docs/testing/writing_layout_tests.md:715: baseline would have depended on the
fonts installed on the testing computer, and
On 2016/11/22 21:53:11, eae wrote:
> Using Ahem does *not* guarantee that there won't be platform differences. It
> helps *minimize* them for specific runs of text.
> 
> In this case the test should disable the text output and only compare the
pixel
> results as the render-tree dump is irrelevant. There are very few cases where
> having a render-tree text dump is relevant for pixel tests. I'd like to see
the
> guideline discourage it.
> 
> 

I thought the guideline discourages it? Relevant text cited below.

"for these reasons, DRT tests should **only** be used to cover aspects of the
layout code that can only be tested by looking at the render tree. Any
combination of the other test types is preferable to a DRT test."

foolip

https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_layout_tests.md File docs/testing/writing_layout_tests.md (right): https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_layout_tests.md#newcode145 docs/testing/writing_layout_tests.md:145: * Tests should prefer **modern features** in JavaScript and ...

4 years ago (2016-11-24 08:52:57 UTC) #49

eae

On Thu, Nov 24, 2016 at 3:39 AM, <pwnall@chromium.org> wrote: > eae: Thank you very ...

4 years ago (2016-11-25 17:06:10 UTC) #50

On Thu, Nov 24, 2016 at 3:39 AM,  <pwnall@chromium.org> wrote:
> eae: Thank you very much for the feedback, and for your expertise around
> layout
> tests!
>
> Some of the issues seem to warrant wider input. After landing this CL, I
> intend
> to bring up the document on blink-dev, and we can hash out the issues there.
> I
> think it'll be easier to contribute if this is rendered markdown rather than
> a
> CL. Is that OK?

SGTM, thank you.

>
>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> File docs/testing/writing_layout_tests.md (right):
>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:13: Layout tests should be used to
> accomplish one of the following goals:
> On 2016/11/22 21:53:11, eae wrote:
>> (optional suggestion) I would change this to say that layout tests
> should
>> accomplish the goals outlined in 1 and then have another paragraph
> saying that
>> they're currently also used for 2. We shouldn't be adding more tests
> of type 2
>> now that we have better unit tests support and a mandate to use
> web-platform
>> tests for layout tests.
>
> It might be the case that I haven't read the right documentation on C++
> unit tests, but I still find it a lot easier to write layout tests for
> most of the functionality I'm testing, e.g. drag and drop.

Makes sense, sadly our documentation for unit tests is rather lacking
at the moment.

>
> If there is some good documentation for C++ unit tests, can you please
> point me to it, so I can link it into this guide?
>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:66:
> On 2016/11/22 21:53:11, eae wrote:
>> I'd add web platform tests as a distinct fifth category. They're
> distinct from
>> JavaScript tests in numerous and important ways.
>>
>> It's also probably worth mentioning that a lot of tests belong to two
> or more
>> categories. For instance a lot of our DRT tests are also Javascript
> tests (have
>> asser-like behavior) and many of the pixel tests are also DRT tests.
>
> I agree with your assessment of the current situation. It seems to me
> that, for new tests, it makes sense to separate tests that use
> assertions from tests that produce a render tree dump. It'd be easier to
> debug such tests, as the render tree would be smaller. Duplication can
> be avoided by moving shared code into a resources/ file.
>
> WDYT?

Agreed.

>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:75: elements that are necessary and
> relevant to what is being tested.
> On 2016/11/22 21:53:11, eae wrote:
>> I know it comes from the TTWF guidelines but I don't like this section
> at all.
>> It is direct conflict with the self-describing one below which, imho,
> is *much*
>> more important.
>>
>> Clarity over brevity.
>>
>> We have a number of tests today that intentionally includes extra
> information to
>> show what and how something is broken when there is a failure rather
> than just
>> showing a FAIL line. This has proven incredibly helpful on countless
> occasions
>> and I would hate to loose that to save a few bytes.
>>
>> Can we please reword this to say that tests should only include
> content relevant
>> to what is being tested without enforcing it to be as short as
> possible or with
>> as few elements as possible?
>>
>> Actually, the *minimal* section below covers that pretty well. I'd
> drop this
>> section entirely.
>
> I modified the description to clarify that "short" (reworded as concise,
> which I think is clearer) does not trump the other principles, and added
> the statement regarding making failures easy to debug.
>
> I think that "short" translates to concise, and "minimal" translates to
> no unnecessary complexity on the reader/maintainer. I changed the
> writeup to reflect this.

Much better and I agree, thank you.

>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:76:
> On 2016/11/22 21:53:11, eae wrote:
>> (optional) I'd also include the window size guidelines from the TTWF
> guidelines
>> as we limit the size of the window.
>
> I added some wording for the window size. Thank you very much for
> pointing this out!
>
> I assumed (perhaps incorrectly) that "window size" actually means
> "viewport size", and does not include decorations. Is this correct? I
> imagine we'd have chaos if we'd include platform-dependent window
> decorations in the 800x600 size.

Right, 800x600 viewport.

>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:107: * Prefer JavaScript's
> On 2016/11/22 21:53:11, eae wrote:
>> Why? The google3 js style guide makes the exact *opposite*
> recommendation.
>
> FWIW, the google3 style guide changed completely since last time I read
> it. It might have changed today, so perhaps you're referring to an old
> version? The new guide doesn't seem to take any position on ==.

Good point, we used to disallow === while now we're silent on it. We
should probably be too.

>> The majority of comparisons in layout tests are for pixel values where
> this is
>> bad advice given that blink uses integers or layout units internally
> and js uses
>> floats.
>>
>> More importantly, stylistic rules should be governed by the style
> guide, not
>> this document.
>
> This guide uses "layout tests" to refer to everything under
> WebKit/LayoutTests. The tests I've written (File API, IndexedDB, drag
> and drop) compare strings / booleans / null roughly as much as they
> compare numbers.
>
> That being said, I'm not sure why == differs from === when both sides of
> the comparison are numbers. Can you please explain?

a === b is effectively translates to typeof(a) == typeof(b) && a == b.
The reason we used to discourage it in google3 was that we wanted to
make it *very* clear when something unusual was going on that required
that level of comparisons. For the majority of comparisons == is
perfectly fine and functionally equivalent.

For pixel values we shouldn't use direction comparison *at all* and
instead use isNear or equivalent to allow for floating point
imprecisions and rounding.

>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:145: * Tests should prefer **modern
> features** in JavaScript and in the Web Platform.
> On 2016/11/23 09:39:23, foolip wrote:
>> (What would really help here is if we had a way of running the tests
> on other
>> browsers before landing. Then we could ask test authors to ensure that
> failures
>> on other browsers aren't because of completely unrelated things.)
>
> There is a recommendation below for using caniuse.com and avoiding
> features that aren't shipped in all major engines.
>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:145: * Tests should prefer **modern
> features** in JavaScript and in the Web Platform.
> On 2016/11/22 21:53:12, eae wrote:
>> These guidelines are in conflict with the *cross-platform* and
> *minimal*
>> sections and doesn't match the existing tests.
>
> These guidelines are for new tests.
>
> cross-platform: The features that I mentioned here are shipped in all
> major browsers.
> minimal: Based on your feedback, clarified *minimal* to mean minimal
> burden on reader / maintainer. Based on my Web development experience,
> all the features I cited here (use strict, let / const, classes,
> promises) translate into code that is easier to review and maintain.
>
>> var is used over 50 *fifty* times more than let. Are we also going to
> rewrite
>> all our existing tests? If not I'd argue that consistency is way more
> important.
>> We can't have 90% of our tests fail the guidelines.
>
> We don't have any rules currently, so we have a mix of styles. I think
> that the path towards consistency starts with setting guidelines for new
> tests, and continues with gradually upgrading old tests. I believe this
> is how Google3's style guides started too, as the C++ guide specifically
> mentions the existence of old code that doesn't obey the rules.
>
> I think const and let are the right thing to do for new tests, as their
> semantics enhance readability in at least two ways - (1) the variables
> are defined at block-level, and are only valid after they are declared
> (no hoisting), and (2) const vs let communicates mutability intent and
> makes it easier to reason about the values that a variable can take.

I strongly disagree. If you want to change our style guide please go
though the proper channels and start the discussion there. Please
don't try to shoe-horn in style guide changes here.
We want to minimize the friction to writing tests, mandating the use
of completely irrelevant features seems counter productive at best.

>> It is also worth pointing out that these guidelines do *not* match the
> web
>> platform tests style guide, recommendations or even examples. Given
> that we plan
>> to share these tests with other vendors it doesn't make sense to me to
> enforce
>> rules outside those that other vendors enforce and outside the agreed
> on
>> guidelines.
>
> Tests that we import from WPT (or other projects) do not go through code
> review, so we don't need to worry about them meeting our rules. I think
> they should be regarded as third_party/ code.
>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:173: * Tests must aim to have a
> **coding style** that is consistent with
> On 2016/11/22 21:53:11, eae wrote:
>> Shouldn't this be
> http://testthewebforward.org/docs/test-style-guidelines.html
>> instead?
>
> The WPT page describes test construction, not coding style. I haven't
> found any coding style notes on testthewebforward.org, so I'm following
> the Chromium guideline that we use Google style unless we have a good
> reason to deviate from it.
>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:174: [Google's JavaScript Style
> Guide](https://google.github.io/styleguide/javascriptguide.xml),
> On 2016/11/22 21:53:11, eae wrote:
>> This has moved.
>> https://google.github.io/styleguide/jsguide.html
>
> Done.
> Thanks so much! FWIW, this wasn't the case when the CL started :D
>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:179: * Modern Web Platform and
> JavaScript features should be preferred to legacy
> On 2016/11/22 21:53:11, eae wrote:
>> Again, why?
>
> Please see above :)

I'm sorry but you personal preferences isn't reason enough overrule
the style guide and the TTWF recommendations. I'm fine with *allowing*
es6 features. I'm *not* fine with *mandating* it.

>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:628: * use the [Ahem
> font](https://www.w3.org/Style/CSS/Test/Fonts/Ahem/README) to
> On 2016/11/22 21:53:12, eae wrote:
>> Unless text, text flow, font selection, font fallback, font features
> or other
>> typographic information is part of the test.
>
> Good point. I added this to the writeup. Thank you very much!
>
>> Also it doesn't really do much to minimize platform differences, at
> least not
>> for us, so I'm not sure this is still good advice.
>
> FWIW, I recently wrote a test that generates a pixel result. The result
> was the same for all platforms when I could use Ahem, and I had to use
> per-platform expectations when I couldn't use Ahem. So, I have data
> showing that it still adds value.

Then you were very lucky or wrote a very good test! Either case I'd be
curious to see it, perhaps you found a way to dela with the
differences that we could all learn from!

>> It trades readable self-describing *fast* tests into
> hard-to-reason-about test
>> that have to synchronously load a font from an array buffer using js.
>
> This is a very good point! Thank you very much for bringing it up!
>
> How about (me submitting a CL for) modifying ahem.js to only load the
> Ahem font when window.testRunner is detected? I effectively did this
> (manually, via CSS classes) in a test I wrote.

We really do want tests to behave the same way when run in content
shell or a browser, makes debugging a lot easier. We used to try to
side-load Ahem into testrunner to avoid the perf costs and changes it
to use ahem.js specifically to address the debug-ability problem.

>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:630:
> On 2016/11/22 21:53:11, eae wrote:
>> * The viewport for pixel tests is 800x600, content outside that
> viewport will
>> *not* be compared.
>> * If identifying a failing/passing test isn't immediately obvious
> include a
>> textual description of the desired (passing) outcome.
>
> Done.
> This is super-important. Thank you for bringing it up!
>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:659: A Dump Render Tree test
> renders a web page and produces two results, which are
> On 2016/11/22 21:53:11, eae wrote:
>> A DRT test can produce either text result, pixel results, or both.
>
> I've been trying to use the DRT term to only label tests that produce a
> render tree dump. Can we use the term pixel tests for the tests that
> only produce pixel results? It seems clearer.
>
> I think I was misguided by a WebKit page.
> https://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
>
> It says "How to disable the pixel dumps but keep the render tree dump
> There is currently no way for you to do that."
>
> If there's a way to produce a render tree but not a pixel result, can
> you please tell me how it works? OR, if you prefer, I can leave a TODO
> in the file, and you can update it.

I'm pretty sure there is a set of flags for that, please add a TODO
and I'll fill it out.

>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:669: DRT tests are slow, and their
> baselines depend on platform-specific mechanisms,
> On 2016/11/22 21:53:12, eae wrote:
>> Actually they are much faster than reference tests. There are *many*
> reasons not
>> to use DRT tests but speed isn't really one of them.
>
> Done.
> I corrected the text. Thank you very much for pointing this out!

Sweet, thank you.

>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:694: <p>This paragraph must be
> green.</p>
> On 2016/11/22 21:53:11, eae wrote:
>> This is a bad example. This will render as a series of green boxes and
> lines.
>> Not as a continuous green rectangle as suggested by the TTWF
> guidelines. Either
>> change it to render as a rectangle and/or include a description.
>>
>> <style>
>> div { font: 10px Ahem; color: green; }
>> </style>
>>
>> <div>aaaaa</div>
>> <p>There must be a green box above.</p>
>
> I tried to put together a better example. This is self-describing (when
> the browser doesn't have the Ahem font installed), and tests some
> functionality that couldn't be covered using Web APIs. One could argue
> that most of the functionality could be covered with reftests, but
> perhaps we care about the render tree.
>
>
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> docs/testing/writing_layout_tests.md:715: baseline would have depended
> on the fonts installed on the testing computer, and
> On 2016/11/22 21:53:11, eae wrote:
>> Using Ahem does *not* guarantee that there won't be platform
> differences. It
>> helps *minimize* them for specific runs of text.
>>
>> In this case the test should disable the text output and only compare
> the pixel
>> results as the render-tree dump is irrelevant. There are very few
> cases where
>> having a render-tree text dump is relevant for pixel tests. I'd like
> to see the
>> guideline discourage it.
>>
>>
>
> I thought the guideline discourages it? Relevant text cited below.
>
> "for these reasons, DRT tests should **only** be used to cover aspects
> of the
> layout code that can only be tested by looking at the render tree. Any
> combination of the other test types is preferable to a DRT test."
>
> https://codereview.chromium.org/2492733003/

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

pwnall

I made a few more adjustments and responded to some points. I tried to remove ...

4 years ago (2016-11-28 23:51:33 UTC) #51

I made a few more adjustments and responded to some points. I tried to
remove the quoted lines that don't pertain to these responses. I hope
I did a good job. If anything looks out of place in the conversation,
please assume that I messed up and accept my apologies.

On Fri, Nov 25, 2016 at 9:06 AM, Emil A Eklund <eae@chromium.org> wrote:
> >
> >
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> > docs/testing/writing_layout_tests.md:107: * Prefer JavaScript's
> > On 2016/11/22 21:53:11, eae wrote:
> >> Why? The google3 js style guide makes the exact *opposite*
> > recommendation.
> >
> > FWIW, the google3 style guide changed completely since last time I read
> > it. It might have changed today, so perhaps you're referring to an old
> > version? The new guide doesn't seem to take any position on ==.
>
> Good point, we used to disallow === while now we're silent on it. We
> should probably be too.

Our current Blink guide says that == is OK when it is sufficient.
https://www.chromium.org/blink/coding-style/layout-test-style-guidelines#TOC-...

However, assert_equals in testharness.js uses ===, so I think it'd be
quite confusing to default to == in our own code.
https://github.com/w3c/testharness.js/blob/7bc5fb0eca7ccf15e3fbaa69d527e53297...
https://github.com/w3c/testharness.js/blob/7bc5fb0eca7ccf15e3fbaa69d527e53297...

Also, WebKit's docs subtly encourage ===, by using it in an example
where both === and == would have worked.
https://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

That being said, I know that I'm biased towards ===, as I'd much
rather debug a test that uses ===, so I admit I didn't look for
arguments against it. I assume that blink-dev will have some.

> >> The majority of comparisons in layout tests are for pixel values where
> > this is
> >> bad advice given that blink uses integers or layout units internally
> > and js uses
> >> floats.
> >>
> >> More importantly, stylistic rules should be governed by the style
> > guide, not
> >> this document.
> >
> > This guide uses "layout tests" to refer to everything under
> > WebKit/LayoutTests. The tests I've written (File API, IndexedDB, drag
> > and drop) compare strings / booleans / null roughly as much as they
> > compare numbers.
> >
> > That being said, I'm not sure why == differs from === when both sides of
> > the comparison are numbers. Can you please explain?
>
> a === b is effectively translates to typeof(a) == typeof(b) && a == b.
> The reason we used to discourage it in google3 was that we wanted to
> make it *very* clear when something unusual was going on that required
> that level of comparisons. For the majority of comparisons == is
> perfectly fine and functionally equivalent.
>
> For pixel values we shouldn't use direction comparison *at all* and
> instead use isNear or equivalent to allow for floating point
> imprecisions and rounding.

I assume that we'd compare pixel values using assert_approx_equals and
other testharness.js assertions most of the time, and == / === would
only come into play for control flow logic.

I added a recommendation to use the assert_ functions instead of
assert + a manual comparison whenever possible.

> >
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> > docs/testing/writing_layout_tests.md:145: * Tests should prefer **modern
> > features** in JavaScript and in the Web Platform.
> > On 2016/11/22 21:53:12, eae wrote:
> >> These guidelines are in conflict with the *cross-platform* and
> > *minimal*
> >> sections and doesn't match the existing tests.
> >
> > These guidelines are for new tests.
> >
> > cross-platform: The features that I mentioned here are shipped in all
> > major browsers.
> > minimal: Based on your feedback, clarified *minimal* to mean minimal
> > burden on reader / maintainer. Based on my Web development experience,
> > all the features I cited here (use strict, let / const, classes,
> > promises) translate into code that is easier to review and maintain.
> >
> >> var is used over 50 *fifty* times more than let. Are we also going to
> > rewrite
> >> all our existing tests? If not I'd argue that consistency is way more
> > important.
> >> We can't have 90% of our tests fail the guidelines.
> >
> > We don't have any rules currently, so we have a mix of styles. I think
> > that the path towards consistency starts with setting guidelines for new
> > tests, and continues with gradually upgrading old tests. I believe this
> > is how Google3's style guides started too, as the C++ guide specifically
> > mentions the existence of old code that doesn't obey the rules.
> >
> > I think const and let are the right thing to do for new tests, as their
> > semantics enhance readability in at least two ways - (1) the variables
> > are defined at block-level, and are only valid after they are declared
> > (no hoisting), and (2) const vs let communicates mutability intent and
> > makes it easier to reason about the values that a variable can take.
>
> I strongly disagree. If you want to change our style guide please go
> though the proper channels and start the discussion there. Please
> don't try to shoe-horn in style guide changes here.
> We want to minimize the friction to writing tests, mandating the use
> of completely irrelevant features seems counter productive at best.

I think we actually agree here, and I'm sorry for being unclear in my
review request! This writeup will be shown to blink-dev and discussed
there. I think that's the widest forum that would be interested in
such a discussion. I don't have a delusion of being able to push any
sort of style rules on anyone.

I asked for a few targeted initial reviews to remove any glaring
errors in the document. Thank you very much for your help here!

> >
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> > docs/testing/writing_layout_tests.md:179: * Modern Web Platform and
> > JavaScript features should be preferred to legacy
> > On 2016/11/22 21:53:11, eae wrote:
> >> Again, why?
> >
> > Please see above :)
>
> I'm sorry but you personal preferences isn't reason enough overrule
> the style guide and the TTWF recommendations. I'm fine with *allowing*
> es6 features. I'm *not* fine with *mandating* it.

Done, I think?

This writeup aims to recommend some defaults. As such, I tried to use
should everywhere, as opposed to must. I used "prefer" instead of
stronger wording like "always use X instead of Y" for the same reason.
This was intentional, as I think it's fine to deviate from
recommendations when there is a good reason to do so. You have more
context in your head than could fit in a Markdown document :)

I just realized that I accidentally used must in the bullet point that
we're discussing here, and I replaced it with should. I also added a
note clarifying that should and must are used as in RFC 2119, so
readers wouldn't assume that should was intended to mean must. AFAIK,
the only legitimate use of "must" is in the self-contained principle,
because layout tests would ideally run without Internet access. Thank
you very much for helping me catch this bug!

FWIW, cross-browser compatibility does not appear to be a good reason
to avoid any of the specific ES6 constructs recommended above. They
are all supported in all major browsers that are still under
development, so using them does not decrease WPT's reach.

> >
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> > docs/testing/writing_layout_tests.md:628: * use the [Ahem
> > font](https://www.w3.org/Style/CSS/Test/Fonts/Ahem/README) to
> > On 2016/11/22 21:53:12, eae wrote:
> >> Unless text, text flow, font selection, font fallback, font features
> > or other
> >> typographic information is part of the test.
> >
> > Good point. I added this to the writeup. Thank you very much!
> >
> >> Also it doesn't really do much to minimize platform differences, at
> > least not
> >> for us, so I'm not sure this is still good advice.
> >
> > FWIW, I recently wrote a test that generates a pixel result. The result
> > was the same for all platforms when I could use Ahem, and I had to use
> > per-platform expectations when I couldn't use Ahem. So, I have data
> > showing that it still adds value.
>
> Then you were very lucky or wrote a very good test! Either case I'd be
> curious to see it, perhaps you found a way to dela with the
> differences that we could all learn from!

This is the CL: https://codereview.chromium.org/2441023003/

It produces an image result that holds the feedback ("ghost") for a
drag and drop operation. When the feedback is a DOM element, there's a
single image baseline, because I used Ahem. When the feedback is
pieced together by Blink using some special-case code, we need
per-platform baselines.

> >> It trades readable self-describing *fast* tests into
> > hard-to-reason-about test
> >> that have to synchronously load a font from an array buffer using js.
> >
> > This is a very good point! Thank you very much for bringing it up!
> >
> > How about (me submitting a CL for) modifying ahem.js to only load the
> > Ahem font when window.testRunner is detected? I effectively did this
> > (manually, via CSS classes) in a test I wrote.
>
> We really do want tests to behave the same way when run in content
> shell or a browser, makes debugging a lot easier. We used to try to
> side-load Ahem into testrunner to avoid the perf costs and changes it
> to use ahem.js specifically to address the debug-ability problem.

I can't think of a better way to get the benefits of Ahem and readable
text at the same time :( As Ahem is recommended in the previous
documentation (WebKit, TTWF), so I'd rather leave it in and have a
discussion.

https://webkit.org/writing-new-tests/
http://testthewebforward.org/docs/test-style-guidelines.html

FWIW, tests that use test-only APIs like testRunner have two code
paths anyway -- one is used when the testing APIs are available, while
the other path has the test gracefully degrade into a manual test when
the testing APIs are not available.

> >
https://codereview.chromium.org/2492733003/diff/300001/docs/testing/writing_l...
> > docs/testing/writing_layout_tests.md:659: A Dump Render Tree test
> > renders a web page and produces two results, which are
> > On 2016/11/22 21:53:11, eae wrote:
> >> A DRT test can produce either text result, pixel results, or both.
> >
> > I've been trying to use the DRT term to only label tests that produce a
> > render tree dump. Can we use the term pixel tests for the tests that
> > only produce pixel results? It seems clearer.
> >
> > I think I was misguided by a WebKit page.
> >
https://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree
> >
> > It says "How to disable the pixel dumps but keep the render tree dump
> > There is currently no way for you to do that."
> >
> > If there's a way to produce a render tree but not a pixel result, can
> > you please tell me how it works? OR, if you prefer, I can leave a TODO
> > in the file, and you can update it.
>
> I'm pretty sure there is a set of flags for that, please add a TODO
> and I'll fill it out.

TODO added. Thank you for volunteering to help!

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

spark

Final grammar review. https://codereview.chromium.org/2492733003/diff/360001/docs/testing/writing_layout_tests.md File docs/testing/writing_layout_tests.md (right): https://codereview.chromium.org/2492733003/diff/360001/docs/testing/writing_layout_tests.md#newcode56 docs/testing/writing_layout_tests.md:56: system settings. For this reason, it ...

4 years ago (2016-11-29 00:57:48 UTC) #52

pwnall

spark: Thank you very much for the feedback! https://codereview.chromium.org/2492733003/diff/360001/docs/testing/writing_layout_tests.md File docs/testing/writing_layout_tests.md (right): https://codereview.chromium.org/2492733003/diff/360001/docs/testing/writing_layout_tests.md#newcode56 docs/testing/writing_layout_tests.md:56: system ...

4 years ago (2016-11-29 01:19:23 UTC) #53

pwnall

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org, rbyers@chromium.org Link to ...

4 years ago (2016-11-29 01:19:33 UTC) #55