1386689 - disable non-e10s tests which are duplicated with e10s jobs

Status: RESOLVED FIXED

(Testing :: General, enhancement)

Description

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

now that we are on the firefox 57 train we will ship e10s by default, we can reduce the load on our infrastructure and maintenance of tests.

Comment 1

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

Attachment: run tests on e10s only if we were running it in both e10s and non-e10s

Comment 2

[Geoff Brown [:gbrown]]

Comment on attachment 8892947 [details] [diff] [review]
run tests on e10s only if we were running it in both e10s and non-e10s

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

A couple of concerns here + I'd like to see it on try.

::: taskcluster/ci/test/tests.yml
@@ +130,5 @@
>          by-test-platform:
>              android-4.3-arm7-api-15/debug: 10
>              android.*: 4
>              default: 1
> +    e10s: true

How about Android? We should run non-e10s Android crashtests. I don't understand how this is working currently.

@@ +459,1 @@
>              default: both

Shouldn't this default change to 'true'?

Comment 3

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

Attachment: run tests on e10s only if we were running it in both e10s and non-e10s

updated patch to remove all android instances as we have a transform which ensures android only runs in non-e10s mode.

some try pushes:
https://treeherder.mozilla.org/#/jobs?repo=try&author=[e-mail]&fromchange=1ee3d5cf919a598103c2466930524e21998ea60e&tochange=c8fbbaa723e9df1b726ff75b83938edaf7ba540a

Comment 4

[Geoff Brown [:gbrown]]

Comment on attachment 8893038 [details] [diff] [review]
run tests on e10s only if we were running it in both e10s and non-e10s

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

In your try push, I notice Ru is running both e10s and non-e10s; is that intentional?

Comment 5

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

Attachment: run tests on e10s only if we were running it in both e10s and non-e10s

this test will default to e10s only and removes as many e10s: lines from tests.yml :)  If e10s wasn't in the tests.yml for a given job, then we would default to 'both' and that is not good.

here is a try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=83349cd99098623a92de2e80d309010f5496e5f4

Comment 6

[Geoff Brown [:gbrown]]

Comment on attachment 8893074 [details] [diff] [review]
run tests on e10s only if we were running it in both e10s and non-e10s

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

This keeps getting better! try looks good to me.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d1d26e7fa93
disable non-e10s tests which are duplicated with e10s jobs. r=gbrown

Comment 8

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

https://hg.mozilla.org/mozilla-central/rev/0d1d26e7fa93

Comment 9

[:Gijs (he/him)]

