Closed Bug 1328411 Opened 7 years ago Closed 7 years ago

BaseMarionetteTestRunner should log e10s status based on appinfo not incoming args

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: erahm, Assigned: impossibus)

Details

(Keywords: pi-marionette-runner)

Attachments

(3 files)

Bug 1328411 - Log e10s status in Marionette runner based on appinfo;
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1328411 - Move e10s prefs setup to BaseMarionetteTestRunner;
59 bytes, text/x-review-board-request
automatedtester
: review+
Details
Bug 1328411 - Enforce relationship between e10s runner option and browser appinfo;
59 bytes, text/x-review-board-request
automatedtester
: review+
Details 
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
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?
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
Assignee: nobody → mjzffr
Status: NEW → ASSIGNED
Summary: Setting e10s=false does not disable e10s in MarionetteTestRunner → BaseMarionetteTestRunner should log e10s status based on appinfo not incoming args
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?
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 on attachment 8823755 [details]
Bug 1328411 - Log e10s status in Marionette runner based on appinfo;

https://reviewboard.mozilla.org/r/102262/#review102874
Attachment #8823755 - Flags: review?(dburns) → review+
Comment on attachment 8823756 [details]
Bug 1328411 - Move e10s prefs setup to BaseMarionetteTestRunner;

https://reviewboard.mozilla.org/r/102264/#review102876
Attachment #8823756 - Flags: review?(dburns) → review+
Comment on attachment 8823757 [details]
Bug 1328411 - Enforce relationship between e10s runner option and browser appinfo;

https://reviewboard.mozilla.org/r/102266/#review102878
Attachment #8823757 - Flags: review?(dburns) → review+
Pushed by mjzffr@gmail.com:
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
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?).
Sheriffs: test-only uplift to aurora to accurately log e10s status, please and thank you.
Whiteboard: [checkin-needed-aurora]
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: