Closed Bug 997244 Opened 10 years ago Closed 10 years ago

Refactor emulator.py out of marionette and into mozrunner

Categories

(Testing :: Mozbase, defect)

Product:
Testing
Component:
Mozbase
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox31 wontfix, firefox32 wontfix, firefox33 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla33
Tracking Flags:
Tracking Status
firefox31 --- wontfix
firefox32 --- wontfix
firefox33 --- fixed

People

(Reporter: ahal, Assigned: ahal)

References

(Blocks 1 open bug)

Details

(Keywords: ateam-b2g-task)

Attachments

(1 file, 9 obsolete files)

refactor_mozrunner
248.67 KB, patch
ahal
: review+
Details | Diff | Splinter Review 
I'm currently working on a way to run gaia-integration tests using our python toolchain. If possible I'd like to avoid using the python marionette runner in addition to the js marionette runner. In other words, I want to use the functionality in emulator.py without depending on the python marionette client at all.

I think mozrunner is a good place to put it, since that is what is ultimately responsible for starting/stopping things, which is also what emulator.py does.

At the same time I want to refactor mozrunner to get away from the multiple inheritance pattern we are starting to use more and more (mostly my bad!) by using composition instead.

Does anyone have any objections to any of this?
Assignee: nobody → ahalberstadt
Status: NEW → ASSIGNED
(In reply to Andrew Halberstadt [:ahal] from comment #0)
> I'm currently working on a way to run gaia-integration tests using our
> python toolchain. If possible I'd like to avoid using the python marionette
> runner in addition to the js marionette runner. In other words, I want to
> use the functionality in emulator.py without depending on the python
> marionette client at all.
> 
> I think mozrunner is a good place to put it, since that is what is
> ultimately responsible for starting/stopping things, which is also what
> emulator.py does.
> 
> At the same time I want to refactor mozrunner to get away from the multiple
> inheritance pattern we are starting to use more and more (mostly my bad!) by
> using composition instead.
> 
> Does anyone have any objections to any of this?

This sounds great to me. I'd love to give feedback on any patch.
The refactor is quite large and complicated. I'm starting to get to the "test things out and fix all the stuff that broke" phase. Unfortunately I go on vacation after this week, and I don't think I'll be finished by then. I'll at least try to get a patch up that people can give feedback on by end of day Friday.
Attached file First draft of refactor (obsolete) — 
This is the first attempt at refactoring emulator.py out of marionette and into mozrunner. This contains a major mozrunner refactor as well, the details of which are in the reviewboard description.

I'll be away until May 6th, but while I'm gone I'd welcome any and all feedback around how the new mozrunner is structured!
Attachment #8412055 - Flags: feedback?
Also, the vast majority of the patch is adding or deleting files, so it may be easier to just apply it locally than it is to look at the diff.
Attached patch Mozrunner refactor (unbitrot) (obsolete) — — Splinter Review 
Same as last patch with some bitrot fixes. Apparently people are having problems logging into reviewboard, so just posting to bugzilla for now.
Comment on attachment 8419590 [details] [diff] [review]
Mozrunner refactor (unbitrot)

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

It would be helpful to get some more details about all the new classes and what they are used for. So like for targets, context, and others. It would help at least me to better understand the code without having to spend an hour to analyze all that on my own. I assume documentation changes would be necessary in any way, right?
Yeah, I added information about the refactor to the description of the reviewboard request:
https://reviewboard.allizom.org/r/74/

Once we've done some feedback cycles and we're happy with the new interface, I'll definitely update the documentation.
(In reply to Andrew Halberstadt [:ahal] from comment #8)
> Yeah, I added information about the refactor to the description of the
> reviewboard request:
> https://reviewboard.allizom.org/r/74/
> 
> Once we've done some feedback cycles and we're happy with the new interface,
> I'll definitely update the documentation.

s/update/add/ ;)

We have no documentation for mozrunner at present, at least not on http://mozbase.readthedocs.org
Attachment #8412055 - Flags: feedback?
Attached patch Refactor attempt #2 (obsolete) — — Splinter Review 
This isn't very thoroughly tested yet, so don't look at it too closely. I just want to know if this structure/organization is an improvement over the last time or if you'd like me to go back to the drawing board (if you want me to go back to using multiple inheritance I'll do it.. I don't want to block on this much longer).
Attachment #8412055 - Attachment is obsolete: true
Attachment #8419590 - Attachment is obsolete: true
Attachment #8428890 - Flags: feedback?(wlachance)
Comment on attachment 8428890 [details] [diff] [review]
Refactor attempt #2

So I only glanced over the high-level concepts.

I would personally use multiple inheritance over the context stuff, but this is a big improvement over the last time. I can follow what's going on without too much difficulty, which definitely wasn't the case before. I think we can go ahead with this implementation.
Attachment #8428890 - Flags: feedback?(wlachance) → feedback+
Depends on: 1019015
Attached patch Refactor attempt #2 - updated (obsolete) — — Splinter Review 
Here's an updated patch that passes more things. Currently some B2G emulator mochitests are broken (though the suite runs ok) and a couple Mnw failures I'm trying to figure out.
Attachment #8428890 - Attachment is obsolete: true
Blocks: 1014653
Getting close:
https://tbpl.mozilla.org/?tree=Try&rev=c222d6d45f27

The valgrind error I fixed locally. The mochitest emulator debug is running and passing, but crashing on shutdown, but this is because I added a check_for_crashes on finish which we weren't doing previously due to bug 969146. The only real unsolved problem is the 'addError' stuff in emulator mochitest-5, I'll keep working at it.
Looks like the debug xpcshell is perma-orange too.. though all tests are marked as pass, that's confusing
Ok I think I've sorted through all the issues now. The mochitest one was happening because I was misusing tempfile causing the sdcard to be read-only. Just waiting on a final try run now, in the meantime I'll write some mozrunner docs and format the patch for review.
Spoke too soon. There's a single mochitest failure and a single xpcshell failure on debug emulators only: https://tbpl.mozilla.org/?tree=Try&rev=d328c7399412

And I can't seem to reproduce either locally. Sigh.
Attached patch refactor_mozrunner (obsolete) — — Splinter Review 
Ok, I still haven't fixed those last two errors, but I have some ideas and am waiting for try results. I think at this point any changes will be very minor, so I'd like to get started with the review process.

This is just mozbase. I'll get mdas to review the marionette parts and jgriffin to review the other test harness pieces, just to save Will some pain. Since those depend on this, I won't ask review for them just yet.

Will, let me know if you'd prefer a reviewboard review or something.
Attachment #8435166 - Attachment is obsolete: true
Attachment #8439405 - Flags: review?(wlachance)
Attached patch refactor_marionette (obsolete) — — Splinter Review 
The marionette pieces of the puzzle.
Attached patch refactor_other (obsolete) — — Splinter Review 
Updating test harnesses and other random files for the new mozrunner/marionette.
Comment on attachment 8439405 [details] [diff] [review]
refactor_mozrunner

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

Overall this seems like a nice step forward. I have a bunch of comments on the documentation.

r+ with those addressed and the devicemanager changes removed.

I would like to clarify the following questions in the future: 

(1) mozrunner's relationship with marionette (if any)
(2) Can we remove any expectation of seeing specific test output in the device runner?

::: testing/mozbase/docs/mozrunner.rst
@@ +2,5 @@
> +============================================================
> +
> +Mozrunner provides management of gecko processes, whether it is a local
> +binary like Firefox, or an operating system running on a mobile device
> +or emulator like Firefox OS.

Sorry to be nitpicky, but I think there's a few problems with this pargraph:

* The Gecko application is just one part of Firefox OS, and strictly speaking it's just a process, not a whole operating system.
* The local binary terminology I think is a bit overly technical

I think this might work better:

"Mozrunner provides an API to manage a gecko-based application with an arbitrary configuration profile. It currently supports both desktop Firefox and the FirefoxOS core application on mobile devices and emulators."

@@ +4,5 @@
> +Mozrunner provides management of gecko processes, whether it is a local
> +binary like Firefox, or an operating system running on a mobile device
> +or emulator like Firefox OS.
> +
> +Mozrunner is a specialized wrapper around mozprocess that accepts or

I don't think calling mozrunner a wrapper is very helpful, nor do I think we really need to mention mozprocess (it's an implementation detail). mozprofile is more important, but we can talk about it later. I'd probably just nuke this paragraph.

@@ +22,5 @@
> +    runner = FirefoxRunner(binary=binary)
> +    runner.start()
> +    runner.wait()
> +
> +This automatically creates and uses a default mozprofile object. If you

I wonder if we should link to the mozprofile section of the documentation here. Might be helpful.

@@ +35,5 @@
> +
> +    binary = 'path/to/firefox/binary'
> +    profile_path = 'path/to/profile'
> +    if os.path.exists(profile_path):
> +        profile_obj = Profile.clone(path_from=profile_path)

profile_obj -> profile

@@ +42,5 @@
> +    runner = FirefoxRunner(binary=binary, profile=profile_obj)
> +    runner.start()
> +    runner.wait()
> +
> +See the mozprofile documentation for how to create customized profiles.

I think you can remove this sentence. It should be obvious from the above that the mozprofile documentation is where to look.

@@ +48,5 @@
> +
> +Handling output
> +---------------
> +
> +By default, mozrunner dumps output to stdout. It is possible to add arbitrary

dumps output to stdout -> dumps the output of the gecko process to standard output

@@ +49,5 @@
> +Handling output
> +---------------
> +
> +By default, mozrunner dumps output to stdout. It is possible to add arbitrary
> +output handlers by passing them in to mozprocess via the `process_args`

add handler functions to process the output via the `process_args` argument

(I think we can omit the mention of mozprocess here)

@@ +66,5 @@
> +    process_args = { 'stream': sys.stdout,
> +                     'processOutputLine': [handle_output_line] }
> +    runner = FirefoxRunner(binary=binary, process_args=process_args)
> +
> +See the mozprocess documentation for a full list of available arguments.

Ok, this is one place where I think it's fair to mention mozprocess. :) Say something like:

Mozrunner uses mozprocess to manage the gecko process and handle output. For documentation on the available arguments for the process handlers, see the mozprocess documentation.

(link to mozprocess above)

@@ +72,5 @@
> +
> +Handling timeouts
> +-----------------
> +
> +Sometimes gecko can hang, or maybe it is just taking too long. In this case you

In this case you may want to set timeouts for it. -> To handle this case you may want to set a timeout.

@@ +78,5 @@
> +traditional `timeout`, and the `outputTimeout`. These get passed into the
> +`runner.start()` method. Setting `timeout` will cause gecko to be killed after
> +the specified number of seconds, no matter what. Setting `outputTimeout` will cause
> +gecko to be killed after the specified number of seconds with no output. In both
> +cases the process handler's `onTimeout` callbacks will be fired.

fired -> triggered

@@ +111,5 @@
> +
> +Using a device runner
> +---------------------
> +
> +The previous examples all used a GeckoRuntimeRunner. If you want to control a

all used -> used

@@ +112,5 @@
> +Using a device runner
> +---------------------
> +
> +The previous examples all used a GeckoRuntimeRunner. If you want to control a
> +gecko process on a remote device, you need to use a DeviceRunner. The api is

remote device -> device
api -> API

@@ +114,5 @@
> +
> +The previous examples all used a GeckoRuntimeRunner. If you want to control a
> +gecko process on a remote device, you need to use a DeviceRunner. The api is
> +nearly identical except you don't pass in a binary, instead you create a device
> +object. For example, for B2G emulators you might do:

