[webkit-reviews] review granted: [Bug 21407] Add a number of new layout tests, all passing on ToT : [Attachment 24199] 14 tests + results

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Pam Greene <pam at chromium.org>'s
request for review:
Bug 21407: Add a number of new layout tests, all passing on ToT
https://bugs.webkit.org/show_bug.cgi?id=21407

Attachment 24199: 14 tests + results
https://bugs.webkit.org/attachment.cgi?id=24199&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
These tests are OK, but not consistently great. The quality varies.

Reviewing all 13 at once is tricky. I regret missing the opportunity to
communicate with the person who made each test to help them understand how to
make a great test.

Whenever possible we want a dumpAsText test, even for things when the test can
more easily be written to use a render tree dump. When that's not possible a
render tree dump is the next best thing. The worst case is a test where you can
only judge success or failure based on pixel test results. I can't tell which
of these tests fall into which category. For example, does the render-tree-dump
output of input-field-text-truncated.html actually show the success or failure
of the test?

Many of the tests have output that doesn't make it clear the test passed.

For example, border-height.html and image-border.html describes how the browser
should behave in an introductory paragraph, but they don't state what you
should see to visually determine the test has the correct result. So someone
looking at it can't easily know what to look for to see if it passed or failed.


The output of singleton-modifications.html simply says "true" in a frame named
"framey". Unclear to someone looking at the test whether that's success or
failure.

And while orphaned-frame-access.html does write out "PASSPASSPASS", nothing
indicates there will be three occurrences of the word "PASS" and I don't see
why they have to be all rammed up next to one another in a single big word.

And input-field-text-truncated.html isn't entirely clear on what the person
testing should look for.

It's not a good idea to use the "em" unit in the
input-field-text-truncated.html test. Is there a reason we need to do that
instead of using px? It's especially strange because the size of a font is a
mostly vertical thing an "em" is a horizontally-defined unit. So it makes the
test depend strangely on the design of the font.

input-type-text-min-width.html should have a newline at the end of the file.
And I have no idea how we're supposed to judge whether the test passed or
failed.

The hindi-spacing.html test contains nothing to say what it's supposed to test
or how to judge success vs. failure.

The bad-xml-entity.html test may not be in a good state. It's an XML file, but
the extension is .html, so it's going to be parsed as HTML. Is the intention to
test the HTML parsing or the XML parsing? It's a little strange to specify
iso-8859-2 as the encoding. If the test is indeed about the HTML parser
screwing up when passed an XML file, I think that's remarkable enough that the
test content should make some mention of that.

I assume many ofthese tests were things that failed either in older versions of
WebKit on other platforms or at perhaps in Chrome. I'm concerned that some of
the tests don't really test what they're intending to. Ideally we'd make sure
each one fails when the fix it corresponds to is rolled out, or at least that
it fails in, say, an old version of Safari.

Putting the crash-multiple-family-fontface.html test into the
http/tests/loading directory has an effect that may be unwanted. The tests in
that directory dump the frame delegate messages. It might be better to put the
test somewhere else in the http tests hierarchy so we test only the behavior of
@font-face without dumping the load delegate messages.

I'm going to say review+, because even flawed tests are good to add, as long as
there are no spurious failures. But I think there's room for improvement in
these tests.