996443 - B2G Emulator-x86: "./mach marionette-remote" doesn't carry "--emulator=x86" automatically

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Emulator, defect)

Description

[Vicamo Yang [:vicamo][:vyang]]

Attachment: error-marionette.txt

While trying to run Marionette test cases with emulator-x86-kk, it fails with:

  File not found: /home/vicamo/WorkSpace/mozilla/b2g/qemu/kk/x86/master/1/out/target/product/generic

in testing/marionette/client/marionette/b2gbuild.py.  While appending --emulator=x86 to the same command, Marionette runs.

Comment 1

[Vicamo Yang [:vicamo][:vyang]]

I can't test on ICS emulator-x86 because b2g crashes and I don't have a x86 gdbserver to investigate the problem.

Comment 2

[Vicamo Yang [:vicamo][:vyang]]

Mochitest runs.  Test command: ./mach mochitest-remote --emulator x86 --this-chunk 1 --total-chunks 9

Comment 3

[Vicamo Yang [:vicamo][:vyang]]

Attachment: bug-996443-carry-emulator-arch-automatically.patch

This patch calculates correct emulator arch from its device_name.  In theory, the --emulator argument can be removed, but I'm not sure whether it's referenced in other automation tools or not.  Besides, I'd prefer to rename it to device_arch instead so that we can still have correct device arch when running on physical devices or emulator variants that doesn't have a "emulator" prefix in its name.

This patch also fixes bug 996473 -- running xpcshell on emulator-x86.

Try: https://tbpl.mozilla.org/?tree=Try&rev=b87580f385a7

Comment 4

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8406847 [details] [diff] [review]
bug-996443-carry-emulator-arch-automatically.patch

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

Passing this review to ahal.

Comment 5

[Andrew Halberstadt [:ahal]]

Comment on attachment 8406847 [details] [diff] [review]
bug-996443-carry-emulator-arch-automatically.patch

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

So this isn't your fault, but I would like to just remove the --emulator option completely. We know the device_name already, so there's no reason to make the user specify it explicitly. If you aren't comfortable making this change, just let me know and I'll come up with a patch.

Comment 6

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Andrew Halberstadt [:ahal] from comment #5)
> Review of attachment 8406847 [details] [diff] [review]:
> -----------------------------------------------------------------
> So this isn't your fault, but I would like to just remove the --emulator
> option completely. We know the device_name already, so there's no reason to
> make the user specify it explicitly. If you aren't comfortable making this
> change, just let me know and I'll come up with a patch.

By "completely", do you mean removing --emulator from "run*testb2g.py" as well?  Mach commands always come with device_name passed, but underlying "run*testb2g.py" do not.  "run*testb2g.py" use --emulator instead of device_name, so removing --emulator implies adding --device-name, and that will probably break some automation tools.  That's a long existing problem and also the root cause of this bug -- general developers use a different command interfaces from the ones used in tbpl.

Comment 7

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch : v2

Remove --emulator from mach commands.

Comment 8

[Andrew Halberstadt [:ahal]]

No sorry, I just meant remove it from the mach target.

And yes, I agree that different interfaces is a problem. I have some ideas of how to fix the problem, but unfortunately it is a lot of work and probably won't appear on my goals list for quite some time. Eventually I think the logic for stuff like detecting the device should live in the harness, but for now removing --emulator from run*testsb2g.py is well out of scope of this bug :).

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8407566 [details] [diff] [review]
patch : v2

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

Thanks, this looks good! I have to r- because it will break the marionette-webapi command on non-emulator devices.

::: layout/tools/reftest/mach_commands.py
@@ +417,5 @@
>  
>      def _run_reftest(self, test_file=None, suite=None, **kwargs):
> +        if self.device_name:
> +            emulator = 'arm'
> +            if self.device_name.find('emulator') == 0:

nit: self.device_name.startswith('emulator') (applies to other instances in this patch as well)

@@ +418,5 @@
>      def _run_reftest(self, test_file=None, suite=None, **kwargs):
> +        if self.device_name:
> +            emulator = 'arm'
> +            if self.device_name.find('emulator') == 0:
> +                if self.device_name.find('x86') > 0:

nit: 'x86' in self.device_name (applies to other instances in this patch as well)

::: testing/marionette/mach_commands.py
@@ +74,5 @@
>          default='b2g')
>      @CommandArgument('tests', nargs='*', metavar='TESTS',
>          help='Path to test(s) to run.')
> +    def run_marionette_webapi(self, tests, testtype=None):
> +        emulator = 'arm'