Can someone spell out what this changes for mochitest and mochitest-browser-chrome? I tried reading the cset but it didn't really help me. :-(

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to :Gijs from comment #9)
> Can someone spell out what this changes for mochitest and
> mochitest-browser-chrome? I tried reading the cset but it didn't really help
> me. :-(

Looks like mochitest and mochitest-browser-chrome now only run in e10s on mode.

Comment 11

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

basically all mochitest runs in e10s only.  mochitest-a11y was defined as non-e10s only in the config, so that remained as non-e10s.

Comment 12

[:Gijs (he/him)]

(In reply to Joel Maher ( :jmaher) (UTC-9) from comment #11)
> basically all mochitest runs in e10s only.  mochitest-a11y was defined as
> non-e10s only in the config, so that remained as non-e10s.

So for mochitest-plain and mochitest-browser and mochitest-devtools, skip-if = !e10s is now meaningless, and skip-if = e10s tests should be removed (ditto for the inverse run-if expressions) ?

That seems (a) wrong and (b) something that should be publicized more widely/explicitly, if that's what we're doing.

There are still cases where users don't get e10s (IIRC notably a11y support, and for <=56, add-ons) and so it seems a bit premature to stop testing it altogether.

Comment 13

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

this is only on firefox 57 which ships e10s by default.  It will ride the trains until we ship 57.

you are correct that for the most part e10s in the .ini files are irrelevant now.  We currently run in non-e10s:
* mochitest-a11y
* mochitest-jetpack (soon to be deprecated)
* xpcshell
* gtest
* cpp

everything else runs in e10s mode.

Comment 14

[Julian Descottes [:jdescottes]]

What will happen to tests and test suites that use "skip-if = e10s" in the .ini file? 

We have a bunch of those in devtools:
  http://searchfox.org/mozilla-central/search?q=skip-if+%3D+e10s+%23&case=false&regexp=false&path=devtools

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We have the same for Marionette tests, and I assume lots of other test suites have a similar situation. When do we want to remove those tests? Or should those stay for now in case we have to re-enable non-10s tests for whatever reason?

Comment 16

[:Gijs (he/him)]

(In reply to Joel Maher ( :jmaher) (UTC-9) from comment #13)
> this is only on firefox 57 which ships e10s by default.  It will ride the
> trains until we ship 57.

Let me rephrase my earlier comment, then: are we dropping support for end-users running Firefox in non-e10s configurations with 57?

If so, that should be made explicit somewhere, and I haven't seen that happening. Maybe that's because I missed the relevant .planning or .platform announcement or whatever, but my understanding was that there were still some cases where e10s is (and/or can be) disabled.

If not, we should not be turning off all our mochitest(-browser) testing on non-e10s.

Comment 17

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

1) if there are non e10s specific test cases- I would argue we do not need them
2) for tests which are skipped on e10s but were running on non-e10s, we are losing coverage- oddly enough we are losing coverage every time we disable a test (a few times/week).  Our total code coverage dropped by .6% https://codecov.io/gh/marco-c/gecko-dev
3) should we run at least browser-chrome?  I would argue no, but offer a suggestion.

Keep in mind we have an artificial test environment designed for easy automation- we never test firefox in all configurations, or with addons that users end up using.  With that said, I believe for the next 6-12 months we will have many users running in non-e10s mode to support addons or a11y or for personal choice reasons.  Would it make sense to run some tests in non-e10s mode on windows 7 opt mode?  

* is this the right configuration? (I am trying to pick one configuration that we run in the cloud - and windows is where most of our users are)
* which tests would we need to run?  we disabled reftest, mochitest, web-platform-test, browser-chrome, devtools, marionette, etc.


Keep in mind we disabled non-e10s performance tests a few months ago- and at some point in time we need to assume that we only fully support e10s mode.  Possibly doing this single configuration gives us some buffer room to let us get tests working in e10s mode and cleanup related configs, etc.

:Gijs, do you have thoughts on this?  I would like to have some realistic options before posting to dev.platform.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Joel Maher ( :jmaher) (UTC-9) from comment #17)
> 2) for tests which are skipped on e10s but were running on non-e10s, we are
> losing coverage- oddly enough we are losing coverage every time we disable a
> test (a few times/week).  Our total code coverage dropped by .6%
> https://codecov.io/gh/marco-c/gecko-dev

At least for DevTools, I would say the loss of test coverage because some tests aren't ready for e10s is the main issue.  Of course, in an ideal world, all tests would already be e10s-ready, but that's not where things stand yet.

To save on test load, perhaps we could have an e10s-off run that _only_ runs the "skip-if = e10s" tests?  That might be one way to bring back the test coverage short term, until tests can be fully migrated, without adding too much extra test load.

Anyway, that's just my perspective on DevTools tests.  Other teams may still want a complete run of tests in e10s-off mode for various reasons.

Comment 19

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

