Bug 21407 – Add a number of new layout tests, all passing on ToT

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Pam Greene (IRC:pamg)]

Attached are 13 new layout tests, all of which pass in a ToT (r37267) build. I'll also be generating expected results (separately for Tiger and Leopard, where needed), but I wanted to be sure the tests were finalized before fussing with that.

[Pam Greene (IRC:pamg)]

Created attachment 24118 [details]
13 new layout tests, no results yet

Comment on attachment 24118 [details]
13 new layout tests, no results yet

+        * css1/box_properties/border-height.html: Added.
+        * css2.1/overflow_hidden.html: Added.

The css1 and css2.1 are supposed to be reserved for external test suites.

[Pam Greene (IRC:pamg)]

Shall I move those two into fast/css/ then?

(In reply to comment #3)
> Shall I move those two into fast/css/ then?

Maybe fast/overflow for the second one.

+     * http/tests/loading/crash-multiple-family-fontface.html: Added.

Is that a test for <http://trac.webkit.org/changeset/30392>?

[Eric Seidel (no email)]

I wonder if some of these aren't already in LayoutTests/ under different names?  Things like bad entity, and multiple font families and recurisve XSLT includes seem like  things we have tested before.

[Pam Greene (IRC:pamg)]

(In reply to comment #4)
> +     * http/tests/loading/crash-multiple-family-fontface.html: Added.
> 
> Is that a test for <http://trac.webkit.org/changeset/30392>?

The test case is slightly different. Hard for me to say whether it's the same underlying cause. My guess is no, since fast/css/font-face-multiple-remote-sources.html (added in r30392) is 3 months older than when crash-multiple-family-fontface.html was added to Chromium's set.  If there had been a test covering our bug, we would have noticed that we were crashing on it.

((In reply to comment #5)
> I wonder if some of these aren't already in LayoutTests/ under different names?
>  Things like bad entity, and multiple font families and recurisve XSLT includes
> seem like  things we have tested before.

I looked for dupes, but renames are harder to spot.  But if there are already tests covering these things, they must be pretty new, since Chromium only added tests when we had bugs that existing tests hadn't revealed.  So if you're remembering things from way back, they're probably similar but not the same.

[Pam Greene (IRC:pamg)]

Created attachment 24133 [details]
Another new test that already passes

Missed this one when collecting them.  Again, once the tests are settled I'll create expected results and prepare a real, complete patch for final review and submission.

[Pam Greene (IRC:pamg)]

Created attachment 24135 [details]
One more new test that probably passes

...and one more.  Actually I'm not 100% certain this one is passing, although it looks basically the same here as in both Firefox 2 and 3. 

The issue here is that the width of a textarea that uses a variable-width font should be computed using a robust and reasonable metric, e.g. the average width of a character, rather than the width of some arbitrary character. If someone who knows where the related code is could either check it or point me to it, it'd be reassuring to verify bottom-up (from the code) as well as top-down (from the test result).

(In reply to comment #8)
> Created an attachment (id=24135) [edit]
> One more new test that probably passes
> 
> The issue here is that the width of a textarea that uses a variable-width font
> should be computed using a robust and reasonable metric, e.g. the average width
> of a character, rather than the width of some arbitrary character. If someone
> who knows where the related code is could either check it or point me to it,
> it'd be reassuring to verify bottom-up (from the code) as well as top-down
> (from the test result).

Sounds related to bug 15312.

[Pam Greene (IRC:pamg)]

Comment on attachment 24135 [details]
One more new test that probably passes

> Sounds related to bug 15312.

Agreed. That one doesn't seem to be going anywhere recently, but I'll omit this test case in favor of whatever happens there.

[Darin Adler]

Comment on attachment 24118 [details]
13 new layout tests, no results yet

It's great to have more tests! These look fine.

I'm not sure exactly why you posted this patch for review. If you want to get the patch in the tree, then we need a patch that does include results; so I'll say review- on that basis.

If you want a discussion of the merits of the individual tests, then you need a patch with a smaller number of tests in it and some explanation of the rationale behind the tests to attract an appropriate reviewer for each test.

[Pam Greene (IRC:pamg)]

Created attachment 24199 [details]
14 tests + results

Thanks, I was primarily looking for just the sorts of comments I got: renaming suggestions and information about dupes.  I have a bunch more tests that might need more discussion that I'll be posting individual bugs about, too.  These are just the straightforward ones.

Comment on attachment 24199 [details]
14 tests + results

>         * fast/text/international/ideo_punc.html: Added.

Does that test cover more than fast/text/line-breaks-after-ideographic-comma-or-full-stop.html does? Perhaps they can be combined.

(In reply to comment #13)
> Does that test cover more than
> fast/text/line-breaks-after-ideographic-comma-or-full-stop.html does? Perhaps
> they can be combined.

See bug 15630 and bug 17411.

[Darin Adler]

Comment on attachment 24199 [details]
14 tests + results

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.

[Pam Greene (IRC:pamg)]

Darin, thanks for the review (as well as all the others you've been doing for my test patches recently), and for approving this one anyway. I understand and agree with your comments about good versus merely adequate tests, and I'll pass them along to the original authors of these. If it helps, think of it as importing a small test suite from an external source. ;)

These tests all check things that failed in some earlier version of Chrome. In most cases, I don't know whether the sources of the errors were in WebKit itself (maybe only on Windows) or in Chrome.

Whoever stops by, please hold off on checking this in.  I'll see about improving at least some of these tests first.

[Adam Barth]

Comment on attachment 24199 [details]
14 tests + results

Clearing the review flag per Pam's last comment.

[Pam Greene (IRC:pamg)]

Created attachment 24373 [details]
Improved tests + new results

Some of the issues raised for improving these tests are intractable at this point, but I did what I could.

All: wrote more informative descriptions about what's being tested and what to expect when looking at the results.

fast/css/border-height.html: this was a painting issue related to rounding in the graphics engine, so unfortunately only the pixel test is descriptive.

fast/dom/Window/orphaned-frame-access.html: improved the output

fast/forms/image-border.html: I can't tell for sure, but I believe this was also a painting issue.

fast/forms/input-field-text-truncated.html: unfortunately at this late stage I don't know whether it was a layout bug or a painting problem, so I don't know whether the render tree alone would catch the error.  Likewise, although I agree that using "em" is odd here, looking back at the bug report it was a very fiddly edge case that caused the (rounding error) bug to show up, so I'm reluctant to change anything about the font sizes for fear of making the test entirely useless.

fast/forms/input-type-text-min-width.html: the result here will be affected by whatever decision is made in bug 15312, but following the philosophy in bug 21419, it's worth adding the test now to catch unintentional changes anyway.

fast/forms/textarea-width.html: ditto

fast/parser/bad-xml-entity.xml: renamed from .html, and removed the encoding specification. The XML parser, unlike the HTML parser, produces a parsing error and would never see a dumpAsText() directive (I tried), so it's stuck as a render-tree test now.

http/tests/misc/crash-multiple-family-fontface.html: moved from http/tests/loading/

-- Tests removed from the patch:

fast/dom/DOMImplementation/singleton-modifications.html: I don't fully understand what this is testing, and I'm not yet convinced it checks what it intends to.  I've asked the original author for clarification and will post it in a separate bug later.

fast/text/international/ideo_punc.html: this is a dupe of fast/text/line-breaks-after-ideographic-comma-or-full-stop.html.

[Alexey Proskuryakov]

(In reply to comment #18)
> fast/parser/bad-xml-entity.xml: renamed from .html, and removed the encoding
> specification. The XML parser, unlike the HTML parser, produces a parsing error
> and would never see a dumpAsText() directive (I tried), so it's stuck as a
> render-tree test now.

This test comes directly from bug 17814. I think that it should remain HTML (as this is the form that was known to cause issues). Also, I'd prefer if "xml-entity" (in file name) and "xml tag" (in description) were renamed to "XML declaration", as this is how the "<?...?>" incantation is formally called.

[Pam Greene (IRC:pamg)]

Created attachment 24460 [details]
Better XML declaration tests

Fair enough.  I spoke to eroman, who reported bug 17814 and wrote the bad-xml-entity test, and he thought it would have shown up in a .xml file too.  But more coverage never hurts, so I've renamed the test to xml-declaration-missing-ending-mark.*, included both .html and .xml versions, and improved/expanded the text within the tests to both use the right terminology and explain why there are two.

No other changes from the previous patch.

[Alexey Proskuryakov]

Comment on attachment 24460 [details]
Better XML declaration tests

It would be cool if the xml version was inside the html one (an iframe?), to avoid having a mysterious test that has no explanation when opened in browser, but only an error text. That would make it text-only, too.

Please add a bug reference and short explanation to the ChangeLog.

> fast/frames/crash-remove-onload-from-src.html

I think it's bug 18174, and Brady was going to land a test that covered it according to a comment in that bug. So, the test can presumably be omitted.

> fast/xsl/xslt-nested-stylesheets.xml

This sounds like bug 15715, which is not fixed yet! The crash doesn't happen with the versions of libxslt shipped by Apple, but the issue is still present. Since the major block to landing a fix for bug 15715 was lack of a test case, someone can probably finish it now.

r=me if you omit these two tests for now, and fix the ChangeLog. Please also consider my comment about putting xml version of xml declaration test into an iframe.

[Pam Greene (IRC:pamg)]

Created attachment 24466 [details]
Patch as committed, addresses Alexey

Patch as landed.

(In reply to comment #21)
> It would be cool if the xml version was inside the html one (an iframe?)

Good idea. Done.

> Please add a bug reference and short explanation to the ChangeLog.

Oops. Done.

> > fast/frames/crash-remove-onload-from-src.html
> 
> I think it's bug 18174, and Brady was going to land a test that covered it
> according to a comment in that bug. So, the test can presumably be omitted.

Yup, looks like this is subsumed by fast/loader/frame-creation-removal.html.  Removed.

> > fast/xsl/xslt-nested-stylesheets.xml
> 
> This sounds like bug 15715, which is not fixed yet! The crash doesn't happen
> with the versions of libxslt shipped by Apple, but the issue is still present.
> Since the major block to landing a fix for bug 15715 was lack of a test case,
> someone can probably finish it now.

OK, removed. I'll look at finishing up that bug soon.  Jonathan Haas was another Chromium developer, but has mostly moved to other projects.

[Pam Greene (IRC:pamg)]

Landed as r37665.