It looks like this command might be intended to work with other devices as well. Setting emulator here will break that, please move it below the 'emulator' if statment.

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch : v4

Address comment 9, also replace other |device_name.find(...)| with |device_name.startwith(...)|.

Try all for it may affect other platforms: https://tbpl.mozilla.org/?tree=Try&rev=9a7323c22071

Comment 11

[Andrew Halberstadt [:ahal]]

Comment on attachment 8407976 [details] [diff] [review]
patch : v4

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

Thanks! Please fix the lone comment, r+ with that addressed.

Also for future reference, the mach commands aren't used by automation, so pushing to try doesn't test them :(

::: testing/marionette/mach_commands.py
@@ +77,5 @@
> +    def run_marionette_webapi(self, tests, testtype=None):
> +        emulator = None
> +        if self.device_name:
> +            emulator = 'arm'
> +            if self.device_name.startswith('emulator'):

This still isn't going to work, i.e device_name could be 'tarako' and we'd be passing in --emulator arm, it needs to be below the startswith('emulator').

Comment 12

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Andrew Halberstadt [:ahal] from comment #11)
> Review of attachment 8407976 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks! Please fix the lone comment, r+ with that addressed.
> 
> Also for future reference, the mach commands aren't used by automation, so
> pushing to try doesn't test them :(
> 
> ::: testing/marionette/mach_commands.py
> @@ +77,5 @@
> > +    def run_marionette_webapi(self, tests, testtype=None):
> > +        emulator = None
> > +        if self.device_name:
> > +            emulator = 'arm'
> > +            if self.device_name.startswith('emulator'):
> 
> This still isn't going to work, i.e device_name could be 'tarako' and we'd
> be passing in --emulator arm, it needs to be below the
> startswith('emulator').

When you passed --emulator to Marionette, Marionette instantiate an Emulator instance and runs test cases on it. [1]  So how can we assign emulator while the device_name is not represents an emulator?  When running Marionette test cases on physical devices, you should not give --emulator anyway.

Actually, there is no way you can run marionette test cases on physical devices via `./mach marionette-webapi` -- there is just no enough arguments allowing you to do so.  For example, you just can't pass --address and --port because they're not exposed in mach interface.  So, No, --emulator is an emulator only parameter.

Besides, how do you know 'tarako' is an ARM-based device?  That's why I said I'd prefer keeping --emulator argument but renaming it to --device-arch in comment 3.

NI for "This still isn't going to work".  That's not correct.

[1]: http://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/marionette.py#493

Comment 13

[Andrew Halberstadt [:ahal]]

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #12)
> > ::: testing/marionette/mach_commands.py
> > @@ +77,5 @@
> > > +    def run_marionette_webapi(self, tests, testtype=None):
> > > +        emulator = None
> > > +        if self.device_name:
> > > +            emulator = 'arm'
> > > +            if self.device_name.startswith('emulator'):
> > 
> > This still isn't going to work, i.e device_name could be 'tarako' and we'd
> > be passing in --emulator arm, it needs to be below the
> > startswith('emulator').
> 
> When you passed --emulator to Marionette, Marionette instantiate an Emulator
> instance and runs test cases on it. [1]  So how can we assign emulator while
> the device_name is not represents an emulator?  When running Marionette test
> cases on physical devices, you should not give --emulator anyway.

This is absolutely correct, but the line "emulator = 'arm'" *is* doing the equivalent of passing --emulator into the test harness. So this code is essentially passing in --emulator even if self.device_name == 'tarako'.

Your point that the mach target isn't really setup for devices anyway because it doesn't support --address and --port is valid, but I think this should be fixed anyway.

Does that make sense?

Comment 14

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Andrew Halberstadt [:ahal] (PTO until May 6th) from comment #13)
> Does that make sense?

Yes.  When I come back to this issue after several days, I suddenly understand what you was saying.  Thanks for your patience.  I'm having another revision to address that.

Comment 15

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch : v5

address comment 11.

Comment 16

[Vicamo Yang [:vicamo][:vyang]]

try all yet again: https://tbpl.mozilla.org/?tree=Try&rev=a3ff4a653ab3

Comment 17

[Vicamo Yang [:vicamo][:vyang]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c1e8e5bee5ea

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1e8e5bee5ea