Closed Bug 996443 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Emulator, defect)

x86
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog)

RESOLVED FIXED
2.0 S1 (9may)
tracking-b2g backlog

People

(Reporter: vicamo, Assigned: vicamo)

References

Details

(Whiteboard: [ft:ril][p=1])

Attachments

(2 files, 3 obsolete files)

Attached file 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.
I can't test on ICS emulator-x86 because b2g crashes and I don't have a x86 gdbserver to investigate the problem.
Blocks: 996449
Blocks: 753928
Mochitest runs.  Test command: ./mach mochitest-remote --emulator x86 --this-chunk 1 --total-chunks 9
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
Assignee: nobody → vyang
Attachment #8406847 - Flags: review?(jgriffin)
OS: Linux → Gonk (Firefox OS)
Hardware: x86_64 → x86
Whiteboard: [ft:ril][p=1]
Target Milestone: --- → 1.4 S6 (25apr)
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.
Attachment #8406847 - Flags: review?(jgriffin) → review?(ahalberstadt)
blocking-b2g: --- → backlog
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.
Attachment #8406847 - Flags: review?(ahalberstadt) → review-
(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.
Attached patch patch : v2 (obsolete) — Splinter Review
Remove --emulator from mach commands.
Attachment #8406847 - Attachment is obsolete: true
Attachment #8407566 - Flags: review?(ahalberstadt)
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 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.
Attachment #8407566 - Flags: review?(ahalberstadt) → review-
Attached patch patch : v4 (obsolete) — Splinter Review
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
Attachment #8407566 - Attachment is obsolete: true
Attachment #8407976 - Flags: review?(ahalberstadt)
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').
Attachment #8407976 - Flags: review?(ahalberstadt) → review+
(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
Flags: needinfo?(ahalberstadt)
(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?
Flags: needinfo?(ahalberstadt)
(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.
Attached patch patch : v5Splinter Review
address comment 11.
Attachment #8407976 - Attachment is obsolete: true
Attachment #8415030 - Flags: review+
Target Milestone: 1.4 S6 (25apr) → 2.0 S1 (9may)
https://hg.mozilla.org/mozilla-central/rev/c1e8e5bee5ea
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
blocking-b2g: backlog → ---
Duplicate of this bug: 996473
You need to log in before you can comment on or make changes to this bug.