Closed Bug 1307719 Opened 8 years ago Closed 8 years ago

Remove B2G and Mulet annotations from layout/reftests/bugs/reftest.list or replace them with Android

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: aryx, Assigned: aryx)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1307719 - Switch B2G-only test annotations in layout/reftests/bugs/reftest.list to Android and disable them until they get fixed.
58 bytes, text/x-review-board-request
dholbert
: review+
Details 
Spin-off from bug 1307332:

See bug 1307332 comment 113, 118 and 119:

"""
https://reviewboard.mozilla.org/r/83076/#review81850

I see, thanks. I just asked over on bug 1177334 (which is where we made these tests B2G-specific) -- let's see what happens over there.

Also: if we *are* removing the tests files here, this changeset's commit message should reflect that. Right now it's "Remove B2G and Mulet annotations from reftest.list: layout/reftests/bugs".  That should probably say: "Remove B2G and Mulet annotations from reftest.list (and B2G-specific reftests): layout/reftests/bugs"
"""

From bug 1177334 comment 7:
"We should probably switch them from skip-if(!B2G) to skip-if(!Android). In theory the tests should run and pass fine on Fennec, although what happens in practice may be different."
The reftests fail if one replaces B2G with Android:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=efb2fff29ba0813d5cdf27dcda83d49a5d2cb5ca
Differences are between 1100 and 1500 pixels.

Should the accepted fuzziness be increased?
Flags: needinfo?(dholbert)
Your try push has two patches - the second patch removes the files. So the reftests aren't actually running. If you look at the reftest analyzer output it just has error messages.
Sorry, forgot to remove that and rebase it.
Flags: needinfo?(dholbert)
(review/needinfo here should probably be directed at kats and/or tnikkel, rather than at me, since they know these tests and I do not.)
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d64534f73a6dbfa911982b24433984c282922f1d
Highest difference in pixels: 272

Should the accepted fuzziness be increased?
Flags: needinfo?(bugmail)
This isn't a fuzziness issue (and a fuzziness annotation wouldn't be appropriate) -- it's a legitimate test-failure (at least, to the extent that the testcase is testing what it intends to be testing).

Here's a link directly to reftest-analyzer:
https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/UdYRuNTbSwKw74W-KOY2Jw/runs/0/artifacts/public%2Flogs%2Flive_backing.log&only_show_unexpected=1

In e.g. the first testcase, 1133905-1-v.html, the testcase and reference case both render with a single gray vertical bar.  But the bar is at a completely different position (and a completely different height) in the testcase vs. the reference case.  So, the test isn't fuzzy -- it's just not matching at all.

[leaving needinfo open, since we still need to know how to proceed here]
I guess mark them as failing and file a bug to look into whats going wrong to re-enable them at some point?
Thanks.

So, Aryx, I believe an annotation like this would be appropriate, for any of these tests that fail:
> skip-if(!Android) fails-if(Android) == 1133905-1-v.html 1133905-1-v-ref.html

And we should probably just get rid of the existing "fuzzy-if(B2G,102,720)" annotations (rather than blindly applying s/B2G/Android/ to them).  If a fuzziness annotation is appropriate for those tests on Android, we can add one when we re-triage them.  And there's a decent chance the fuzzy-if(B2G...) annotation was never appropriate in the first place -- I'll bet it might've really been something like what I was seeing in comment 6, and we always should've just considered these tests as failing rather than fuzzy.
Flags: needinfo?(bugmail) → needinfo?(aryx.bugmail)
I think the use of fuzziness was reasonable for these tests (I added it when I added the tests after verifying that the tests were testing what I wanted on try server by looking at the resulting images). I just wanted to make sure that scrollbars were put in the right place, but due to the way I had to test that I couldn't get the math to work out so they lined up perfectly. These tests were the best way that I could come up with. Certainly better then having no test and regressing the behaviour again (which we had a lot of problems with).
I agree with what Timothy said. Please cc me on the new bug you file and I can look into the failures. It looks like the page is just rendered at a different zoom which should be relatively easy to fix.
Comment on attachment 8799102 [details]
Bug 1307719 - Switch B2G-only test annotations in layout/reftests/bugs/reftest.list to Android and disable them until they get fixed.

https://reviewboard.mozilla.org/r/84402/#review83176

r=me with a comment added to explain the odd state we're leaving these tests in.

::: layout/reftests/bugs/reftest.list:1878
(Diff revision 1)
>  == 1127107-2-capitalize.html 1127107-2-capitalize-ref.html
>  == 1127679-1a-inline-flex-relpos.html 1127679-1b-inline-flex-relpos.html
>  == 1128354-1.html 1128354-1-ref.html
>  == 1130231-1-button-padding-rtl.html 1130231-1-button-padding-rtl-ref.html
>  == 1130231-2-button-padding-rtl.html 1130231-2-button-padding-rtl-ref.html
> -skip-if(!B2G) == 1133905-1.html 1133905-ref.html
> +skip-if(!Android) fails-if(Android) == 1133905-1.html 1133905-ref.html

Superficially, it looks odd to see these tests being skipped/expected-fail everywhere (with an oddly-specific query).

Please add an explanatory comment -- something like:

# The 1133905-*.html reftests only make sense on platforms where both APZ and
# <meta viewport> are enabled.
# (Note: bug 1308702 covers these tests' failures on Android)

(The information in that explanation is taken from bug 1177334 comment 0, btw.  And everything but the last line will still be valid & helpful even after the followup is addressed.)
Attachment #8799102 - Flags: review?(dholbert) → review+
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/7a7a446e5256
Switch B2G-only test annotations in layout/reftests/bugs/reftest.list to Android and disable them until they get fixed. r=dholbert
https://treeherder.mozilla.org/logviewer.html#?job_id=4824574&repo=autoland is unrelated (same file, but much earlier in the list).

Daniel, the patch has been updated and now has the 28 tests which passed enabled. Do you want to review it again or should I land it? Thanks.
Flags: needinfo?(dholbert)
Thanks! The interdiff in comment 16 looks good to me -- you can carry forward the existing r=dholbert.
Flags: needinfo?(dholbert)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/e0f7328a3633
Switch B2G-only test annotations in layout/reftests/bugs/reftest.list to Android and disable them until they get fixed. r=dholbert
That sucks. :-/  That's probably because these tests cause rebucketing, which pushes some problematic test from one R-### group to the next, which triggers some fuzziness in this later test. (or something)

rs=me to add a fuzziness anotation to that test (ideally in its own commit, with an explanatory commit message like the one we used in the similar http://hg.mozilla.org/mozilla-central/rev/97a9b83bc394 followup-patch.)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/1fcd8fa662b5
Switch B2G-only test annotations in layout/reftests/bugs/reftest.list to Android and disable them until they get fixed. r=dholbert
(In reply to Daniel Holbert [:dholbert] from comment #21)
> rs=me to add a fuzziness anotation to that test (ideally in its own commit,
> with an explanatory commit message like the one we used in the similar
> http://hg.mozilla.org/mozilla-central/rev/97a9b83bc394 followup-patch.)
This happened in bug 1309533 because the rebucketing already kicked in earlier today when mozilla-central got merged to inbound.
https://hg.mozilla.org/mozilla-central/rev/1fcd8fa662b5
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: