1147307 - Unable to run mochitest-remote on B2G emulator-x86

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Shian-Yow Wu [:swu]]

It shows the following error message when running mochitest on B2G emulator-x86:
====================================================================================
The mochitest-remote command requires an engineering build. It may be the case that
VARIANT=user or PRODUCTION=1 were set. Try re-building with VARIANT=eng:

    $ VARIANT=eng ./build.sh

There should be an app called 'test-container.gaiamobile.org' located in
/home/sywu/work/B2G-emulator-x86-kk/out/target/product/generic/data/local/webapps.
====================================================================================

This looks like the path issue, the TARGET_OUT_DATA retrieved in 
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/mach_commands.py#1037
is "out/target/product/generic/data", while the actual path in the system is "out/target/product/generic_x86/data".

Comment 1

[Shian-Yow Wu [:swu]]

Attachment: Patch: Fix mochitest-remote failure on B2G emulator-x86.

@ahal, do you think it makes sense to simply replace the path with correct one for emulator-x86?

Comment 2

[Andrew Halberstadt [:ahal]]

Comment on attachment 8585401 [details] [diff] [review]
Patch: Fix mochitest-remote failure on B2G emulator-x86.

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

Yeah, this looks reasonable. But I'd rather handle this in the mach b2g bootscript here:
https://github.com/mozilla-b2g/B2G/blob/master/tools/mach_b2g_bootstrap.py#L262

This way it will be fixed for all mach commands that might need this, not just mochitest.

Comment 3

[Shian-Yow Wu [:swu]]

As I checked, there are at least 3 places that need the target out path:
https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/mach_commands.py#1036
https://dxr.mozilla.org/mozilla-central/source/testing/xpcshell/mach_commands.py#325
https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozrunner/mozrunner/devices/emulator.py#30

To handle it in b2g bootscript, I think we can do this way.

1. Resolve target output path in mach_b2g_bootstrap.py and pass it to each mach commands, like below:

diff --git a/tools/mach_b2g_bootstrap.py b/tools/mach_b2g_bootstrap.py
index 154eed3..247eb37 100644
--- a/tools/mach_b2g_bootstrap.py
+++ b/tools/mach_b2g_bootstrap.py
@@ -260,6 +260,13 @@ def bootstrap(b2g_home):
         context.xre_path = xre_path
         # device name is set from load configuration step above
         context.device_name = os.environ.get('DEVICE_NAME', '').rstrip()
+        if context.device_name.startswith('emulator-x86'):
+            target_device = 'generic_x86'
+        else:
+            target_device = 'generic'
+        context.target_out = os.path.join(
+            get_build_var('TARGET_PRODUCT_OUT_ROOT'),
+            target_device)
         context.get_build_var = get_build_var
 
     mach = mach.main.Mach(b2g_home)

2. Then, use it to make the final path in each mach command, for example:

    host_webapps_dir = os.path.join(self.target_out, 'data', 'local', 'webapps')

Does it make sense?

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

(In reply to Shian-Yow Wu [:swu] from comment #3)
>          context.device_name = os.environ.get('DEVICE_NAME', '').rstrip()
> +        if context.device_name.startswith('emulator-x86'):
> +            target_device = 'generic_x86'
> +        else:
> +            target_device = 'generic'

Is there a reason for doing this ad-hoc processing on DEVICE_NAME instead of just using DEVICE?

Comment 5

[Andrew Halberstadt [:ahal]]

Yeah, I think your approach makes sense. But I agree with Jed, it looks like you can just use DEVICE instead of hardcoding 'generic/generic_x86'. But otherwise I think adding either/both of device and target_out to the mach context is perfectly fine.

Comment 6

[Shian-Yow Wu [:swu]]

Thanks for the info about DEVICE.

Comment 7

[Shian-Yow Wu [:swu]]

Attachment: Patch: Add device and target_out to the mach context.

Comment 8

[Shian-Yow Wu [:swu]]

Attachment: Patch: Use the target_out value for b2g mach mochitest.

The patch uses the target_out value for b2g mach mochitest.

For xpcshell and marionette, I am not sure we should change them as well.  If needed, we can file different bugs for them.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 8586629 [details] [review]
Patch: Add device and target_out to the mach context.

Thanks, this looks good to me! I'm the right person to review that file, but I'm not a peer of this module. Just letting you know I'm not familiar with the process of landing there.

Comment 10

[Andrew Halberstadt [:ahal]]

Comment on attachment 8586639 [details] [diff] [review]
Patch: Use the target_out value for b2g mach mochitest.

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

Thanks, this looks good. But please add the if statement back otherwise it will cause an error for people who haven't updated their B2G repo yet.

::: testing/mochitest/mach_commands.py
@@ +1019,5 @@
>  
>      def __init__(self, context):
>          MachCommandBase.__init__(self, context)
>  
> +        for attr in ('b2g_home', 'xre_path', 'device_name', 'target_out'):

Could you add a link to where these are defined in the B2G repo as a comment?

@@ +1031,5 @@
>              conditions.is_b2g,
>              is_emulator])
>      @B2GCommand
>      def run_mochitest_remote(self, test_paths, **kwargs):
> +        host_webapps_dir = os.path.join(

I think you need to keep the 'if self.target_out' here. Otherwise this will fail if developers try to run this script on an outdated B2G repo.

Comment 11

[Shian-Yow Wu [:swu]]

(In reply to Andrew Halberstadt [:ahal] from comment #10)
> Comment on attachment 8586639 [details] [diff] [review]
> Patch: Use the target_out value for b2g mach mochitest.
> 
> Review of attachment 8586639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, this looks good. But please add the if statement back otherwise it
> will cause an error for people who haven't updated their B2G repo yet.
> 
> ::: testing/mochitest/mach_commands.py
> @@ +1019,5 @@
> >  
> >      def __init__(self, context):
> >          MachCommandBase.__init__(self, context)
> >  
> > +        for attr in ('b2g_home', 'xre_path', 'device_name', 'target_out'):
> 
> Could you add a link to where these are defined in the B2G repo as a comment?

OK.

> 
> @@ +1031,5 @@
> >              conditions.is_b2g,
> >              is_emulator])
> >      @B2GCommand
> >      def run_mochitest_remote(self, test_paths, **kwargs):
> > +        host_webapps_dir = os.path.join(
> 
> I think you need to keep the 'if self.target_out' here. Otherwise this will
> fail if developers try to run this script on an outdated B2G repo.

OK, thanks for reminding this point.

Comment 12

[Shian-Yow Wu [:swu]]

Attachment: Patch(v2): Use the target_out value for b2g mach mochitest.

Please review again, thank you!

Comment 13

[Shian-Yow Wu [:swu]]

(In reply to Andrew Halberstadt [:ahal] from comment #9)
> Comment on attachment 8586629 [details] [review]
> Patch: Add device and target_out to the mach context.
> 
> Thanks, this looks good to me! I'm the right person to review that file, but
> I'm not a peer of this module. Just letting you know I'm not familiar with
> the process of landing there.

Thanks for the review, merged.

https://github.com/mozilla-b2g/B2G/commit/067a725a499bbd631609147bcb86445c21d956d6

Comment 14

[Andrew Halberstadt [:ahal]]

Comment on attachment 8587134 [details] [diff] [review]
Patch(v2): Use the target_out value for b2g mach mochitest.

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

Thanks!

Comment 15

[Shian-Yow Wu [:swu]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/22567bbbd851

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/22567bbbd851