I started work on this in bug 865349 but decided to split it into it's own bug to keep things cleaner.

Comment on attachment 748057 [details] [diff] [review] WIP 1.0 - refactor mozrunner, add mozrunner remote class Review of attachment 748057 [details] [diff] [review]: ----------------------------------------------------------------- The B2G start() method seems not quite generic enough, since it handles the post-start execution of a Marionette script that our test runners happen to need. I wonder if wouldn't be better to separate out this logic (basically everything after 'self.process_handler.run') and put it back in the test runner, or possibly another integration point? If we did that, we could make this class not depend on Marionette, which is probably a good idea. This class defines several private methods that aren't used internally, like _restart_b2g. We should either make these public or move them out of here.

Sorry for swamping you with patches jgriffin, but this is basically the b2gautomation.py replacement and you are the best person for that. Will, I know you said you wanted runner.py to be even more generic since fennec likely won't use mozprocess. I decided to leave as is for now because it is possible to pass in a custom "process_class" into the constructor. So fennec could just provide it's own custom process class instead of mozprocess, but maybe this isn't good enough? Anyway, it's certainly up for debate.

Should mention to make review easier.. local.py is a copy/paste of what used to be in runner.py minus a few functions I thought were generic enough to stay in runner.py. remote.py is where most of the new stuff is.

(In reply to Andrew Halberstadt [:ahal] from comment #3) > Will, I know you said you wanted runner.py to be even more generic since > fennec likely won't use mozprocess. I decided to leave as is for now because > it is possible to pass in a custom "process_class" into the constructor. So > fennec could just provide it's own custom process class instead of > mozprocess, but maybe this isn't good enough? Anyway, it's certainly up for > debate. I really don't think "process" is the appropriate high-level abstraction for Fennec on Android. On Android the real object that you manipulate is an "application", not a process. And applications don't have standard out / standard error, nor can you can control them directly after they've started (starting an android app is done through a utility called am which just forks, starts the process, dumps a few statistics and then immediately exits). So yes, I'd like to make the high-level mozrunner more generic, without assuming that we're directly controlling a process or capturing its standard output/error. We've tried doing that with Talos and Mozilla-Central on Android and IMO the results are quite confusing. :) To be clear, I think it's OK if we do that at a low-level, but I think it'd be better if the user of mozrunner was only concerned with the "application" that they are running (whether that be B2G, desktop firefox, fennec for android, whatever) That said, it doesn't seem like your changes make anything *worse* than it is now, and oddly enough, it seems like the way b2g is started/stopped/controlled is similar enough to desktop firefox so that this actually seems to work ok. And I think it'd be hard to properly support fennec without a proper use case, and I think making fennec testing use this inside mozilla-central is way too much to chew on in addition to this. So I think I'd be fine with this going in more or less as-is. We can always change the way things work in the future if the cost/benefit works out.

Comment on attachment 762851 [details] [diff] [review] Patch 1.0 - factor mozrunner into remote and local, pull b2gautomation into remote Review of attachment 762851 [details] [diff] [review]: ----------------------------------------------------------------- I know remote.py is mostly copypasted code, but I took the liberty of giving some feedback on where I think it could be improved. :) The only serious issues are waitForNet not being defined and the unused internal methods. That said, giving f+ per overall comment above. ::: mozrunner/mozrunner/remote.py @@ +125,5 @@ > + if not self.marionette.emulator: > + self.rebootDevice() > + time.sleep(5) > + #wait for wlan to come up > + if not self.waitForNet(): waitForNet does not seem to be defined @@ +129,5 @@ > + if not self.waitForNet(): > + raise Exception("network did not come up, please configure the network" + > + " prior to running before running the automation framework") > + > + self.dm._runCmd(['shell', 'stop', 'b2g']) Would prefer using shellCheckOutput here. @@ +137,5 @@ > + self.kp_kwargs['onTimeout'] = [self.on_timeout] > + self.process_handler = self.process_class(self.command, **self.kp_kwargs) > + self.process_handler.run(timeout=timeout, outputTimeout=outputTimeout) > + > + time.sleep(5) Wait are we waiting for here? Can we do a check for it, as opposed to arbitrarily waiting? @@ +142,5 @@ > + > + # Set up port forwarding again for Marionette, since any that > + # existed previously got wiped out by the reboot. > + if not self.marionette.emulator: > + self.dm._checkCmd(['forward', I'd rather not use a private API here. Essentially all we're doing here is shelling out to "adb forward tcp:xxx tcp:xxx". Maybe just do that with subprocess and be done with it? Maybe define an internal helper method to make that more convenient. @@ +197,5 @@ > + "out after %s seconds with no output", > + self.last_test, self.timeout) > + > + > + def _restart_b2g(self): As jgriffin mentioned, it doesn't look like this is actually used internally. Should we make this public or just delete? I'd prefer deletion unless there's some obvious use case. @@ +200,5 @@ > + > + def _restart_b2g(self): > + # TODO hangs in subprocess.Popen without this delay > + time.sleep(5) > + self.dm._checkCmd(['shell', 'stop', 'b2g']) shellCheckOutput @@ +203,5 @@ > + time.sleep(5) > + self.dm._checkCmd(['shell', 'stop', 'b2g']) > + # Wait for a bit to make sure B2G has completely shut down. > + time.sleep(10) > + self.dm._checkCmd(['shell', 'start', 'b2g']) shellCheckOutput @@ +209,5 @@ > + self.marionette.emulator.wait_for_port() > + > + def _reboot_device(self): > + serial, status = self.getDeviceStatus() > + self.dm._runCmd(['shell', '/system/bin/reboot']) shellCheckOutput @@ +234,5 @@ > + # otherwise we use the (presumably only) device shown in 'adb devices'. > + serial = serial or self.dm._deviceSerial > + status = 'unknown' > + > + for line in self.dm._runCmd(['devices']).stdout.readlines(): See comment above about forwarding. @@ +249,5 @@ > + Copy profile and update the remote profiles ini file to point to the new profile > + """ > + > + # copy the profile to the device. > + self.dm._checkCmdAs(['shell', 'rm', '-r', self.remote_profile]) Use shellCheckOutput here, if you can. @@ +259,5 @@ > + > + # Copy the extensions to the B2G bundles dir. > + extension_dir = os.path.join(self.profile.profile, 'extensions', 'staged') > + # need to write to read-only dir > + self.dm._checkCmdAs(['remount']) See comment above about forwarding @@ +261,5 @@ > + extension_dir = os.path.join(self.profile.profile, 'extensions', 'staged') > + # need to write to read-only dir > + self.dm._checkCmdAs(['remount']) > + for filename in os.listdir(extension_dir): > + self.dm._checkCmdAs(['shell', 'rm', '-rf', shellCheckOutput again.

Comment on attachment 762851 [details] [diff] [review] Patch 1.0 - factor mozrunner into remote and local, pull b2gautomation into remote Review of attachment 762851 [details] [diff] [review]: ----------------------------------------------------------------- I think this is a good overall abstraction. I'm r-'ing primarily because there's too much harness-specific logic that's crept in, that I think best belongs either in the testrunners or some other layer. Mozrunner itself shouldn't make many assumptions about how consumers will end up wanting to use it (e.g., running a Marionette script after B2G is launched). ::: mozrunner/mozrunner/remote.py @@ +137,5 @@ > + self.kp_kwargs['onTimeout'] = [self.on_timeout] > + self.process_handler = self.process_class(self.command, **self.kp_kwargs) > + self.process_handler.run(timeout=timeout, outputTimeout=outputTimeout) > + > + time.sleep(5) I agree with Will. Our original attempts at porting these harnesses to B2G involved some hacks (like the use of time.sleep) because we were under a tremendous amount of time pressure, but now that we're not, let's try to avoid these. @@ +149,5 @@ > + > + if self.marionette.emulator: > + self.marionette.emulator.wait_for_port() > + else: > + time.sleep(5) wait_for_port() now exists as a top-level API in Marionette, so this block can be replaced by: self.marionette.wait_for_port() @@ +154,5 @@ > + > + # start a marionette session > + session = self.marionette.start_session() > + if 'b2g' not in session: > + raise Exception("bad session value %s returned by start_session" % session) This code and down is very harness-specific, so probably shouldn't live in mozrunner, but in the B2G mochitest runner. @@ +189,5 @@ > + def on_output(self, line): > + print line > + match = re.findall(r"TEST-START \| ([^\s]*)", line) > + if match: > + self.last_test = match[-1] This is all harness-specific as well. @@ +194,5 @@ > + > + def on_timeout(self): > + self.log.testFail("%s | application timed " > + "out after %s seconds with no output", > + self.last_test, self.timeout) Harness-specific.

drive by notes: + proxy = DEFAULT_PORTS Yes, I realize that you didn't change this but....you don't want assignment. Since we change proxy, this will update the global DEFAULT_PORTS, which ABICT isn't what is desired. Instead, proxy = DEFAULT_PORTS.copy() suffices here. > I agree with Will. Our original attempts at porting these > harnesses to B2G involved some hacks (like the use of time.sleep) > because we were under a tremendous amount of time pressure, but now > that we're not, let's try to avoid these. I agree with Jonathan ;) And would add that even under time pressure I would recommend documenting why the (e.g.) sleep and its value. Otherwise, it becomes really mysterious. (bonus points for a quick failing test illustrating what happens if Something Bad Happen which if you're clever may even help you test your code and determine the proper value). > + def __init__(self, profile, devicemanager, > + clean_profile=None, > + process_class=None, > + env=None, > + remote_test_root=None, > + restore=True): Personally, I'm not too much of a fan of this style (self+two args on the first line, the rest one), but I don't really care. (I'm also not a fan of super in the case, as this, when you know what the parent ctor you want is....for all the reasons given in https://fuhm.net/super-harmful/ , but at least as much for the fact it feels a bad fit for python, for one being implicit and traversing the ancestor tree v just calling explicitly what is wanted. This isn't to knock super across the board, but seems silly in this -- and TBH most -- case(s))

Wow, thanks for all the feedback! I think pretty much everything everyone said makes sense, I'll go through it all and get a new patch uploaded.

I think the only thing I left was the B2G specific comments because they are all in the B2GRunner subclass. All the code in there can be used by all the B2G harnesses, so I don't think runtestsb2g.py is the best place for them (maybe mozbase also isn't the right place, but I don't know of a better solution in the short term).

(In reply to Andrew Halberstadt [:ahal] from comment #10) > Created attachment 767392 [details] [diff] [review] > Patch 2.0 - Address feedback > > I think the only thing I left was the B2G specific comments because they are > all in the B2GRunner subclass. All the code in there can be used by all the > B2G harnesses, so I don't think runtestsb2g.py is the best place for them > (maybe mozbase also isn't the right place, but I don't know of a better > solution in the short term). I think we really don't want this in mozrunner. Mozrunner is designed as a generic runner, not just for in-tree testrunner support. I think we need to figure out how to share code between the harnesses that doesn't live in mozbase.

Comment on attachment 767392 [details] [diff] [review] Patch 2.0 - Address feedback During the mozbase work week we decided to leave the patch as is for now while we figure out how to handle shared code that doesn't quite belong in mozbase going forward.

Comment on attachment 767392 [details] [diff] [review] Patch 2.0 - Address feedback I very much suspect we will eventually want/need a cleaner separation between the runner and "things that use the runner", but as discussed last week, we can take this as a first iteration.

Issue in bug 865349 is resolved so I'm going to go ahead and get this landed so I can start syncing mozbase modules to m-c. Also removing Fennec from the title as that never ended up happening and I don't want to cause confusion. I'll take care of the try runs and such in the bug to sync it over (this also got pushed succesfully to try multiple times when I was debugging 865349, albeit mochitest only).

https://github.com/ahal/mozbase/commit/a659a23094695b8b5c93c0a540996b64a414c58a Also did version bumps for the three modules this patch touched, tagged and will release to pypi shortly.

(In reply to Andrew Halberstadt [:ahal] from comment #15) > https://github.com/ahal/mozbase/commit/ > a659a23094695b8b5c93c0a540996b64a414c58a I assume you meant to paste the link for the official mozilla repository: https://github.com/mozilla/mozbase/commit/a659a23094695b8b5c93c0a540996b64a414c58a

Oops, thanks!