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

: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)

: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?

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.

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.

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

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

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

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).

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.

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

(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.

this should do the trick!

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

(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

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

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

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 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 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)

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? :)

Pushed by jmaher@mozilla.com: 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

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