Add |mach reftest-remote| and |mach crashtest-remote| for mobile/android

RESOLVED FIXED in Firefox 44

Status

()

Core
mach
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: nalexander, Assigned: gbrown)

Tracking

unspecified
mozilla44
All
Android
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
Exactly as the summary says.  Let's make it easier to run remote reftests and crashtests locally.
(Reporter)

Comment 1

3 years ago
Created attachment 8509939 [details] [diff] [review]
Add |mach {reftest,crashtest}-remote| for mobile/android. r=gps

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!
Attachment #8509939 - Flags: review?(gps)
Why should it be a separate command? That is, why would anyone expect mach reftest to not run remote tests for android builds?
(Reporter)

Comment 3

3 years ago
(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//.
(Reporter)

Comment 4

3 years ago
(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 7

3 years ago
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.
Attachment #8509939 - Flags: review?(gps)
(Reporter)

Comment 8

3 years ago
Created attachment 8512104 [details] [diff] [review]
Add |mach {reftest,crashtest}| for mobile/android. r=gps

Changed nothing but the name: dropped -remote.
Attachment #8509939 - Attachment is obsolete: true
Attachment #8512104 - Flags: review?(gps)

Comment 9

3 years ago
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.
Attachment #8512104 - Flags: review?(gps)
(Assignee)

Comment 10

2 years ago
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?
Flags: needinfo?(nalexander)
(Reporter)

Comment 11

2 years ago
(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.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(nalexander)
Flags: needinfo?(jonalmeida942)
Yeah, I have been running some reftests locally for that bug. What exactly does "remote" mean here?
Flags: needinfo?(s.kaspari)
(Reporter)

Comment 13

2 years ago
(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.
Flags: needinfo?(s.kaspari)
(Assignee)

Comment 15

2 years ago
I'm back, so I'll take this back!
Assignee: nobody → gbrown
Flags: needinfo?(s.kaspari)
Flags: needinfo?(jonalmeida942)
(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..
(Assignee)

Comment 17

2 years ago
Created attachment 8673700 [details] [diff] [review]
add mach {reftest,crashtest,jstestbrowser} for android

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).
Attachment #8512104 - Attachment is obsolete: true
Attachment #8663183 - Attachment is obsolete: true
Attachment #8673700 - Flags: review?(nalexander)
(Reporter)

Comment 18

2 years ago
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 :(
Attachment #8673700 - Flags: review?(nalexander) → review+
(Assignee)

Comment 19

2 years ago
(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).
(Assignee)

Comment 21

2 years ago
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
(Assignee)

Updated

2 years ago
Blocks: 1196793
You need to log in before you can comment on or make changes to this bug.