| Summary: | Replaces Yin-Yang character with a thumb-nail image in two layout tests | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jungshik Shin <jshin> | ||||||||||
| Component: | Tools / Tests | Assignee: | Jungshik Shin <jshin> | ||||||||||
| Status: | RESOLVED WONTFIX | ||||||||||||
| Severity: | Normal | CC: | dglazkov, eroman, hyatt, ian, mitz | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | All | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Jungshik Shin
2009-01-22 12:53:04 PST
Created attachment 26940 [details]
patch
I added 'yinyang.png' to 'support' directory in css2.1 and made two tests refer to it in place of actual U+262F (Yin Yang symbol).
I think there is a policy of refraining from modifying tests in the css2.1 directory, because they have an external source. Instead, an equivalent test, with the proposed modifications, can be created in a different directory. Thank you for the info. What directory would be best for putting modified versions of two tests?
There are other css tests with similar issues (e.g. tests with arrows in various directions). Aha, LayoutTests/platform/win/css2.1/resources has Mac-compatible-font-fallback.css and two html files (run-webkit-tests-{prologue,epilogue}.html) to work around them. However, that trick makes those tests rely on an unrelated feature (CSS @font-face with unicode range). I think it's better to avoid that dependency if possible.
The names suggest that those are float tests, so fast/block/float may be a good place for them. Comment on attachment 26940 [details]
patch
Setting flag to review- based on the comments in this bug.
Making an equivalent test you can run when you lack a font with that symbol seems fine -- it seems slightly silly to add an actual "yin/yang" image, but probably OK.
Created attachment 27078 [details]
patch with tests in fast/block/float
I copied the following 3 tests to fast/block/float (as suggested) and modified the copies to use png images (YinYang symbol and SE arrow) and '<-' (Leftward white arrow).
css2.1/t0905-c414-flt-fit-01-d-g.html
css2.1/t0905-c5525-fltblck-00-d-ag.html
css2.1/t0905-c5526-flthw-00-c-g.html
Comment on attachment 27078 [details]
patch with tests in fast/block/float
Seems OK. r=me
Landed as http://trac.webkit.org/changeset/41393. The patch that was landed included a bunch of *.rej and *.orig files. I removed them in http://trac.webkit.org/changeset/41405 . Dimitri, try adding those to your svn global-ignores list. :) Hyatt is complaining that the results checked in are wrong. I'm not sure we even want to fix this bug this way. Why don't we just add some fonts w/ this symbol to the ones which test_shell uses for running the tests? It seems silly to hack our tests to work around Windows lame fonts. My apologies. I had to hand-roll this patch (as you can see from the diff, the copying of the files wasn't done correctly), and I didn't notice the merge artifacts. Let me know what you guys decide and I'll make it right. (In reply to comment #10) > Hyatt is complaining that the results checked in are wrong. > > I'm not sure we even want to fix this bug this way. Why don't we just add some > fonts w/ this symbol to the ones which test_shell uses for running the tests? > It seems silly to hack our tests to work around Windows lame fonts. On the other hand, these tests were written for visual inspection (by Ian) but I don't think they're suitable for automated pixel tests. For automated pixel tests, we should make them NOT depend on anything other than what's actually tested. And, my patch tried to achieve 'that independence'. The test failure is due to the svn-copied tests (html files) got landed without being revised. It seems that 'svn-apply' does not work well when I 'svn-copy foo bar', revise 'bar' and generate 'svn-create-patch'. Or did I do something wrong (as Dimitri wrote)? Anyway, Dave told me on IRC that it's better to revise the original tests in place (as opposed to copy them and put revised versions in a new location). So, I'll make a new patch to do that. Created attachment 28288 [details] patch with in-place modifications of css2.1 tests Instead of duping css2.1 tests to a new place and revising them there, this patch modifies the original tests in place. It also removes the previously added tests in fast/block/float. Somehow, png files added in attachment 27078 [details] that are removed in this patch are not recognized as binary and diff are not base64-encoded in the patch. (new png files in css2.1/supports are recognized as image/png and base64-encoded). However, running 'svn-unapply' and 'svn-apply' multiple times work. Also after each 'svn-apply', webkit test passes. Where do these tests come from? Can we get them "fixed" upstream instead? They come from CSS 2.1 test suite. Ian, what do you think of these changes? With actual characters replaced by thumbnail images, 'human testers' would still know what to look for and visual inspection wouldn't be affected, which you're worried about when I proposed removing those characters altogether (instead of replacing with thumbnail images). BTW, this is http://crbug.com/2304 (chromium bug 2304). Among 5 tests deferred (see http://crbug.com/2304#c11), all but css2.1/t0805-c5519-brdr-r-01-e.html are taken care of by the latest patch here. Created attachment 28371 [details] patch same as the previous but without the roll-out of r41393 (assuming that'll be done separately) This is the same as attachment 28288 [details], but does not contain the patch for removing files added in r41393 ( http://trac.webkit.org/changeset/41393). This can be used if r41393 is rolled out before I get r+ for attachment 28288 [details]. Perhaps, this is safer and makes everybody happy sooner :-). I don't think we want this change. I think that Chromium should either include some fonts when running test_shell which include this symbol (which is what Apple does when running DumpRenderTree on windows), or that you should lobby the CSS working group to get a similar change in the upstream test suite. This is minimal value, for possibly a lot of pain trying to keep this diff if we later update our copy of the CSS 2.1 tests. Comment on attachment 28288 [details]
patch with in-place modifications of css2.1 tests
See my comments above. I don't think we want this change.
Sam rolled out the previous commit earlier today: http://trac.webkit.org/changeset/41539 (In reply to comment #18) > ...Chromium should either include > some fonts when running test_shell which include this symbol (which is what > Apple does when running DumpRenderTree on windows) Can you be more specific about how DRT calls up fonts that aren't installed on the system? Are they distributed along with DRT, and if so, where did the fonts come from (i.e. what licensing terms, and are they in the tree)? Trying to figure out how we can remove this font dependency, which really just gets in the way, without having to lose coverage of what they're intending to be testing. Changing the CSS 2.1 tests is the best solution, but it may take a long time. (In reply to comment #21) > Can you be more specific about how DRT calls up fonts that aren't installed on > the system? Are they distributed along with DRT, and if so, where did the fonts > come from (i.e. what licensing terms, and are they in the tree)? Trying to > figure out how we can remove this font dependency, which really just gets in > the way, without having to lose coverage of what they're intending to be > testing. Changing the CSS 2.1 tests is the best solution, but it may take a > long time. Sadly, the solution for DRT isn't particularly elegant either. WebKit.org does not distribute the fonts. however the Apple (and qt) versions of DRT require the set of ttf fonts distributed with Safari for Windows. Individual contributers find these fonts in their own ways, and install them into a directory pointed to by WEBKIT_TEST_FONTS and drt takes care of loading them into process: http://trac.webkit.org/browser/trunk/WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp#L67 |