(In reply to Joel Maher ( :jmaher) (UTC-9) from comment #17)
> at some point in time we need to assume that we only fully support e10s mode.

That time is now. Rolling out e10s-multi to 100% of users is a key part of the 57 release plans. We're explicitly going to be blocking non-webext addons in the next week or so on Nightly and I'm already seeing bugs blocking bug 1347507 starting to land now that 57 is on Nightly. Users of older addons will need to use ESR52 if they want a supported browser that still will run them or find alternatives to those addons. Turning off e10s to keep them working isn't going to work.

Regarding a11y (the only real reason people could have for getting e10s disabled at this point once the addon block hits), my understanding is that we're very close to being ready to remove that block due to some heroic engineering work. See bug 1387507 as more evidence of that.

In short, non-e10s isn't something we're committing to support in 57 and I fully expect it to break in relative short order as a result.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #18)
> At least for DevTools, I would say the loss of test coverage because some
> tests aren't ready for e10s is the main issue.  Of course, in an ideal
> world, all tests would already be e10s-ready, but that's not where things
> stand yet.

My recollection from being involved in getting disabled tests across various suites fixed and re-enabled for the initial e10s rollout is that many DevTools tests have been disabled for literally years now (and a quick glance at some of the test manifests and associated hg blame backs up that claim). I know the DevTools team has had other priorities over that time period, but unfortunately we're at a point where we've waited as long as we possibly can. If the lost test coverage is seen as truly a major issue for DevTools, then I'm afraid the time has come to bite the bullet and get those disabled tests fixed and re-enabled. We don't have the resources to continue supporting them as the demands on our CI infrastructure continue to grow and Firefox continues to move in an e10s-only direction.

> To save on test load, perhaps we could have an e10s-off run that _only_ runs
> the "skip-if = e10s" tests?  That might be one way to bring back the test
> coverage short term, until tests can be fully migrated, without adding too
> much extra test load.

As I mentioned above, I fully expect non-e10s mode to break rather quickly now that we're getting much closer to the point of having high confidence of all users having it enabled. I have a hard time seeing this as justified engineering effort given all the other priorities that team has in support of the 57 release.

And yes, we still need to figure out what to do with the remaining mochitest-a11y/chrome tests. I know the a11y folks have been working on getting their tests ported to other suites, but I have no idea who if anybody is doing the same for mochitest-chrome.

FWIW, my only real concern about doing this now is that we've just started the 56 Beta cycle and I worry about uplifts that causes non-e10s bustage on obvious or possibly non-obvious ways for the 56 release. But even then, I think that's a risk we need to take at this point.

Comment 20

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> In short, non-e10s isn't something we're committing to support in 57 and I
> fully expect it to break in relative short order as a result.

Then I think we should be announcing this through the appropriate channels, and I haven't seen that happen.

I'm also surprised this is preceding the removal of the opt-in about:config and about:preferences (!) prefs and the non-e10s-window menuitems/buttons and so on. Breaking things for which we have visible UI isn't OK.

Note that the browser still runs some tabs with HTML and/or XUL stuff in the parent process, even when running e10s, so continuing to test non-e10s is not without value. Like you, I also expect untested things to break, but redoing all our tests to have extra cases that check the edgecases of parent-process about: pages even when running in e10s is going to be a massive pain, so losing that coverage without any compensation is unfortunate at best.

(In reply to Joel Maher ( :jmaher) (UTC-9) from comment #17)
> 1) if there are non e10s specific test cases- I would argue we do not need
> them
> 2) for tests which are skipped on e10s but were running on non-e10s, we are
> losing coverage- oddly enough we are losing coverage every time we disable a
> test (a few times/week).  Our total code coverage dropped by .6%
> https://codecov.io/gh/marco-c/gecko-dev
> 3) should we run at least browser-chrome?  I would argue no, but offer a
> suggestion.
> 
> Keep in mind we have an artificial test environment designed for easy
> automation- we never test firefox in all configurations, or with addons that
> users end up using.  With that said, I believe for the next 6-12 months we
> will have many users running in non-e10s mode to support addons or a11y or
> for personal choice reasons.  Would it make sense to run some tests in
> non-e10s mode on windows 7 opt mode?  
> 
> * is this the right configuration? (I am trying to pick one configuration
> that we run in the cloud - and windows is where most of our users are)

If we're only doing one thing (which I agree might be a sensible trade-off), either Windows or Linux64 makes sense, Windows because of the reasons you state and linux64 because it's faster (though I don't know why) and gets used for trypushes more (again, not 100% sure why, though partly because it's faster) and we know that "I pushed to try, why does it break on central anyway" is a legit developer annoyance.

