1348754 - stylo reftests appear to be running as non-e10s but are reported as e10s

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task, P2)

Description

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

while reading logs related to bug 1346232, I noticed that the stylo reftest jobs (listed as e10s) are actually non-e10s.

this is marked in taskcluster as e10s:true, but in mozharness we hardcode --disable-e10s:
https://dxr.mozilla.org/mozilla-central/source/testing/mozharness/configs/unittests/linux_unittest.py#229

Comment 1

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

:ted, I see in bug 1318200 the definition for reftest-stylo has a hardcoded --disable-e10s, was this intentional? (gps doesn't accept ni and you reviewed the code)

Comment 2

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

:kmoir, I see that you disabled the non-e10s stylo reftests, based on the discovery of the hardcoded --disable-e10s, I suspect we were running reftests twice both non-e10s and just different names- is there a chance you know more background here why we would be running these as e10s, but having --disable-e10s being called?

Comment 3

[Kim Moir [:kmoir] ET]

You requested requested that non-e10s tests be turned off because we will ship stylo after ff 57 ships.  See bug 1343301 for details.  It looks like we should disable --disable-e10s in the mh config unless there is a reason not too.

Comment 4

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

I would like to know the history of why we have --disable-e10s in the configs and if it was ever not there- otherwise removing it is a 1 line change.

Comment 5

[Chris Peterson [:cpeterson]]

The Stylo team would generally like to run tests in both e10s and non-e10s modes to find different types of bugs, but accepts that reftests are expensive and so it is OK to run reftests only in e10s mode. This spreadsheet documents my current understanding of the Stylo test configurations: 

https://docs.google.com/spreadsheets/d/1HmBsxrEpFfCgpLBG5Mvj5wEA9oEq5N8m4cxQYsSo4S8/edit#gid=1737640065

Comment 6

[Bobby Holley (:bholley)]

Also, note that I'd like to multiplex our e10s/non-e10s test coverage by testing different traversal algorithms for each. See bug 1318187.

Comment 7

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

doing a try run to remove the --disable-e10s doesn't seem to run the tests in e10s mode, maybe we have code elsewhere:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b4a5fcd414a8d069cf8d49376c328ae8dc261f65

Comment 8

[Geoff Brown [:gbrown]]

https://dxr.mozilla.org/mozilla-central/rev/05bfa2831c0ba4a26fa72328ffe6a99aba9c356a/taskcluster/ci/test/tests.yml#1109

Notice that e10s will be different on try (default) from central (I don't know why).

Comment 9

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

I did get e10s tests running on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d808679009eb30842e2e2af1e1fdc8e15e1d17a6&filter-resultStatus=success&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=running&filter-resultStatus=pending&filter-resultStatus=runnable

not a final patch, but it shows we have some perma failing tests- i.e. more work to do.

Comment 10

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

:bholley, can you get someone to fix the perma failing issues above?

Comment 11

[Bobby Holley (:bholley)]

(In reply to Joel Maher ( :jmaher) from comment #10)
> :bholley, can you get someone to fix the perma failing issues above?

I am fine with just marking those tests as failing on e10s if that's what they're actually doing. We still have thousands of failing tests on stylo, so twiddling the expectations on a handful of tests to match reality is totally fine.

Comment 12

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

Attachment: run stylo on e10s

this should do the trick!

Comment 13

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

trying to figure out the best way to disable tests in the reftest-stylo.list files, is there a convention used for this?  Most of these are off by a long way, as in we should skip them.  I can just add a # to the beginning of the line, for reference, here is the e10s tests:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5039c51d91a17a58d547fff889113b08a8aabb4&selectedJob=85583609

Comment 14

[Bobby Holley (:bholley)]

(In reply to Joel Maher ( :jmaher) from comment #13)
> trying to figure out the best way to disable tests in the reftest-stylo.list
> files, is there a convention used for this?  Most of these are off by a long
> way, as in we should skip them.

Well, we should mark them as failing so that we get UNEXPECTED-PASSes when they stop failing. We should only comment-out tests that crash.

>  I can just add a # to the beginning of the
> line, for reference, here is the e10s tests:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=c5039c51d91a17a58d547fff889113b08a8aabb4&selectedJob=8
> 5583609

Comment 15

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

Attachment: disable tests which fail regularly (20%+) in e10s mode

when run with the other patch in this bug, we should see green results and real e10s data!

Comment 16

[Kim Moir [:kmoir] ET]

Comment on attachment 8850084 [details] [diff] [review]
run stylo on e10s

I don't think you need the part that patches
taskcluster/ci/test/tests.yml 

The issue was that the tests were running, but they were just running in non-e10s mode which the mh fix will address.  If you change default: true, the tests will run on other branches, not just the branches specified

Comment 17

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

I needed that default:true to get this to work on try, otherwise these would run in non-e10s all the time.  Possibly I don't need that for integration/m-c though.  My understanding of e10s is:
both = non-e10s && e10s  (default)
false = non-e10s only
true = e10s only

I am happy to try this without the tests.yml part, but would like some confirmation prior do doing that.

Comment 18

[Kim Moir [:kmoir] ET]

Comment on attachment 8850084 [details] [diff] [review]
run stylo on e10s

Actually nevermind, the patch is fine

I forgot that the tests are limited by branch above

 run-on-projects:
    by-test-platform:
         linux64-stylo/opt: [ 'stylo', 'autoland', 'mozilla-central', 'try' ]
         linux64-stylo/debug: [ 'stylo', 'autoland', 'mozilla-inbound', 'mozilla-central', 'try' ]
         default: []

Comment 19

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8850574 [details] [diff] [review]
disable tests which fail regularly (20%+) in e10s mode

Review of attachment 8850574 [details] [diff] [review]:
-----------------------------------------------------------------

bz suggested that we add if(stylo) all the time when something is stylo-specific, so that we can distinguish between a failure inherited from reftest.list and one from just stylo.

::: layout/reftests/async-scrolling/reftest-stylo.list
@@ +30,5 @@
>  fails == fixed-pos-scrollable-1.html fixed-pos-scrollable-1.html
>  == culling-1.html culling-1.html
>  == position-fixed-iframe-1.html position-fixed-iframe-1.html
>  == position-fixed-iframe-2.html position-fixed-iframe-2.html
> +fuzzy-if(skiaContent,1,11300) skip-if(!asyncPan) fails-if(browserIsRemote) == position-fixed-in-scroll-container.html position-fixed-in-scroll-container.html

fails-if(stylo&&browserIsRemove)

@@ +39,5 @@
>  == offscreen-prerendered-active-opacity.html offscreen-prerendered-active-opacity.html
>  fails == offscreen-clipped-blendmode-1.html offscreen-clipped-blendmode-1.html
>  fails == offscreen-clipped-blendmode-2.html offscreen-clipped-blendmode-2.html
>  fuzzy-if(Android,6,4) skip == offscreen-clipped-blendmode-3.html offscreen-clipped-blendmode-3.html
> +fuzzy-if(Android,6,4) skip-if(!asyncPan) fails-if(browserIsRemote) == offscreen-clipped-blendmode-4.html offscreen-clipped-blendmode-4.html

ditto.

@@ +44,5 @@
>  fails == perspective-scrolling-1.html perspective-scrolling-1.html
>  fails == perspective-scrolling-2.html perspective-scrolling-2.html
> +fuzzy-if(Android,7,4) skip-if(!asyncPan) fails-if(browserIsRemote) == perspective-scrolling-3.html perspective-scrolling-3.html
> +fuzzy-if(Android,7,4) skip-if(!asyncPan) fails-if(browserIsRemote) == perspective-scrolling-4.html perspective-scrolling-4.html
> +pref(apz.disable_for_scroll_linked_effects,true) skip-if(!asyncPan) fails-if(browserIsRemote) == disable-apz-for-sle-pages.html disable-apz-for-sle-pages.html

ditto.

::: layout/reftests/invalidation/reftest-stylo.list
@@ +3,5 @@
>  == table-repaint-c.html table-repaint-c.html
>  == table-repaint-d.html table-repaint-d.html
>  == 540247-1.xul 540247-1.xul
>  fails == 543681-1.html 543681-1.html
> +fails-if(!browserIsRemote) == 1243409-1.html 1243409-1.html

fails-if(stylo&&!browserIsRemove)

::: layout/reftests/layers/reftest-stylo.list
@@ +16,5 @@
>  fails == pull-background-displayport-2.html pull-background-displayport-2.html
> +skip-if(!asyncPan) fails-if(browserIsRemote) == pull-background-displayport-3.html pull-background-displayport-3.html
> +skip-if(!asyncPan) fails-if(browserIsRemote) == pull-background-displayport-4.html pull-background-displayport-4.html
> +skip-if(!asyncPan) fails-if(browserIsRemote) == pull-background-displayport-5.html pull-background-displayport-5.html
> +skip-if(!asyncPan) fails-if(browserIsRemote) == pull-background-displayport-6.html pull-background-displayport-6.html

fails-if(stylo&&browserIsRemote)

::: layout/reftests/position-sticky/reftest-stylo.list
@@ +40,5 @@
>  == overconstrained-3.html overconstrained-3.html
>  fails == inline-1.html inline-1.html
>  fails == inline-2.html inline-2.html
>  fails == inline-3.html inline-3.html
> +skip-if(!asyncPan) fails-if(browserIsRemote) == inline-4.html inline-4.html

ditto.

::: layout/reftests/w3c-css/submitted/ui3/reftest-stylo.list
@@ +6,5 @@
>  == box-sizing-content-box-002.xht box-sizing-content-box-002.xht
>  == box-sizing-content-box-003.xht box-sizing-content-box-003.xht
>  == box-sizing-replaced-001.xht box-sizing-replaced-001.xht
>  == box-sizing-replaced-002.xht box-sizing-replaced-002.xht
> +random-if(browserIsRemote) == box-sizing-replaced-003.xht box-sizing-replaced-003.xht

random-if(stylo&&browserIsRemote)

Comment 20

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

thanks for the feedback :xidorn, I have edited the patch and validating on try prior to landing it.

Ideally we can fix all the changes to have stylo and then we could get rid of the reftest-stylo.list files? :)

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8fe34c71824
stylo reftests appear to be running as non-e10s but are reported as e10s. r=kmoir
https://hg.mozilla.org/integration/mozilla-inbound/rev/05ab5ec30704
annotate reftests to allow stylo to run in e10s. r=xidorn

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e8fe34c71824
https://hg.mozilla.org/mozilla-central/rev/05ab5ec30704