Bug 118599 – Replace MathML pixel tests by reftests

• Status: RESOLVED
• Resolution: FIXED
• Component: MathML

[Frédéric Wang (:fredw)]

As indicated in

http://trac.webkit.org/wiki/Writing%20Layout%20Tests%20for%20DumpRenderTree

and as I explained in bug 118053 comment 7, using pixel tests is too fragile especially for MathML where the rendering is not defined precisely (as opposed to e.g. SVG or CSS). Basically, if we start to change anything like fixing bug 115787 or the patch Martin Robinson is working on for stretchy operators, that will just break all the MathML tests. Hence one has to verify the tests by eye and regenerate the references again which is error-prone and a bit in contradiction with the goal of automated regression tests. Moreover it is not always clear what's the tests are verifying (position of scripts? sizes? spacing? font style? everything at the same time?) and currently references seem to only available on GTK, EFL and Mac (so I guess the other platforms just load the pages without verifying against a references?)

I suggest to replace them by reftests. We won't be able to test the subjective fact that a formula is "good-looking" and the test pages may be somewhat less "natural". But at least they become easy to verify (the two pages should just match / not match), they allow to restrict the unit test to only the minimal feature you want to check and ignore unrelated stuff, they won't break so easily and will be more cross-compatible (in particular no need to generate the references for each platform).

[Frédéric Wang (:fredw)]

Created attachment 206530 [details]
Patch V1

A first version that converts some of the tests.

For scripts: I assume all what we want to test is that the scripts are centered above/below the base and that they are drawn smaller (although I don't think the scriptlevel stuff is implemented correctly in WebKit). There seems to be a test for largeop in inline mode (displaystyle=false) for which under/overscripts should be placed as sub/supscripts but that does not seem to work in WebKit. I've created a separate test for bug 95404.

For tokens: I kept the testing of regular/italic. WebKit does not implement any MathML spacing yet nor does it draw quotes around ms, so it's not worth trying to do something more complicated.

[Frédéric Wang (:fredw)]

Created attachment 206532 [details]
Patch V1 - bis

[Build Bot]

Comment on attachment 206532 [details]
Patch V1 - bis

Attachment 206532 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1064249

New failing tests:
mathml/presentation/attributes3.html

[Build Bot]

Created attachment 206588 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 206532 [details]
Patch V1 - bis

Attachment 206532 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1045564

New failing tests:
mathml/presentation/attributes3.html

[Build Bot]

Created attachment 206604 [details]
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-10  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Frédéric Wang (:fredw)]

Created attachment 206749 [details]
Patch V2

[Frédéric Wang (:fredw)]

Created attachment 206752 [details]
Patch V2 - bis

[Build Bot]

Comment on attachment 206752 [details]
Patch V2 - bis

Attachment 206752 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1089173

New failing tests:
mathml/presentation/bug97990.html
mathml/presentation/scripts4.html

[Build Bot]

Created attachment 206763 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Frédéric Wang (:fredw)]

Created attachment 206771 [details]
Patch V3

[Frédéric Wang (:fredw)]

Comment on attachment 206771 [details]
Patch V3

OK, the new MathML tests pass now. I haven't converted everything yet but this patch is already big so I'll finish that in a follow-up bug.

[chris fleizach]

Comment on attachment 206771 [details]
Patch V3

View in context: https://bugs.webkit.org/attachment.cgi?id=206771&action=review

> LayoutTests/mathml/presentation/attributes1-expected-mismatch.html:12
> +	<mn>1</mn>

bad indendation

> LayoutTests/mathml/presentation/attributes1.html:1
> +<!doctype html>

can we make the names more descriptive, so if something fails the title will give away what should be looked at
so this would be attributes-style.

> LayoutTests/mathml/presentation/attributes2.html:1
> +<!doctype html>

attributes-background-color.html

> LayoutTests/mathml/presentation/attributes2.html:14
> +      <math>

bad indentation of <math> in comparison to children tags

> LayoutTests/mathml/presentation/attributes2.html:29
> +      <math mathbackground="orange">

ditto

> LayoutTests/mathml/presentation/attributes3.html:1
> +<!doctype html>

attributes-mathsize.html

> LayoutTests/mathml/presentation/attributes4-expected.html:14
> +	 "U+1D468 MATHEMATICAL BOLD ITALIC CAPITAL A") but WebKit implementation

but "the" WebKit imple...

> LayoutTests/mathml/presentation/attributes4.html:2
> +<html>

attributes-mathvariant.html

> LayoutTests/mathml/presentation/attributes4.html:10
> +	 single-char mi elements and "normal" otherwise. -->

single-char <mi> elements

> LayoutTests/mathml/presentation/attributes5-expected.html:9
> +    <!-- This only draws the background boxes since the x is hidden. -->

what X are you referring to here? are you referring to the content inside the tag?

> LayoutTests/mathml/presentation/attributes5.html:3
> +  <head>

attributes-display.html

> LayoutTests/mathml/presentation/attributes5.html:27
> +		  background: blue;">

ditto for indendation

> LayoutTests/mathml/presentation/bug95015-expected.html:10
> +    <!-- The msubsup should be in supscript and so hidden by the red

should be in "a" subscript, so "it should be" hidden by..

> LayoutTests/mathml/presentation/bug95015.html:10
> +    <!-- The msubsup should be in supscript and so hidden by the red

ditto for comment

> LayoutTests/mathml/presentation/bug95404-expected.html:16
> +		  background: red; width: 100%;">

are you wrapping to 80 chars for every line? it seems like a lot of these lines would be easier to read on one line

> LayoutTests/mathml/presentation/scripts1.html:2
> +<html>

this probably deserves two tests
scripts-underover
scripts-supsub

> LayoutTests/mathml/presentation/scripts1.html:208
> +	   not currently implement scriptlevel and only sets the size to

bug number?

> LayoutTests/mathml/presentation/scripts2.html:2
> +<html>

scripts-fontsize

[Frédéric Wang (:fredw)]

(In reply to comment #1)
> Created an attachment (id=206530) [details]
> Patch V1
> 
> A first version that converts some of the tests.
> 
> For scripts: I assume all what we want to test is that the scripts are centered above/below the base and that they are drawn smaller (although I don't think the scriptlevel stuff is implemented correctly in WebKit). There seems to be a test for largeop in inline mode (displaystyle=false) for which under/overscripts should be placed as sub/supscripts but that does not seem to work in WebKit. I've created a separate test for bug 95404.
>

(In reply to comment #13)
> > LayoutTests/mathml/presentation/scripts1.html:208
> > +	   not currently implement scriptlevel and only sets the size to
> 
> bug number?

I've opened bug 118737 and bug 118738 for these issues.

[Frédéric Wang (:fredw)]

Created attachment 206806 [details]
Patch V4

[chris fleizach]

Comment on attachment 206806 [details]
Patch V4

Thanks

[WebKit Commit Bot]

Comment on attachment 206806 [details]
Patch V4

Rejecting attachment 206806 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-03', 'validate-changelog', '--check-oops', '--non-interactive', 206806, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.appspot.com/results/1080334

[Frédéric Wang (:fredw)]

Created attachment 206821 [details]
Patch Final Version

Oops, once again I've badly updated the ChangeLog after having addressed the review comments (I called prepare-Changelog and updated the file by hand, but I'm wondering if there is a better way to update the list of modified files)

[Build Bot]

Comment on attachment 206821 [details]
Patch Final Version

Attachment 206821 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1110039

New failing tests:
fullscreen/full-screen-iframe-with-max-width-height.html

[Build Bot]

Created attachment 206840 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[WebKit Commit Bot]

Comment on attachment 206821 [details]
Patch Final Version

Clearing flags on attachment: 206821

Committed r152777: <http://trac.webkit.org/changeset/152777>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Frédéric Wang (:fredw)]

Note: this adds expected-mismatch reftests that are likely to fail on Qt bots and should be skipped:

+        * mathml/presentation/attributes-style-expected-mismatch.html: Added.
+        * mathml/presentation/scripts-font-size-expected-mismatch.html: Added.