1328411 - BaseMarionetteTestRunner should log e10s status based on appinfo not incoming args

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Eric Rahm [:erahm]]

After updating AWSY to use the |e10s| flag when using MarionetteTestRunner [1] we found that setting it to False does not actually disable e10s.

Snippet:

> runner = MarionetteTestRunner(
>  binary=self.tester.binary,
>  profile=profile,
>  logger=self.parent.logger,
>  address="localhost:%d" % self.port,
>  gecko_log=self.gecko_log,
>  startup_timeout=60,
>  e10s=e10s)
            
Here |e10s| is False.

STR:
- Start a test with MarionetteTestRunner(e10s=False)
- Note the marionette log indicates e10s is disabled
- Navigate to a page
- Note content process is created

[1] https://github.com/mozilla/areweslimyet/commit/7d83a7d8b6cc1fb36dc89b681cadb96521dd21a9

Comment 1

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

Wow, we store --disable-e10s as self.e10s and tread it as enable flag later? Or do I miss something without looking deeper to this code?

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/runner/base.py#351

So I assume it's getting handled vice-versa at the moment?

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

The logic whimboo refers to in Comment 1 is fine because of "store_false" parameter, so --disable-e10s sets self.e10s to false and things happen accordingly.

e10s actually gets enabled via the "prefs" argument to MarionetteTestRunner. So, --disable-e10s causes e10s-related preferences to be omitted, but only if you call verify_usage [1]. 

I have to run now, but see how the marionette-test mach command calls verify_usage before passing args to MarionetteTestRunner. [2] You need to do something equivalent to that.

We should change the logging back to using something like |self.appinfo.get('browserTabsRemoteAutostart', False)| to report what's actually happening the browser.


[1] http://searchfox.org/mozilla-central/rev/22be34bcc4d5c56b62482a537bba77a6cdce117b/testing/marionette/harness/marionette_harness/runner/base.py#446
[2] http://searchfox.org/mozilla-central/rev/22be34bcc4d5c56b62482a537bba77a6cdce117b/testing/marionette/mach_commands.py#54-59

Comment 3

[Eric Rahm [:erahm]]

I see, so the issue is I'm not calling |verify_usage| (because I don't have a command line). Can we make MarionetteTestRunner add the profile entries instead of |verify_usage|, or will that break other things?

Comment 4

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Yes. Let's make verify_usage only verify usage. I'll try a patch with the e10s-prefs block moved to the BaseMarionetteTestRunner __init__. That probably* won't break anything.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff521269be294acdd1a1ab995c446b452c407aed

Comment 5

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1328411 - Log e10s status in Marionette runner based on appinfo;

Previously, this log message was based on self.e10s, which only reflects the
option passed into Marionette runner, not what is actually happening in the browser.

Review commit: https://reviewboard.mozilla.org/r/102262/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102262/

Comment 6

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1328411 - Move e10s prefs setup to BaseMarionetteTestRunner;

This allows the |e10s| argument in BaseMarionetteTestRunner to enable e10s in
the browser without depending on |BaseMarionetteArguments.verify_usage|.

Unlike other args like |jsdebugger|, the |e10s| argument is found in both
BaseMarionetteArguments and BaseMarionetteTestArguments, so it's more
intuitive to pass it along and perform all the associated setup
in BaseMarionetteTestRunner.

Review commit: https://reviewboard.mozilla.org/r/102264/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102264/

Comment 7

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Attachment: Bug 1328411 - Enforce relationship between e10s runner option and browser appinfo;

Review commit: https://reviewboard.mozilla.org/r/102266/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102266/

Comment 8

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e78bc66e499d3553829eac5b6e6b6e80bc6559ee

Comment 9

[David Burns :automatedtester]

Comment on attachment 8823755 [details]
Bug 1328411 - Log e10s status in Marionette runner based on appinfo;

https://reviewboard.mozilla.org/r/102262/#review102874

Comment 10

[David Burns :automatedtester]

Comment on attachment 8823756 [details]
Bug 1328411 - Move e10s prefs setup to BaseMarionetteTestRunner;

https://reviewboard.mozilla.org/r/102264/#review102876

Comment 11

[David Burns :automatedtester]

Comment on attachment 8823757 [details]
Bug 1328411 - Enforce relationship between e10s runner option and browser appinfo;

https://reviewboard.mozilla.org/r/102266/#review102878

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8929b961457
Log e10s status in Marionette runner based on appinfo; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/8a7c1bfc7186
Move e10s prefs setup to BaseMarionetteTestRunner; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/90c553c768ef
Enforce relationship between e10s runner option and browser appinfo; r=automatedtester

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/b8929b961457
https://hg.mozilla.org/mozilla-central/rev/8a7c1bfc7186
https://hg.mozilla.org/mozilla-central/rev/90c553c768ef

Comment 14

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

Maja, mind checking for an uplift to 52 so it can make the next ESR?

Beside that it's good to know that when you run Fennec tests now, you always have to pass in --disable-e10s. Otherwise the harness will bail out due to Fennec runs in non-e10s only (for Marionette?).

Comment 15

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Sheriffs: test-only uplift to aurora to accurately log e10s status, please and thank you.

Comment 16

[Carsten Book [:Tomcat]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/fd5c16ef7c97fa39d1ef1840abd2c9304df580f2
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/b074ffce576d52dcb830ea8316fd38a90af68424
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/0e5631748857e7df0b8e9c849fb7348f3318dfe4