Exactly as the summary says. Let's make it easier to run remote reftests and crashtests locally.

And now we have three separate ways to invoke a sub-module in the same file (as a Python module; via Make; and via a Python command line). Aren't wrappers fun!

Why should it be a separate command? That is, why would anyone expect mach reftest to not run remote tests for android builds?

(In reply to Mike Hommey [:glandium] from comment #2) > Why should it be a separate command? That is, why would anyone expect mach > reftest to not run remote tests for android builds? Huh, I hadn't thought of that. Happy to s/-remote//.

(In reply to Nick Alexander :nalexander from comment #3) > (In reply to Mike Hommey [:glandium] from comment #2) > > Why should it be a separate command? That is, why would anyone expect mach > > reftest to not run remote tests for android builds? > > Huh, I hadn't thought of that. Happy to s/-remote//. Oh, I remember why I did it this way: the make command is reftest-remote; and this follows the b2g example. I agree that neither of these are valuable reasons to keep the -remote command name.

mach help: Testing: (...) crashtest Run crashtests (Check if crashes on a page). (...) reftest Run reftests (layout and graphics correctness). Those commands already exists, they should just trigger "remote" tests when the built app can't be run locally.

I approve of glandium's suggestion. Let's not take the make targets as gospel, they suffer from many problems due to the limits of make. mach commands are much more flexible, let's make things better!

Comment on attachment 8509939 [details] [diff] [review] Add |mach {reftest,crashtest}-remote| for mobile/android. r=gps Review of attachment 8509939 [details] [diff] [review]: ----------------------------------------------------------------- Let's axe the -remote. The -remote stems from the early days when we were doing 1:1 ports of make targets to make the initial rollout easier. Things like the various flavors of mochitest commands were required because there was no way to reliably identify what type of test each file was. Test manifests have changed that. My ultimate goal (I concede I'm not sure if it is achievable) is to have a single |mach test| that invokes the proper test harness automatically. We can do command argument groups in mach now (see |mach run|), which will make the hodgepodge of different suite arguments more manageable. For the cases where we wish to pass extra, per-suite arguments down to the test runner, we can adopt the convention of all arguments after "--" get proxied.

Changed nothing but the name: dropped -remote.

Comment on attachment 8512104 [details] [diff] [review] Add |mach {reftest,crashtest}| for mobile/android. r=gps Review of attachment 8512104 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine with the exception of the redundant @Command handlers. What's up with that? ::: layout/tools/reftest/mach_commands.py @@ +540,5 @@ > + > + > +@CommandProvider > +class AndroidCommands(MachCommandBase): > + @Command('reftest', category='testing', What about the @Command('reftest') earlier in this file? Having two @Command handlers for the same command feels very wrong. I'm a bit surprised mach is even allowing this to occur. ::: layout/tools/reftest/remotereftest.py @@ +486,5 @@ > if not options.ignoreWindowSize: > parts = dm.getInfo('screen')['screen'][0].split() > width = int(parts[0].split(':')[1]) > height = int(parts[1].split(':')[1]) > + if (width < 1366 or height < 1050): This looks like it doesn't belong.

:nalexander's patch has seriously bit-rotted now, but was still useful for inspiration. This works for me, for "mach reftest" and "mach reftest layout/reftests/reftest-sanity/reftest.list"; I'm not sure about other arguments: It needs some testing. :nalexander -- I won't be able to look at this again for at least 2 weeks. Would you consider picking it up and pushing it over the finish line?

(In reply to Geoff Brown [:gbrown] (pto Sept 21 - Oct 2) from comment #10) > Created attachment 8663183 [details] [diff] [review] > work in progress > > :nalexander's patch has seriously bit-rotted now, but was still useful for > inspiration. This works for me, for "mach reftest" and "mach reftest > layout/reftests/reftest-sanity/reftest.list"; I'm not sure about other > arguments: It needs some testing. > > :nalexander -- I won't be able to look at this again for at least 2 weeks. > Would you consider picking it up and pushing it over the finish line? Unfortunately, I am committed to two high priority projects right now, so I won't get to this in the next 2 weeks :( Thanks for thinking of me. sebastian, jonalmeida: y'all are doing some work that might want you to run reftests locally (in Bug 1063868). You might pick up and test this patch while you hack on that stuff.

Yeah, I have been running some reftests locally for that bug. What exactly does "remote" mean here?

(In reply to Sebastian Kaspari (:sebastian) from comment #12) > Yeah, I have been running some reftests locally for that bug. What exactly > does "remote" mean here? "Remote" means "not host". Like when cross-building, "target" means "not host". So "reftest" means run on the host machine; "remote" means run on some other target (in this case, an Android device or emulator). If you have been running reftests locally, you're well positioned to make sure the mach command is doing similar things and works!

(In reply to Nick Alexander :nalexander from comment #13) > If you have been running reftests locally, you're well positioned to make > sure the mach command is doing similar things and works! Sold! Setting NI to remember.

I'm back, so I'll take this back!

(In reply to Geoff Brown [:gbrown] from comment #15) > I'm back, so I'll take this back! Sorry, I actually didn't run a single reftest during that time. So I didn't test it locally just yet..

This adds |mach {reftest,crashtest,jstestbrowser}| support for android -- just like desktop. I test locally with/without a test path for each suite. https://treeherder.mozilla.org/#/jobs?repo=try&revision=8102d3bfaeb5 confirms this does no harm on treeherder. I fixed a couple of annoying typos that I encountered while testing and also suppressed the remote device info when running from mach (we did this for mochitests and robocop recently -- it makes the output much more readable in a terminal).

Comment on attachment 8673700 [details] [diff] [review] add mach {reftest,crashtest,jstestbrowser} for android Review of attachment 8673700 [details] [diff] [review]: ----------------------------------------------------------------- If it works for you, it works for me. Thanks for picking this up! ::: layout/tools/reftest/mach_commands.py @@ +62,5 @@ > Add 'ENABLE_MARIONETTE=1' to your mozconfig file and re-build the application. > Your currently active mozconfig is %s. > '''.lstrip() > > +here = os.path.abspath(os.path.dirname(__file__)) You only use it once. Capitalize it or better yet, inline it. @@ +283,5 @@ > + with open(path, 'r') as fh: > + imp.load_module('reftest', fh, path, ('.py', 'r', imp.PY_SOURCE)) > + import reftest > + > + # Remove the stdout handler from the internal logger and let mach deal with it nit: full sentence. This is ... odd. I leave it to your judgement. @@ +285,5 @@ > + import reftest > + > + # Remove the stdout handler from the internal logger and let mach deal with it > + runreftest.log.removeHandler(runreftest.log.handlers[0]) > + self.log_manager.enable_unstructured() try: return refteest.run(**kwargs) finally: self.log_manager.disable_unstructured() ::: layout/tools/reftest/reftestcommandline.py @@ +663,5 @@ > type=str, > dest="httpdPath", > help="path to the httpd.js file") > > + self.add_argument("--suppressDeviceInfo", I see that this works, but it's really misleading :(

(In reply to Nick Alexander :nalexander from comment #18) > ::: layout/tools/reftest/reftestcommandline.py > @@ +663,5 @@ > > type=str, > > dest="httpdPath", > > help="path to the httpd.js file") > > > > + self.add_argument("--suppressDeviceInfo", > > I see that this works, but it's really misleading :( I'm not sure I understand how it's misleading. Because the option name does not match the variable? I would have made the option --printDeviceInfo, but I want the default to be the historical norm (printDeviceInfo=True).

I went with --no-device-info, more similar to some other options. (In reply to Nick Alexander :nalexander from comment #18) > > + # Remove the stdout handler from the internal logger and let mach deal with it > > nit: full sentence. > > This is ... odd. I leave it to your judgement. I left this as it was since it was copied from the desktop case, seems to work, and, well, because it is odd and I'm not sure what it's about!

https://hg.mozilla.org/mozilla-central/rev/340c1df41b69 https://hg.mozilla.org/mozilla-central/rev/6d06a979ab59