834737 - 2 text reftests fail while running unitests on ubuntu 12.04 on ec2 VMs

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

we are working on scaling our test infrastructure and this includes virtual machines on amazon ec2.  We have tests running and a few are failing.  There are two text related reftests which fail:

log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=19103276&tree=Cedar

tests:
13:51:40     INFO -  REFTEST TEST-UNEXPECTED-PASS | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/text/475092-pos.html | image comparison (==)
13:52:08     INFO -  REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/text-decoration/underline-button-2.html | image comparison (==), max difference: 216, number of differing pixels: 58

tbpl view for reftest analyzer:
https://tbpl.mozilla.org/?tree=Cedar&rev=706f8a9ee780

Comment 1

[Karl Tomlinson (:karlt)]

underline-button-2 already fails-if(winWidget). "the underline spacing inside the button is different" sounds like the same thing.

Perhaps using the same font inside and outside the button might help, but I'm surprised if the underline spacing for a button can come from the font outside the button.

Comment 2

[Karl Tomlinson (:karlt)]

The difference in 475092-pos may be due to bytecode/autohinter differences (Bug 482596 comment 12).  If we switch atomically, it can be marked as passing I guess.
Otherwise, random-if(gtk2Widget).

Comment 3

[Boris Zbarsky [:bzbarsky]]

So the issue is basically that in one case the button is drawing the underline while in the other case the <span> is, iirc.  They're drawing it on the same text but for some reason end up with differences in the underline vertical position...

And yes, the Windows issue was the same.

We should definitely try forcing the same font inside and outside the button to see if that hepls.

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

how can I force the font inside and outside the button?  Is there a pref or a change to the test I can do?  If this doesn't work I can fix with manifests for gtk2Widget.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Just setting font-family and font-size to the same value (e.g. 'sans-serif') on both the button and the <span> in the test should work.

So toss style="font: 16px sans-serif" on both elements?

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

this style fix resolves the case for the file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/text-decoration/underline-button-2.html test!

:karlt, should we mark the file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/text/475092-pos.html as random-if until we can only run on ubuntu vms instead of fedora machines?

Comment 7

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: fix the two problems outlined in this test (1.0)

1) the underline-button problem is fixed as per the suggestion
2) 475092-pos.html is random-if(gtk2Widget)

We should land this bug and [leave open] until we switch to ubuntu for all reftests.  This will allow us to remove the random-if statement.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> this style fix resolves the case for the underline-button-2.html test!

Excellent.   Maybe it fixes on Windows too, so we can enable the test there?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 709833 [details] [diff] [review]
fix the two problems outlined in this test (1.0)

r=me

Comment 10

[Karl Tomlinson (:karlt)]

Setting font family and size for the button made underline-button-2 pass on WINNT.
https://tbpl.mozilla.org/?tree=Try&rev=57a4eeebffcb

Comment 11

[Karl Tomlinson (:karlt)]

(In reply to Joel Maher (:jmaher) from comment #6)
> :karlt, should we mark the
> file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/text/
> 475092-pos.html as random-if until we can only run on ubuntu vms instead of
> fedora machines?

(In reply to Joel Maher (:jmaher) from comment #7)
> We should land this bug and [leave open] until we switch to ubuntu for all
> reftests.  This will allow us to remove the random-if statement.

Sounds good, thanks.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/178c6a84dcab

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: mark random-if reftest from previous patch as fails-if (1.0)

now that we are running reftests on ubuntu VM's and not on Fedora machines anymore, we can mark this as fails-if to ensure quality coverage.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 720002 [details] [diff] [review]
mark random-if reftest from previous patch as fails-if (1.0)

r=me

Comment 15

[Ed Morley [:emorley]]

Backed out for:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=491d60eb44bf&jobname=reftest
05:29:12 INFO - REFTEST TEST-UNEXPECTED-PASS | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/text/475092-pos.html | image comparison (==)


https://hg.mozilla.org/integration/mozilla-inbound/rev/9b52f3a5b53b

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: mark test as passing since this passes on linux (2.0)

this test continually passes on the ubuntu testers, so lets mark it as passing!

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 725006 [details] [diff] [review]
mark test as passing since this passes on linux (2.0)

r=me

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/337e4c8c7b25