1120630 - Turn on Mn-e10s

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Chris Manchester (limited bugmail, email directly)]

This is to support the m21s project, so our support e10s in marionette doesn't unexpectedly regress due to marionette or gecko changes.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bz://1120630/chmanchester

Comment 2

[Chris Manchester (limited bugmail, email directly)]

/r/2383 - Bug 1120630 - Add --e10s to turn on electrolysis for marionette tests.
/r/2385 - Bug 1120630 - Skip tests failing with e10s enabled.

Pull down these commits:

hg pull review -r dae0d745563c99587db3d777f3d377e99ff5e52a

Comment 3

[Chris Manchester (limited bugmail, email directly)]

/r/2383 - Bug 1120630 - Add --e10s to turn on electrolysis for marionette tests.
/r/2385 - Bug 1120630 - Skip tests failing with e10s enabled.

Pull down these commits:

hg pull review -r dae0d745563c99587db3d777f3d377e99ff5e52a

Comment 4

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bz://1120630/chmanchester

Comment 5

[Chris Manchester (limited bugmail, email directly)]

/r/2389 - Bug 1120630 - Add --e10s to marionette's mozharness script.

Pull down this commit:

hg pull review -r ec04a612b04a06c949f5d43081ce3dc363e304e1

Comment 6

[Chris Manchester (limited bugmail, email directly)]

I've tested the above commits with mozharness locally.

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Attachment: Add marionette-e10s to trunk branches

Hi Armen, this is based off of your work in bug 989533, but I'm
not sure if this is the way to do things these days (and I don't
have much experience with buildbot configs).

I think these would be very valuable to run on even one platform,
opt only, per checkin. This patch starts them off on cedar, but
running them hidden to start might be even better.

Comment 8

[Armen [:armenzg]]

Comment on attachment 8547954 [details] [diff] [review]
Add marionette-e10s to trunk branches

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

It looks about right.

You can run your patch against this script and it will show you exactly the difference of builders with your patch:
https://hg.mozilla.org/build/braindump/file/df19552df45c/community/list_builder_differences.sh (You need to the check the repo out)
It takes barely a minute to run.

::: mozilla-tests/config.py
@@ +638,5 @@
> +MARIONETTE_E10S = [
> +    ('marionette-e10s', {
> +        'use_mozharness': True,
> +        'script_path': 'scripts/marionette.py',
> +        'extra_args': ['--e10s'],

We _might_ be able to keep this parameter in the tree.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Builders added:
+ Ubuntu VM 12.04 cedar opt test marionette-e10s

Comment 10

[Jonathan Griffin (:jgriffin)]

https://reviewboard.mozilla.org/r/2387/#review1573

::: scripts/marionette.py
(Diff revision 1)
> -       + copy.deepcopy(blobupload_config_options)
> +        ["--e10s"],

Could we use --app-arg for this, instead of introducing a new config parameter?

Comment 11

[Jonathan Griffin (:jgriffin)]

https://reviewboard.mozilla.org/r/2381/#review1575

::: testing/marionette/client/marionette/tests/unit/test_navigation.py
(Diff revision 1)
> +    @skip_if_e10s

Please add a comment or bug # about why tests are skipped on e10s.

::: testing/marionette/client/marionette/geckoinstance.py
(Diff revision 1)
> -                  gecko_log=None, prefs=None, ):
> +                  gecko_log=None, prefs=None, required_prefs=None):

Are we adding a new required_prefs kwarg instead of using the existing prefs kwarg because of the munging we do in restart()?  Could we unify them?

::: testing/marionette/client/marionette/runner/base.py
(Diff revision 1)
> +        self.add_option('--e10s',

Like the other patch, is there a reason not to use --app-arg here?

Comment 12

[Chris Manchester (limited bugmail, email directly)]

https://reviewboard.mozilla.org/r/2381/#review1579

> Like the other patch, is there a reason not to use --app-arg here?

--app-arg looks like it's for passing command line arguments to Firefox, but I don't know if there's one that will turn on electrolysis. mochitests and reftests take this approach of a special argument that their harnesses translate into setting this pref.

> Are we adding a new required_prefs kwarg instead of using the existing prefs kwarg because of the munging we do in restart()?  Could we unify them?

Right, if we just use self.prefs those get wiped by a restart. I think it's reasonable enough to copy prefs given to the constructor to persist across restarts instead of adding the new kwarg, I'll push a fixup.

Comment 13

[Chris Manchester (limited bugmail, email directly)]

/r/2383 - Bug 1120630 - Add --e10s to turn on electrolysis for marionette tests.
/r/2385 - Bug 1120630 - Skip tests failing with e10s enabled.

Pull down these commits:

hg pull review -r 1ba5de8c9b7fa85e41976d142ac6a536aef0e593

Comment 14

[Jonathan Griffin (:jgriffin)]

(In reply to Chris Manchester [:chmanchester] from comment #12)
> https://reviewboard.mozilla.org/r/2381/#review1579
> 
> > Like the other patch, is there a reason not to use --app-arg here?
> 
> --app-arg looks like it's for passing command line arguments to Firefox, but
> I don't know if there's one that will turn on electrolysis. mochitests and
> reftests take this approach of a special argument that their harnesses
> translate into setting this pref.
> 
Thanks, for some reason I thought --e10s was a valid Firefox command-line arg.

Comment 15

[Jonathan Griffin (:jgriffin)]

https://reviewboard.mozilla.org/r/2381/#review1581

Ship It!

Comment 16

[Jonathan Griffin (:jgriffin)]

https://reviewboard.mozilla.org/r/2387/#review1583

Ship It!

Comment 17

[Chris Manchester (limited bugmail, email directly)]

mozharness:
https://hg.mozilla.org/build/mozharness/rev/672aeab80d64

Comment 18

[Chris Manchester (limited bugmail, email directly)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/340b2907b368
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e3b9dcef0c71

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/340b2907b368
https://hg.mozilla.org/mozilla-central/rev/e3b9dcef0c71

Comment 20

[Chris Cooper [:coop] (he/him)]

In production: https://hg.mozilla.org/build/mozharness/rev/672aeab80d64

Comment 21

[Chris Manchester (limited bugmail, email directly)]

This has been running, visible, and green on linux32 opt trunk jobs since this morning.

Comment 23

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1120630 - Add --e10s to marionette's mozharness script.

Comment 25

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1120630 - Skip tests failing with e10s enabled.

Comment 26

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1120630 - Add --e10s to turn on electrolysis for marionette tests.

Comment 27

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.