B2G -> B2G (Firefox OS)

::: testing/mozbase/mozdevice/mozdevice/devicemanager.py
@@ +374,5 @@
>          :param timeout: specified in seconds, defaults to 'default_timeout'
>          :param root: Specifies whether command requires root privileges
>          """
>  
> +    def shellOutput(self, cmd, env=None, cwd=None, timeout=None, root=False):

I think you meant to take this out? Nothing uses it.

::: testing/mozbase/mozrunner/mozrunner/base/device.py
@@ +76,5 @@
> +        else:
> +            print("timed out waiting for '%s' process to start" % self.app_ctx.remote_process)
> +
> +    def on_output(self, line):
> +        match = re.findall(r"TEST-START \| ([^\s]*)", line)

I think this stuff doesn't really belong in mozrunner. I know it was there before so I don't want to block this bug on it, but can we file a bug to take it out and put it in the test harness itself somehow?

::: testing/mozbase/mozrunner/mozrunner/devices/emulator.py
@@ +190,5 @@
> +            else:
> +                self._rotate_log(destlog, index+1)
> +        shutil.move(srclog, destlog)
> +
> +    # TODO this function is B2G specific and shouldn't live here

What's the plan for getting rid of these b2g specific things? It seems like they're the only reason why mozrunner depends on marionette currently. Not that big a deal I guess, but still...
Attachment #8439405 - Flags: review?(wlachance) → review+
(In reply to William Lachance (:wlach) from comment #21)
> I would like to clarify the following questions in the future: 
> 
> (1) mozrunner's relationship with marionette (if any)
> (2) Can we remove any expectation of seeing specific test output in the
> device runner?

1) Marionette uses mozrunner to start the gecko/emulator processes. Aside from those two methods you found in emulator.py, mozrunner doesn't depend on marionette anymore. I'll get to those below.

2) I think you mean the on_output handler? Yeah, I can file a bug for that.. though the last_test variable is used to print out which tests timed out, etc. We'll have to decide whether that's something we want to make every harness do on their own, or mozrunner should provide. At least when structured logging lands, we shouldn't need to use a regex anymore.


> Sorry to be nitpicky, but I think there's a few problems with this pargraph:

No worries, they're good suggestions!


> > +    def shellOutput(self, cmd, env=None, cwd=None, timeout=None, root=False):
> 
> I think you meant to take this out? Nothing uses it.

It's used in the refactor_marionette patch (probably should have included this change there). I need a way to get the output of a shell call without raising an exception if there is a non-zero return code. If you prefer, I can pass a 'raises=True' argument into shellCheckOutput instead, but I think you said you weren't a fan of that. I think this way aligns somewhat with the subprocess module (check_output vs check_call).

> > +    def on_output(self, line):
> > +        match = re.findall(r"TEST-START \| ([^\s]*)", line)
> 
> I think this stuff doesn't really belong in mozrunner. I know it was there
> before so I don't want to block this bug on it, but can we file a bug to
> take it out and put it in the test harness itself somehow?

Sure, though taking this out means taking out the timeout handling and the test name from the crash reporting. We'd have to re-implement that in every harness that uses mozrunner. Once structured logging lands, we can at least stop using the regex.


> > +    # TODO this function is B2G specific and shouldn't live here
> 
> What's the plan for getting rid of these b2g specific things? It seems like
> they're the only reason why mozrunner depends on marionette currently. Not
> that big a deal I guess, but still...

I'm not really sure.. they are kind of awkward functions that don't belong in marionette. I think the best way forward is to rename them to something more generic like "wait_for_application_loaded"
oops, clicked save by accident..

and then move the logic into the application specific classes. Though this is a bit tricky to see what it would look like without a second application using the device runner. As for depending on marionette, unfortunately there's not much we can do about that. I think passing in a marionette object is the best thing to do, or maybe we can create one if none get passed in.
The last_test thing seems to assume a particular design of the tests being run, which isn't generally applicable (although it might be in lots of cases). With web-platform-tests, for example, the harness is always in control so it doesn't need to reconstruct the status by examining the output. OTOH, wpt literally just uses mozrunner to launch firefox processes with a clean profile and certain prefs set, so maybe it isn't the main target of this code.
Attached patch refactor_marionette (obsolete) — — Splinter Review 
Fixes a bug I noticed when using connect_to_running_emulator and updates the mozdevice dependency. Malini, would you like me to update marionette itself too? I don't know when that usually happens.
Attachment #8439406 - Attachment is obsolete: true
Attachment #8439935 - Flags: review?(mdas)
(In reply to James Graham [:jgraham] from comment #24)
> The last_test thing seems to assume a particular design of the tests being
> run, which isn't generally applicable (although it might be in lots of
> cases). With web-platform-tests, for example, the harness is always in
> control so it doesn't need to reconstruct the status by examining the
> output. OTOH, wpt literally just uses mozrunner to launch firefox processes
> with a clean profile and certain prefs set, so maybe it isn't the main
> target of this code.

Yeah, I agree. Mozrunner might not even be running tests at all.. I'll file a follow up and we can move discussion to there since as Will noted, this patch isn't changing anything in that regard.
Blocks: 1025051
Also to note, I scanned through gaia-ui-tests for uses of marionette/mozrunner and it doesn't look like anything will need to be updated. But cc'ing zac so he is aware this is coming (just in case).
Attached patch refactor_other (obsolete) — — Splinter Review 
This just fixes some leftover debug statements I found.
Attachment #8439407 - Attachment is obsolete: true
Attachment #8439959 - Flags: review?(jgriffin)
Comment on attachment 8439959 [details] [diff] [review]
refactor_other

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

::: build/mobile/b2gautomation.py
@@ +193,5 @@
>          # Wait for a bit to make sure B2G has completely shut down.
>          time.sleep(10)
>          self._devicemanager._checkCmd(['shell', 'start', 'b2g'])
>          if self._is_emulator:
> +            self.marionette.emulator.wait_for_port(self.marionette.port)

Can't we just use self.marionette.wait_for_port() here?

@@ +261,5 @@
>                                             'tcp:%s' % self.marionette.port,
>                                             'tcp:%s' % self.marionette.port])
>  
>          if self._is_emulator:
> +            self.marionette.emulator.wait_for_port(self.marionette.port)

ditto.

::: layout/tools/reftest/mach_commands.py
@@ +335,5 @@
>          help='Path to busybox binary to install on device')
>      func = busybox(func)
>  
> +    logdir = CommandArgument('--logdir', default=None,
> +        help='directory to store log dump files')

What are 'log dump' files?  If we mean more than logcat files, maybe we should just say log files.

::: layout/tools/reftest/runreftestb2g.py
@@ +102,5 @@
>                          "be installed on the emulator prior to test")
>          defaults["geckoPath"] = None
> +        self.add_option("--logdir", action="store",
> +                        type="string", dest="logdir",
> +                        help="directory to store log dump files")

Same as above.

@@ +524,5 @@
>                    'deviceRoot': options.remoteTestRoot}
>          if options.deviceIP:
>              kwargs.update({'host': options.deviceIP,
>                             'port': options.devicePort})
> +        dm = DeviceManagerADB(**kwargs)

Ha!  I guess no one was using this on emulator...

::: testing/mochitest/mach_commands.py
@@ +555,5 @@
>          help='Path to busybox binary to install on device')
>      func = busybox(func)
>  
> +    logdir = CommandArgument('--logdir', default=None,
> +        help='directory to store log dump files')

Same as above.

::: testing/mochitest/runtestsb2g.py
@@ -16,5 @@
>  sys.path.insert(0, here)
>  
>  from runtests import Mochitest
>  from runtests import MochitestUtilsMixin
> -from runtests import MochitestOptions

Oops, good catch.

::: testing/xpcshell/runxpcshelltests.py
@@ +231,5 @@
>                  self.has_failure_output = True
>  
>          return proc.communicate()
>  
> +    def launchProcess(self, cmd, stdout, stderr, env, cwd, timeout=None):

We don't use 'timeout' here; is this a no-op and only present for supporting the remote version?  If so, we should probably add a comment to that effect.
Attachment #8439959 - Flags: review?(jgriffin) → review+
(In reply to Jonathan Griffin (:jgriffin) from comment #29)
> ::: build/mobile/b2gautomation.py
> @@ +193,5 @@
> >          # Wait for a bit to make sure B2G has completely shut down.
> >          time.sleep(10)
> >          self._devicemanager._checkCmd(['shell', 'start', 'b2g'])
> >          if self._is_emulator:
> > +            self.marionette.emulator.wait_for_port(self.marionette.port)
> 
> Can't we just use self.marionette.wait_for_port() here?

I think so, but there's a bug on file about removing that function from marionette.. so I figured I'd use the one in emulator.py.
 

> ::: layout/tools/reftest/mach_commands.py
> @@ +335,5 @@
> >          help='Path to busybox binary to install on device')
> >      func = busybox(func)
> >  
> > +    logdir = CommandArgument('--logdir', default=None,
> > +        help='directory to store log dump files')
> 
> What are 'log dump' files?  If we mean more than logcat files, maybe we
> should just say log files.

Yeah, it's logcat + qemu.log so calling it logcat-dir seemed confusing. I agree, I'll update the help message.

> @@ +524,5 @@
> >                    'deviceRoot': options.remoteTestRoot}
> >          if options.deviceIP:
> >              kwargs.update({'host': options.deviceIP,
> >                             'port': options.devicePort})
> > +        dm = DeviceManagerADB(**kwargs)
> 
> Ha!  I guess no one was using this on emulator...

Yep.. nothing to see there..

> ::: testing/xpcshell/runxpcshelltests.py
> @@ +231,5 @@
> >                  self.has_failure_output = True
> >  
> >          return proc.communicate()
> >  
> > +    def launchProcess(self, cmd, stdout, stderr, env, cwd, timeout=None):
> 
> We don't use 'timeout' here; is this a no-op and only present for supporting
> the remote version?  If so, we should probably add a comment to that effect.

Yeah, I noticed that there's a thing in the manifest that lets tests request a longer timeout. Except on remote this didn't work because the dm.shell() call would timeout first raising an exception. I'll add a comment explaining this.
(In reply to Andrew Halberstadt [:ahal] from comment #25)
> Created attachment 8439935 [details] [diff] [review]
> refactor_marionette
> 
> Fixes a bug I noticed when using connect_to_running_emulator and updates the
> mozdevice dependency. Malini, would you like me to update marionette itself
> too? I don't know when that usually happens.

Do you mean the marionette version number? we do that when someone requests a new version since they aren't using in-tree marionette, or we're breaking things and need to refer to release a specific version, so we won't need that unless it's necessary.
It could potentially break something out of tree that isn't pegged to an earlier version.. though I've looked through gaiatest and it seems like it will be ok. I don't know what else out of tree might depend on it.
Comment on attachment 8439935 [details] [diff] [review]
refactor_marionette

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

::: testing/marionette/client/marionette/runner/mixins/b2g.py
@@ +14,2 @@
>      elif marionette and marionette.device_serial and dm_type == 'adb':
> +        return mozdevice.DeviceManagerADB(deviceSerial=marionette.device_serial, **kwargs)

IIUC, it looks like this logic will return the existing dm if we have a b2g emulator or device, else will create one if that's not the case and I just have a device device serial and 'adb' set. 

If I run this against a device with a specific serial, then wouldn't it return me a DeviceManager object that doesn't take my serial number into consideration? the dm() code in application.py doesn't take in the device serial. How will it execute the elif line in this case?
(In reply to Malini Das [:mdas] from comment #33)
> If I run this against a device with a specific serial, then wouldn't it
> return me a DeviceManager object that doesn't take my serial number into
> consideration? the dm() code in application.py doesn't take in the device
> serial. How will it execute the elif line in this case?

So it looks like the else block is only used for non-emulator devices, is that right? If that's true then in the emulator case, the emulator object already has a dm object that is connected to it, so there's no need to create a new one. In the case that it *isn't* an emulator, then marionette.runner will be None and the first condition in the if statement will be false, so we should hit the else clause.

You raise a good point though, this will likely need to change when we start using mozrunner in marionette for non-emulator devices (this isn't quite ready yet). I don't mind changing this if you have a better way.
(In reply to Andrew Halberstadt [:ahal] from comment #34)
> (In reply to Malini Das [:mdas] from comment #33)
> > If I run this against a device with a specific serial, then wouldn't it
> > return me a DeviceManager object that doesn't take my serial number into
> > consideration? the dm() code in application.py doesn't take in the device
> > serial. How will it execute the elif line in this case?
> 
> So it looks like the else block is only used for non-emulator devices, is
> that right? If that's true then in the emulator case, the emulator object
> already has a dm object that is connected to it, so there's no need to
> create a new one. In the case that it *isn't* an emulator, then
> marionette.runner will be None and the first condition in the if statement
> will be false, so we should hit the else clause.
> 
Ah, okay I understand the logic now.
> You raise a good point though, this will likely need to change when we start
> using mozrunner in marionette for non-emulator devices (this isn't quite
> ready yet). I don't mind changing this if you have a better way.

This will be fine for now, thanks!
Attachment #8439935 - Flags: review?(mdas) → review+
Here's all three patchs folded into one with all review comments addressed. This is to make it easier to back out. Carry r pluses forward.

I was worried about some bitrot + changes for the nits, so did another full try run:
https://tbpl.mozilla.org/?tree=Try&rev=f23a389cdcc5

The windows opt X fail is due to a clobber issue. The emulator Mnw was a mistake I made fixing the shellOutput stuff, for extra cautiousness I did another try push just for that here:
https://tbpl.mozilla.org/?tree=Try&rev=5154d3557495

I'll land once that second run comes back green.
Attachment #8439405 - Attachment is obsolete: true
Attachment #8439935 - Attachment is obsolete: true
Attachment #8439959 - Attachment is obsolete: true
Attachment #8442078 - Flags: review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/81a51de30a2a

I'll upload the new mozrunner and mozdevice versions once this hits m-c. Needinfo'ing myself to remember.
Flags: needinfo?(ahalberstadt)
Blocks: 1023935
Blocks: 1027574
https://hg.mozilla.org/mozilla-central/rev/f5d1163268ae
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Depends on: 1028119
Depends on: 1027607
Mozrunner 6.0 has been released.
Flags: needinfo?(ahalberstadt)
FYI, we have to backport the changes for TPS to aurora and beta. Given that the full mozrunner patch here will not be backported (right?), I will do this on bug 1033271. Andrew, please let me know about the status of this patch. Thanks.
status-firefox31: --- → ?
status-firefox32: --- → ?
status-firefox33: --- → fixed
Flags: needinfo?(ahalberstadt)
I think pegging tpsrunner to the latest versions of mozbase on those branches is the safer bet.. but if it easier to backport the necessary changes, then let's do that. Do you need me to help land them?
Flags: needinfo?(ahalberstadt)
Nope, it's a single file I have to get landed on the other branches. So I will do this on bug 1033271. Also I assume this patch is a wontfix for 32.0 and 31.0.
status-firefox31: ? → wontfix
status-firefox32: ? → wontfix
Blocks: 1050756
No longer depends on: 1079890
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: