This bug will track the progress of moving b2g mochitests process management on top of mozbase instead of using automation.py. It will likely involve a fair bit of changes to mozbase, possibly outside of mozprocess as well. By the end I'd guess that 95+% of the functionality inside automation.py will exist somewhere in mozbase.

After staring at code for a couple of hours I think my general plan of attack will be as follows: 1) Create a RemoteRunner class in mozrunner. This will do the same thing as the normal Runner, i.e it will handle setting up the profile and starting the process. For the case of B2G mochitests it's likely I'll need to subclass this further, if I do, it will not be a part of mozrunner. RemoteRunner will not inherit from Runner. These two classes have very little in common, any common functionality I find I need I can implement as a mixin or similar. 2) RemoteRunner will take in a devicemanager object and won't make any distinction between ADB or SUT. This might take quite a bit of work once we decide to port android automation over, but we can revisit it then. 3) Remove all the profiles_ini munging that b2g mochitests do, this will be handled by the RemoteRunner class (or the b2g specific subclass of it). 4) Remove all environment creation from runtestsb2g.py and remove the dependency on automation.environment(). RemoteRunner will take care of default environment variables and the mochitest runner can pass in more specific ones. 5) Override Mochitest.runTests and call RemoteRunner here instead of automation.runApp (maybe at this point it doesn't even make sense to be subclassing Mochitest, in which case, stop subclassing it). 6) Start the second iteration which involves all the random crap that got stuffed into Mochitest.runTests and automation.runApp 7) At this point the only reference to automation.py should be in MochitestServer, which is a different story. As I go through this I will try my best to keep everything generic with fennec in mind. Though I'm sure I won't get it perfect.

(In reply to Andrew Halberstadt [:ahal] from comment #1) > After staring at code for a couple of hours I think my general plan of > attack will be as follows: > > 1) Create a RemoteRunner class in mozrunner. This will do the same thing as > the normal Runner, i.e it will handle setting up the profile and starting > the process. For the case of B2G mochitests it's likely I'll need to > subclass this further, if I do, it will not be a part of mozrunner. > RemoteRunner will not inherit from Runner. These two classes have very > little in common, any common functionality I find I need I can implement as > a mixin or similar. > > 2) RemoteRunner will take in a devicemanager object and won't make any > distinction between ADB or SUT. This might take quite a bit of work once we > decide to port android automation over, but we can revisit it then. I actually started work on this in a seperate branch about a year ago, and attempted to consolidate it with mozrunner: https://github.com/wlach/mozbase/blob/937a711c29a28537d3e3638bcd6f3e41bc9110ef/mozrunner/mozrunner/runner.py https://github.com/wlach/mozbase/blob/937a711c29a28537d3e3638bcd6f3e41bc9110ef/mozrunner/mozrunner/remoterunner.py I wound up abandoning it when I figured out that I needed a more abstract class in Eideticker for launching apps in general, not just fennec. In retrospect, I think the idea of trying to refactor the existing mozrunner class was a bad idea too. Better to create something entirely new. I don't know if I'd call it RemoteRunner though. In fact, I'm not sure if the idea of a "runner" class actually makes sense for b2g. In the case of firefox and fennec, there definitely is the concept of "starting" an application on the device with a specific profile, etc. Are we actually doing that in b2g? If not, perhaps another abstraction may make more sense. Anyway, feel free to reuse any of that code if it's useful to you. It's obviously a bit out of date.

(In reply to William Lachance (:wlach) from comment #2) > I wound up abandoning it when I figured out that I needed a more abstract > class in Eideticker for launching apps in general, not just fennec. Yeah, this is the tricky part. I think mozrunner should only be responsible for making sure the product being tested gets launched with the proper profile. In B2G we aren't "launching an app" like we are in Fennec. In B2G the system is the thing we are testing. For that reason I want to keep it very generic and very simple (maybe all it does is make sure the profile is set up properly and then send a command over devicemanager). > In fact, I'm not sure if the idea > of a "runner" class actually makes sense for b2g. In the case of firefox and > fennec, there definitely is the concept of "starting" an application on the > device with a specific profile, etc. Are we actually doing that in b2g? If > not, perhaps another abstraction may make more sense. Yes, B2G has profiles just like Firefox and Fennec. These profiles aren't exposed to users, but we do create custom ones and install them within our automation. > Anyway, feel free to reuse any of that code if it's useful to you. It's > obviously a bit out of date. Thanks! I'll take a look.

For anyone following along, this patch works for me locally. There is still a lot of work to do as it's probably impossible for anyone to follow and is littered with debug statements. But it kind of gives an idea of my general approach. Of note is how automation.py is only used for starting the Mochitest server and in the option parsing.

This is the mozbase side of the refactor. I add a RemoteRunner class with a B2GRunner subclass. This subclass takes care of all the profile handling and mozprocess integration. Feel free to provide feedback, but note that there is also a lot of work to be done here and this will be changing quite a bit before it's finished.

That reminds me, we have a few cases now where option-parsing code might benefit from a common component. Jeff actually has such a class, http://k0s.org/mozilla/hg/configuration/. We should discuss at the next mozbase meeting if this is something we should promote to mozbase, although this wouldn't necessarily be tied to the mochitest work.

Yeah, I'll add that to the agenda. For this case I was planning to just not inherit the CLI parser from automation.py, strip out all of the prefs that are irrelevant/dont work yet for b2g and create a new parser (even if there is some duplication of code).

Comment on attachment 747595 [details] [diff] [review] WIP 1.0 (gecko) - refactor b2g mochitest runner Review of attachment 747595 [details] [diff] [review]: ----------------------------------------------------------------- Overall you're removing a lot more code than you're adding, which is nice. ::: testing/mochitest/b2g_start_script.js @@ +43,5 @@ > +mm.addMessageListener("SPProcessCrashService", specialPowersObserver); > +mm.addMessageListener("SPPingService", specialPowersObserver); > +mm.addMessageListener("SpecialPowers.Quit", specialPowersObserver); > +mm.addMessageListener("SpecialPowers.Focus", specialPowersObserver); > +mm.addMessageListener("SPPermissionManager", specialPowersObserver); This kind of sucks, any reason we can't move this into the SpecialPowers code? I'm guessing the way that SpecialPowers currently registers its observers doesn't work on B2G for some reason? (Which is odd because it was specifically designed to work with OOP stuff.) ::: testing/mochitest/runtests.py @@ +777,5 @@ > self.automation.log.info("INFO | runtests.py | Received keyboard interrupt.\n"); > status = -1 > except: > + import traceback > + traceback.print_exc() I thought log.exception already gave you a traceback? ::: testing/mochitest/runtestsb2g.py @@ +151,5 @@ > + log.info("\nINFO | runtests.py | Running tests: end.") > + > + if manifest is not None: > + self.cleanup(manifest, options) > + return status Kinda sucks to repeat all this logic from automation.py, maybe we can find a middle ground? (even if that involves refactoring automation.py or runtests.py).

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #8) > Comment on attachment 747595 [details] [diff] [review] > WIP 1.0 (gecko) - refactor b2g mochitest runner > > Review of attachment 747595 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall you're removing a lot more code than you're adding, which is nice. > > ::: testing/mochitest/b2g_start_script.js > @@ +43,5 @@ > > +mm.addMessageListener("SPProcessCrashService", specialPowersObserver); > > +mm.addMessageListener("SPPingService", specialPowersObserver); > > +mm.addMessageListener("SpecialPowers.Quit", specialPowersObserver); > > +mm.addMessageListener("SpecialPowers.Focus", specialPowersObserver); > > +mm.addMessageListener("SPPermissionManager", specialPowersObserver); > > This kind of sucks, any reason we can't move this into the SpecialPowers > code? I'm guessing the way that SpecialPowers currently registers its > observers doesn't work on B2G for some reason? (Which is odd because it was > specifically designed to work with OOP stuff.) > The reason for this is that SpecialPowers uses the global message manager, which works with OOP windows, but not with OOP frames (such as those used by B2G apps); to load a script into an OOP frame, you need to use that frame's specific message manager. We could probably modify SpecialPowers to work better with B2G, if there's a way to reliably and globally detect when new iframes are created. But, we should do that in a separate bug. Some IPC mochitests create OOP frames and this kind of change would likely require some test revisions as well.

> ::: testing/mochitest/runtests.py > @@ +777,5 @@ > > self.automation.log.info("INFO | runtests.py | Received keyboard interrupt.\n"); > > status = -1 > > except: > > + import traceback > > + traceback.print_exc() > > I thought log.exception already gave you a traceback? It wasn't for some reason.. though this is just one of the debug statements I left in. > ::: testing/mochitest/runtestsb2g.py > @@ +151,5 @@ > > + log.info("\nINFO | runtests.py | Running tests: end.") > > + > > + if manifest is not None: > > + self.cleanup(manifest, options) > > + return status > > Kinda sucks to repeat all this logic from automation.py, maybe we can find a > middle ground? (even if that involves refactoring automation.py or > runtests.py). So this patch is the result of "Phase 1: Get something working somehow". I basically just copy + pasted stuff all over the place and started deleting chunks of code until I was left with something that could run without automation.py. I don't really expect this patch to look much like it does now by the time I'm finished.

Comment on attachment 747598 [details] [diff] [review] WIP 1.0 (mozbase) - Refactor mozrunner, add mozrunner remote class Decided to split the mozbase portion off into bug 870876 to keep things cleaner.

This is pretty much finished as far as what I had in mind. I call it WIP because it's still untested for the desktop mochitests. It also depends on the mozbase portion landing and the "use in-tree mozbase" bug landing (so I can push everything to try). This patch is pretty massive. I'll attempt to break into a series of smaller and more manageable patches when it's ready for review. That being said, feel free to provide feedback if you want.

Comment on attachment 757555 [details] [diff] [review] WIP 2.0 Review of attachment 757555 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/runtests.py @@ +60,5 @@ > > self.add_option("--appname", > action = "store", type = "string", dest = "app", > help = "absolute path to application, overriding default") > + defaults["app"] = os.path.join(SCRIPT_DIR, self._automation.DEFAULT_APP) You should use build_obj.get_binary_path() here. @@ +66,5 @@ > self.add_option("--utility-path", > action = "store", type = "string", dest = "utilityPath", > help = "absolute path to directory containing utility programs (xpcshell, ssltunnel, certutil)") > + if hasattr(build_obj, 'get_binary_path'): > + defaults["utilityPath"] = build_obj.get_binary_path() get_binary_path returns the path to the executable, you want "build_obj.bindir". I'd probably also just make this test "if build_obj is not None:". Also, we should just default utilityPath to None, requiring callers to pass it on the cmdline, since that removes one more automation.py dependency.

Using mozinfo in the above patch trips up fennec on import. I can either do a similar hack to xpcshell: http://mxr.mozilla.org/mozilla-central/source/testing/xpcshell/Makefile.in#45 Or wait for bug 829211 to land which will move fennec on top of mozharness. I'll leave it for now, until this is closer to being read to land.

I got a green try run for both b2g and desktop mochitests. I still need to test b2g desktop mochitests locally though. Don't bother testing these patches out locally as they are still dependent on several external pieces landing, but I figured I might as well get a head start on the review process while waiting.

Use the build_obj module and mozinfo instead of automation.py. I may still need to add a hack so that this doesn't break android mochitests. I'll make a separate patch for that when needed.

The option parsing for mochitest is really getting out of hand, I figured something needed to be done. I'm not really sure about the approach here, so feedback welcome. One thing to note is that since we can't always rely on automation.py giving us the utility/xre paths, we error out if the build_obj can't find them and they weren't provided. This means we'll need to modify the mozharness scripts to provide --utility-path as well as --xre-path.

Comment on attachment 762254 [details] [diff] [review] Patch 1.1 - use mozinfo Review of attachment 762254 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, I left this half-written comment open in a tab for several hours. ::: testing/mochitest/runtests.py @@ +546,5 @@ > + if hasattr(build_obj, 'topobjdir'): > + mozinfo_paths.insert(0, os.path.join(build_obj.topobjdir, 'mozinfo.json')) > + for path in mozinfo_paths: > + if os.path.isfile(path): > + mozinfo.update(path) I literally just landed a fix in mozinfo for this yesterday in bug 881417. You should be able to replace this with: mozinfo.find_and_update_from_json(SCRIPT_DIR) when that gets merged.

Comment on attachment 762257 [details] [diff] [review] Patch 1.2 - refactor options Review of attachment 762257 [details] [diff] [review]: ----------------------------------------------------------------- So this patch makes the options a little more readable, which is nice, but at the expense of decoupling the options from their verification steps, which I'm not sure is desirable. Ultimately, when desktop tests are mozbased, how will this look? B2G_OPTIONS = BASE_OPTIONS + [...]? It looks like we'd still have to import MochitestOptions in order to use its verifyOptions method. I'm not sure this buys us much. I wonder if we could instead define a set of option classes that would provide both their options and relevant verification steps as mixins, and then we could combine them? class ChunkOptions(object): def options(): return [...] def verifyOptions(**kwargs): ... class B2GOptions(ChunkOptions, ..., ...): I'm not sure that 'ChunkOptions' illustrates the right granularity, and the above is a little simplistic, but it would allow us to keep the verification along with the options while still allowing a more readable option format and cleaner inheritance. r- just for now just for further discussion. ::: testing/mochitest/runtests.py @@ +299,5 @@ > # default xrePath to the app path if not provided > # but only if an app path was explicitly provided > if options.app != self.defaults['app']: > options.xrePath = os.path.dirname(options.app) > + elif build_obj is not None: Where does 'build_obj' come from? ::: testing/mochitest/runtestsb2g.py @@ -303,5 @@ > - if options.remoteLogFile == None: > - options.remoteLogFile = options.remoteTestRoot + '/logs/mochitest.log' > - > - if options.remoteLogFile.count('/') < 1: > - options.remoteLogFile = options.remoteTestRoot + '/' + options.remoteLogFile Where does this code get handled in the new patch, or do we expect to have to explicitly define these options?

Comment on attachment 762253 [details] [diff] [review] Patch 1.0 - base refactor Review of attachment 762253 [details] [diff] [review]: ----------------------------------------------------------------- Cool! Looks like a good first iteration. I'm pretty sure this breaks tests on B2G desktop builds, but that should be easily fixable. ::: testing/mochitest/runtests.py @@ +302,5 @@ > if not os.path.exists(options.app): > msg = """\ > Error: Path %(app)s doesn't exist. > Are you executing $objdir/_tests/testing/mochitest/runtests.py?""" > + log.error(msg % {"app": options.app}) I don't see us defining 'log' anywhere for desktop mochitests, so this will break those, won't it? ::: testing/mochitest/runtestsb2g.py @@ +99,5 @@ > manifest = self.addChromeToProfile(options) > self.copyExtraFilesToProfile(options) > return manifest > > + def run_tests(self, options, onLaunch=None): We never use onLaunch. Looks like this is a holdover from the desktop definition that we can remove, since, with the name change from runTests -> run_tests, this is no longer overriding a method in the base class. @@ +114,5 @@ > + > + testURL = self.buildTestPath(options) > + self.buildURLOptions(options, env) > + if len(self.urlOpts) > 0: > + testURL += "?" + "&".join(self.urlOpts) We don't use testURL in this function anywhere. Looks like we can omit the 3 lines that involve it. @@ +125,5 @@ > + else: > + timeout = 330.0 # default JS harness timeout is 300 seconds > + > + log.info("runtestsb2g.py | Running tests: start.") > + env = {} Why reset env from the first definition above? @@ +137,5 @@ > + 'test_script': self.test_script, > + 'test_script_args': self.test_script_args } > + self.runner = B2GRunner(**runner_args) > + self.runner.start(outputTimeout=timeout) > + self.runner.wait() Will self.runner.wait() return a status code, or do we not check for an exit code since B2G is not stopped? @@ +530,5 @@ > > if options.desktop and not options.profile: > raise Exception("must specify --profile when specifying --desktop") > > + sys.exit(mochitest.run_tests(options, onLaunch=mochitest.startTests)) This instance actually uses the runTests method from runtests.py; we don't want it to use run_tests from B2GMochitest. We should verify this patch against b2g desktop mochitests when it's feasible. We'll have to do this manually, since it isn't run in TBPL. We won't get that part using mozprocess until we convert desktop mochitests.

(In reply to Jonathan Griffin (:jgriffin) from comment #19) > So this patch makes the options a little more readable, which is nice, but > at the expense of decoupling the options from their verification steps, > which I'm not sure is desirable. Yeah, I see your point. As the patch stands now it might be a bad idea to separate verifyOptions from the options themselves. Though longer term (probably out of scope for this patch) I'd like to A) stop passing around that options variable to every single function and B) just remove verifyOptions completely. I envision all options will either default to None or some simple value. For those options that currently require some complex calculations for their default values, it should just be left up to whatever class is consuming them to do the calculation. > Ultimately, when desktop tests are mozbased, how will this look? > > B2G_OPTIONS = BASE_OPTIONS + [...]? It looks like we'd still have to import > MochitestOptions in order to use its verifyOptions method. I'm not sure > this buys us much. Pretty much. At the very least it makes runtests*.py more readable and eventually all options will be in the same place. Other than that it doesn't really get us that much. > I wonder if we could instead define a set of option classes that would > provide both their options and relevant verification steps as mixins, and > then we could combine them? > > class ChunkOptions(object): > > def options(): > return [...] > > def verifyOptions(**kwargs): > ... > > class B2GOptions(ChunkOptions, ..., ...): > > I'm not sure that 'ChunkOptions' illustrates the right granularity, and the > above is a little simplistic, but it would allow us to keep the verification > along with the options while still allowing a more readable option format > and cleaner inheritance. I like this idea. It gives us the two benefits I mentioned above and still allows verification. I'm still not really a fan of the whole verifyOptions() concept (or at least the way verifyOptions is currently being used [1]), but I think this might be a good first step. > ::: testing/mochitest/runtests.py > @@ +299,5 @@ > > # default xrePath to the app path if not provided > > # but only if an app path was explicitly provided > > if options.app != self.defaults['app']: > > options.xrePath = os.path.dirname(options.app) > > + elif build_obj is not None: > > Where does 'build_obj' come from? It's a global variable, see the import of the build module at the top. > ::: testing/mochitest/runtestsb2g.py > @@ -303,5 @@ > > - if options.remoteLogFile == None: > > - options.remoteLogFile = options.remoteTestRoot + '/logs/mochitest.log' > > - > > - if options.remoteLogFile.count('/') < 1: > > - options.remoteLogFile = options.remoteTestRoot + '/' + options.remoteLogFile > > Where does this code get handled in the new patch, or do we expect to have > to explicitly define these options? I think I figured out that this code wasn't doing anything particularly useful (or wasn't being used anywhere?) and decided to just axe it. Sorry it was a while ago, but I can take a closer look and get back to you.

(In reply to Jonathan Griffin (:jgriffin) from comment #20) > Comment on attachment 762253 [details] [diff] [review] > Patch 1.0 - base refactor > > Review of attachment 762253 [details] [diff] [review]: > ----------------------------------------------------------------- > > Cool! Looks like a good first iteration. I'm pretty sure this breaks tests > on B2G desktop builds, but that should be easily fixable. Yes, I wouldn't be surprised if it did. I'll make sure to test it before landing. > ::: testing/mochitest/runtests.py > @@ +302,5 @@ > > if not os.path.exists(options.app): > > msg = """\ > > Error: Path %(app)s doesn't exist. > > Are you executing $objdir/_tests/testing/mochitest/runtests.py?""" > > + log.error(msg % {"app": options.app}) > > I don't see us defining 'log' anywhere for desktop mochitests, so this will > break those, won't it? It should be a global variable at the top. I did globals instead of instance variable so it would be possible to differentiate log messages that come from the different files. But I can change that if you want. > @@ +114,5 @@ > > + > > + testURL = self.buildTestPath(options) > > + self.buildURLOptions(options, env) > > + if len(self.urlOpts) > 0: > > + testURL += "?" + "&".join(self.urlOpts) > > We don't use testURL in this function anywhere. Looks like we can omit the > 3 lines that involve it. Good catch! > @@ +125,5 @@ > > + else: > > + timeout = 330.0 # default JS harness timeout is 300 seconds > > + > > + log.info("runtestsb2g.py | Running tests: start.") > > + env = {} > > Why reset env from the first definition above? Oops, the definition above should just be removed. Environment is handled by the mozrunner.B2GRunner class now. > > @@ +137,5 @@ > > + 'test_script': self.test_script, > > + 'test_script_args': self.test_script_args } > > + self.runner = B2GRunner(**runner_args) > > + self.runner.start(outputTimeout=timeout) > > + self.runner.wait() > > Will self.runner.wait() return a status code, or do we not check for an exit > code since B2G is not stopped? If a timeout happens, then the B2GRunner will print out a timeout error. So here, I don't think we really care whether it timed out or not, we want to do the same cleanup either way. I might be wrong about this though...

For posterity I did get green try runs out of these patches: desktop - https://tbpl.mozilla.org/?tree=Try&rev=27313f970aa3 b2g - https://tbpl.mozilla.org/?tree=Try&rev=6b0b75fbbefb

Nice!

I've addressed the feedback, though for some reason I'm not able to run b2g desktop mochitests locally (even without any patches applied). I'll spend some more time on it next week to test it out.

Carrying ted's r+ forward (I addressed his comment in this patch)

Is this any better? I basically just moved both desktop and b2g options into mochitest_options.py but preserved the class structure. I think there is still a fair amount of work to be done to fix mochitest option parsing but we can leave that to a future bug.

(In reply to Andrew Halberstadt [:ahal] from comment #25) > Created attachment 769257 [details] [diff] [review] > Patch 2.0 - base patch > > I've addressed the feedback, though for some reason I'm not able to run b2g > desktop mochitests locally (even without any patches applied). I'll spend > some more time on it next week to test it out. Looks like the patch is missing here.

Comment on attachment 769261 [details] [diff] [review] Patch 2.2 - mochitest options Review of attachment 769261 [details] [diff] [review]: ----------------------------------------------------------------- As we discussed last week, we may eventually want to change this to use a (to-be-developed) mozconfig package, and we may want to handle option verification differently, but this is a sane incremental step forward.

Weird, I didn't think it was even possible to upload an empty patch. Trying again.

Oh and regarding your comment about log not being defined, it looks like I accidentally put that hunk in part 2 when splitting the patch up with hg qcrecord. I'll move that hunk back to part 1 before pushing.

(In reply to Andrew Halberstadt [:ahal] from comment #31) > Oh and regarding your comment about log not being defined, it looks like I > accidentally put that hunk in part 2 when splitting the patch up with hg > qcrecord. I'll move that hunk back to part 1 before pushing. Yeah I saw that too, thanks.

Comment on attachment 770189 [details] [diff] [review] Patch 2.0 - base patch Review of attachment 770189 [details] [diff] [review]: ----------------------------------------------------------------- Great! Since b2g desktop mochitests seem to fail for you locally, want me to run them with this? ::: testing/mochitest/runtestsb2g.py @@ +109,5 @@ > + self.startWebServer(options) > + self.startWebSocketServer(options, None) > + self.buildURLOptions(options, {'MOZ_HIDE_RESULTS_TABLE': '1'}) > + > + # then again to actually run mochitest I don't understand this comment; is it leftover from a refactor? @@ +336,4 @@ > _dm = None > > + def __init__(self, marionette, devicemanager, profile_data_dir, local_binary_dir, > + remote_test_root=None, remote_log_file=None): nit: weird wrapping; should be aligned with opening delimiter @@ +468,5 @@ > if options.deviceIP: > kwargs.update({'host': options.deviceIP, > 'port': options.devicePort}) > dm = DeviceManagerADB(**kwargs) > + options = parser.verifyRemoteOptions(options ) nit: extra space before ) @@ +475,5 @@ > sys.exit(1) > > + mochitest = B2GDeviceMochitest(marionette, dm, options.profile_data_dir, options.xrePath, > + remote_test_root=options.remoteTestRoot, > + remote_log_file=options.remoteLogFile) nit: wrapping should be aligned with opening delimiter @@ +530,5 @@ > > if options.desktop: > run_desktop_mochitests(parser, options) > else: > + run_remote_mochitests( parser, options) nit: extra space before parser

The latest try run with updated patches works... but has 1725 failures :/ https://tbpl.mozilla.org/?tree=Try&rev=1e09fe13c108

Most of these seem to be one of: * content/base/test/test_CrossSiteXHR.html * content/base/test/test_blobconstructor.html * xhr related Maybe a pref is missing or something? Maybe it's just bug 870013?

There's also this error, which sounds suspicious: 11:47:24 INFO - 2661 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_blobconstructor.html | uncaught exception - SyntaxError: JSON.parse: unexpected character at http://mochi.test:8888/tests/content/base/test/fileutils.js:35

Definitely not that bug if you're getting 1725 failures. I'm not sure what's going on there, you might ask some people in #content.

Some extra data points: 1) all tests passed with patch v1 (https://tbpl.mozilla.org/?tree=Try&rev=6b0b75fbbefb) 2) all tests pass on desktop (https://tbpl.mozilla.org/?tree=Try&rev=b9229947d599) 3) this is reproducible locally I'll look into it some more tomorrow.

Hmm, went to run it again locally and it was working again. Haven't been able to do a second try push due to https://bugzilla.mozilla.org/show_bug.cgi?id=891114#c9

Boris, do you mind taking a quick look at https://tbpl.mozilla.org/php/getParsedLog.php?id=25174085&tree=Try&full=1#error30 and letting me know if there's anything you can think of that I might be doing wrong? I can't reproduce it locally. Much appreciated!

If we suspect this is a profile problem, could we compare the profile's generated with and without this patch, and see what the difference is?

(In reply to Jonathan Griffin (:jgriffin) from comment #41) > If we suspect this is a profile problem, could we compare the profile's > generated with and without this patch, and see what the difference is? As an aside, and definitely not a frontburner, I was thinking it could be useful to have a utility in mozprofile that "diffs" profiles

The log looks like the sjs files are just returning bogus things. Note this: 09:19:11 INFO - I/GeckoDump( 709): 31556 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug622088.html | Expected inner frame location - got <html> <head><title>500 Internal Server Error</title></head> <body> <h1>500 Internal Server Error</h1> <p>Something's broken in this server and needs to be fixed.</p> </body> </html>, expected http://mochi.test:8888/tests/content/base/test/file_bug622088_inner.html and the like..

(In reply to Boris Zbarsky (:bz) from comment #43) > The log looks like the sjs files are just returning bogus things. Note this: > > 09:19:11 INFO - I/GeckoDump( 709): 31556 ERROR TEST-UNEXPECTED-FAIL | > /tests/content/base/test/test_bug622088.html | Expected inner frame location > - got <html> <head><title>500 Internal Server > Error</title></head> <body> <h1>500 > Internal Server Error</h1> <p>Something's broken in > this server and needs to be fixed.</p> > </body> </html>, expected > http://mochi.test:8888/tests/content/base/test/file_bug622088_inner.html > > and the like.. Which should point us back to the profile and the PAC setup that httpd.js uses.

We could set DEBUG=true in httpd.js in a try run to get some output from it re: what's going on.

(In reply to Clint Talbert ( :ctalbert ) from comment #44) > Which should point us back to the profile and the PAC setup that httpd.js > uses. Yep, I've been pushing try runs like crazy, especially around the part of the patches that touch mozprofile/httpd.js. I've looked for differences in the profiles with/without this patch and haven't noticed anything (though maybe I just missed something?). The thing that gets me is that I can't reproduce it locally. If it were a profile problem I don't understand why this would manifest on one environment but not another. (In reply to Jonathan Griffin (:jgriffin) from comment #45) > We could set DEBUG=true in httpd.js in a try run to get some output from it > re: what's going on. Easier said than done. We'd need to upload a new xre.zip to tooltool and then modify the mozharness script to allow overriding that config with the in-tree one. I'm basically doing patch bisection, I've already eliminated half my patch as the source of the problem. It's a little painful, but I think it's the fastest way to figuring this out.

Ok I figured this out. Basically there were several things that happened that created a perfect storm. * This was fallout from bug 882932 which landed midway while I was writing these patches (explains why it worked before and not after) * Bug 882932 assumed b2g mochitests used utilityPath when in fact they used xrePath. I've since determined that utilityPath is completely unnecessary for b2g and will update this patch to get rid of it. * I made a subtle mistake when rebasing my changes on top of those changes * My laptop had an old build with an old version of httpd.js which explains why I could reproduce the problem there but not my desktop (basically I was hitting the same issue two different ways). * I actually almost had the fix a while ago, but was missing an os.path.abspath

Wow, Ahal, nice debugging.

Bug 870875 landed in github, now I need to mirror all the modules over in bug 895940. Meanwhile for this patch, the desktop mochitests all look good: https://tbpl.mozilla.org/?tree=Try&rev=4196f8bba66a I made a mistake re-fixing up the patch which made the same httpd.js problem occur. I fixed it (I believe) and pushed a test run for B2G + android here: https://tbpl.mozilla.org/?tree=Try&rev=e7521f8f2b5a

Doh, '-u None' on that last try run.. https://tbpl.mozilla.org/?tree=Try&rev=015904c46a3e

Had a mistake in that last try run, fixed and now both desktop and remote are looking good: https://tbpl.mozilla.org/?tree=Try&rev=05be88659527

This is just the previous three patches folded into one another. I'm going to land them all at once because the intermediate patches will likely break the tests on their own.

This appears to have broken running mochitests, probably for anything other than mobile. The fix appears trivial, but I am not familiar with this code. Line 111 in mach_commands.py calls MochitestOptions with 2 arguments, dropping the second seems to fix running tests. TypeError: __init__() got an unexpected keyword argument 'appname' File "/Users/shanec/moz/fx-team/testing/mochitest/mach_commands.py", line 280, in run_mochitest_browser return self.run_mochitest(test_file, 'browser', **kwargs) File "/Users/shanec/moz/fx-team/testing/mochitest/mach_commands.py", line 299, in run_mochitest **kwargs) File "/Users/shanec/moz/fx-team/testing/mochitest/mach_commands.py", line 111, in run_mochitest_test opts = mochitest.MochitestOptions(automation, appname=tests_dir) File "/Users/shanec/moz/fx-team/obj-rel/_tests/testing/mochitest/mochitest_options.py", line 324, in __init__ optparse.OptionParser.__init__(self, **kwargs)

(In reply to Shane Caraveo (:mixedpuppy) from comment #55) heh, sorry, seems Tim already created bug 898725.

I think this is also causing failures when trying to run mochitest-robocop.

Anyone run into this? Traceback (most recent call last): File "_tests/testing/mochitest/runtests.py", line 25, in <module> from mochitest_options import MochitestOptions File "t:\Mozilla\MC-REL\_tests\testing\mochitest\mochitest_options.py", line 12, in <module> from mozprofile import DEFAULT_PORTS ImportError: cannot import name DEFAULT_PORTS doing a simple pymake mochitest-metro-chrome ?