> * which tests would we need to run?  we disabled reftest, mochitest,
> web-platform-test, browser-chrome, devtools, marionette, etc.

I think at least mochitest-browser. I can't speak to devtools, marionette, mochitest-plain or wpt or reftest. m.d.platform will have better ideas than me in that respect.

Comment 21

[Geoff Brown [:gbrown]]

(In reply to :Gijs from comment #20)

Good discussion, thanks for bringing up these concerns :Gijs.

> Note that the browser still runs some tabs with HTML and/or XUL stuff in the
> parent process, even when running e10s, so continuing to test non-e10s is
> not without value. Like you, I also expect untested things to break, but
> redoing all our tests to have extra cases that check the edgecases of
> parent-process about: pages even when running in e10s is going to be a
> massive pain, so losing that coverage without any compensation is
> unfortunate at best.

I didn't quite understand this part: If the browser runs some tabs with HTML and/or XUL stuff in the parent process when running e10s, then won't that be covered by running tests in e10s mode?

Comment 22

[:Gijs (he/him)]

(In reply to Geoff Brown [:gbrown] from comment #21)
> (In reply to :Gijs from comment #20)
> 
> Good discussion, thanks for bringing up these concerns :Gijs.
> 
> > Note that the browser still runs some tabs with HTML and/or XUL stuff in the
> > parent process, even when running e10s, so continuing to test non-e10s is
> > not without value. Like you, I also expect untested things to break, but
> > redoing all our tests to have extra cases that check the edgecases of
> > parent-process about: pages even when running in e10s is going to be a
> > massive pain, so losing that coverage without any compensation is
> > unfortunate at best.
> 
> I didn't quite understand this part: If the browser runs some tabs with HTML
> and/or XUL stuff in the parent process when running e10s, then won't that be
> covered by running tests in e10s mode?

My point was, *some* (but not most) pages are still run in the parent process in e10s. This includes about:preferences.

There are a lot of tests running in e10s mode and they test a lot of different things. The vast majority of them will run against pages that the browser renders in the content process (because mochitest-plain runs against http: localhost pages which will render in the content process in e10s, ditto for mochitest-browser helper files which are usually accessed over http if loaded in tabs, and thus loaded in the content process also).

There are comparatively very few tests specifically for about:preferences (or the other pages that run in the parent process). They don't test lots of basic things, such as (for instance), clipboard behaviour, text selection, drag and drop, the document->rendering pipeline working correctly, etc.

As a result, we're losing test coverage of pages running in the parent process because we are no longer running the vast majority of tests that used to cover non-e10s (ie parent-process-loaded) tabs - only the ones specific to those tabs, which don't bother re-testing all the stuff we cover in other (more targeted/specific) tests.

Comment 23

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

Thanks :Gijs for this- I have created a discussion on dev.platform:
https://groups.google.com/forum/#!topic/mozilla.dev.platform/8gS5pOaLw0s

When we have more data we can determine what to turn on and make this happen in the short term.

Comment 24

[:Felipe Gomes (needinfo for replies!)]

It's not clear to me looking at the changeset what happens to mochitest-chrome. Is it still going to run as non-e10s?

Comment 25

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

It's still running, yes. As to what to do with it long-term? I don't think anybody has an answer to that or actively owns that decision :\

Comment 26

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/date/rev/0d1d26e7fa93f99ced13860c7b9f44b02417f971
Bug 1386689 - disable non-e10s tests which are duplicated with e10s jobs. r=gbrown

Comment 27

[(no longer active)]

(In reply to :Felipe Gomes (needinfo me!) from comment #24)
> It's not clear to me looking at the changeset what happens to
> mochitest-chrome. Is it still going to run as non-e10s?

mochitest-chrome has never passed in e10s.  Things like bug 1255095 make me think that the plan was to *always* run this suite in non-e10s mode.