Bug 118599

Summary: Replace MathML pixel tests by reftests
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: MathMLAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: abrhm, buildbot, cdumez, cfleizach, commit-queue, dbarton, gyuyoung.kim, mrobinson, rakuco, rniwa, zalan
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 118812    
Attachments:
Description Flags
Patch V1
none
Patch V1 - bis
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion
none
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2
none
Patch V2
none
Patch V2 - bis
buildbot: commit-queue-
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion
none
Patch V3
none
Patch V4
cfleizach: review+, commit-queue: commit-queue-
Patch Final Version
none
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 none

Frédéric Wang (:fredw)
Reported 2013-07-12 05:56:08 PDT
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).
Attachments
Patch V1 (110.82 KB, patch)
2013-07-12 06:03 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch V1 - bis (309.66 KB, patch)
2013-07-12 06:09 PDT, Frédéric Wang (:fredw) 		buildbot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion (571.41 KB, application/zip)
2013-07-12 20:27 PDT, Build Bot 		no flags
Details
Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 (493.00 KB, application/zip)
2013-07-13 06:10 PDT, Build Bot 		no flags
Details
Patch V2 (196.84 KB, patch)
2013-07-16 03:41 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch V2 - bis (511.62 KB, patch)
2013-07-16 03:57 PDT, Frédéric Wang (:fredw) 		buildbot: commit-queue-
DetailsFormatted DiffDiff
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion (994.00 KB, application/zip)
2013-07-16 04:43 PDT, Build Bot 		no flags
Details
Patch V3 (511.78 KB, patch)
2013-07-16 06:13 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Patch V4 (517.92 KB, patch)
2013-07-16 12:34 PDT, Frédéric Wang (:fredw) 		cfleizach: review+
commit-queue: commit-queue-
DetailsFormatted DiffDiff
Patch Final Version (517.88 KB, patch)
2013-07-16 15:55 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2 (487.86 KB, application/zip)
2013-07-16 20:51 PDT, Build Bot 		no flags
Details
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.
Frédéric Wang (:fredw)
Comment 1 2013-07-12 06:03:14 PDT
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)
Comment 2 2013-07-12 06:09:33 PDT
Created attachment 206532 [details] Patch V1 - bis
Build Bot
Comment 3 2013-07-12 20:27:46 PDT
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
Comment 4 2013-07-12 20:27:48 PDT
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 5 2013-07-13 06:10:12 PDT
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
Comment 6 2013-07-13 06:10:16 PDT
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)
Comment 7 2013-07-16 03:41:36 PDT
Created attachment 206749 [details] Patch V2
Frédéric Wang (:fredw)
Comment 8 2013-07-16 03:57:29 PDT
Created attachment 206752 [details] Patch V2 - bis
Build Bot
Comment 9 2013-07-16 04:43:36 PDT
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
Comment 10 2013-07-16 04:43:39 PDT
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)
Comment 11 2013-07-16 06:13:52 PDT
Created attachment 206771 [details] Patch V3
Frédéric Wang (:fredw)
Comment 12 2013-07-16 06:58:37 PDT
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 13 2013-07-16 09:01:49 PDT
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)
Comment 14 2013-07-16 10:09:21 PDT
(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)
Comment 15 2013-07-16 12:34:57 PDT
Created attachment 206806 [details] Patch V4
chris fleizach
Comment 16 2013-07-16 12:45:43 PDT
Comment on attachment 206806 [details] Patch V4 Thanks
WebKit Commit Bot
Comment 17 2013-07-16 15:41:49 PDT
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)
Comment 18 2013-07-16 15:55:24 PDT
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 19 2013-07-16 20:51:46 PDT
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
Comment 20 2013-07-16 20:51:50 PDT
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 21 2013-07-17 02:15:38 PDT
Comment on attachment 206821 [details] Patch Final Version Clearing flags on attachment: 206821 Committed r152777: <http://trac.webkit.org/changeset/152777>
WebKit Commit Bot
Comment 22 2013-07-17 02:15:42 PDT
All reviewed patches have been landed. Closing bug.
Frédéric Wang (:fredw)
Comment 23 2013-07-17 02:19:03 PDT
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.
Note You need to log in before you can comment on or make changes to this bug.