Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: chmanchester, Assigned: chmanchester)

Tracking

(Blocks 1 bug, {pi-marionette-runner})

unspecified
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+)

Details

Attachments

(4 attachments, 2 obsolete attachments)

This is to support the m21s project, so our support e10s in marionette doesn't unexpectedly regress due to marionette or gecko changes.
/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
Attachment #8547934 - Flags: review?(jgriffin)
/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
Attachment #8547950 - Flags: review?(jgriffin)
/r/2389 - Bug 1120630 - Add --e10s to marionette's mozharness script.

Pull down this commit:

hg pull review -r ec04a612b04a06c949f5d43081ce3dc363e304e1
I've tested the above commits with mozharness locally.
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.
Attachment #8547954 - Flags: feedback?(armenzg)
Assignee: nobody → cmanchester
Status: NEW → ASSIGNED
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.
Attachment #8547954 - Flags: feedback?(armenzg) → feedback+
Builders added:
+ Ubuntu VM 12.04 cedar opt test marionette-e10s
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?
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?
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.
/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
(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.
Attachment #8547934 - Flags: review?(jgriffin) → review+
Attachment #8547950 - Flags: review?(jgriffin) → review+
Depends on: 1121125
Depends on: 1121589
Depends on: 1124741
This has been running, visible, and green on linux32 opt trunk jobs since this morning.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Attachment #8547950 - Attachment is obsolete: true
Attachment #8547934 - Attachment is obsolete: true
Attachment #8619108 - Flags: review+
Attachment #8619109 - Flags: review+
Attachment #8619110 - Flags: review+
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.