1126057 - Provide better error message when ./mach robocop is run w/no MOZ_HOST_BIN

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[Mark Capella [:capella]]

If a new dev tries to fire off a robocop test using |./mach robocop| or |./mach robocop testFoo| without first exporting their MOZ_HOST_BIN value, they receive this somewhat cryptic message:

TypeError: coercing to Unicode: need string or buffer, NoneType found

This might could be friendlier :-)

Comment 1

[Mark Capella [:capella]]

And maybe update the wiki page? Not sure how you want that worded... I still execute manually ( the "deprecated" way :) )

Comment 2

[Geoff Brown [:gbrown]]

The old make targets do a good job of checking MOZ_HOST_BIN. Something very similar might be good for mach:

http://hg.mozilla.org/mozilla-central/annotate/95c76c3b0172/testing/testsuite-targets.mk#l90

Comment 3

[Giovanny Gongora [:gioyik]]

Hi,

I want to work on this issue. Could you tell me on which file I need to make the changes?

Thanks

Comment 4

[Mark Capella [:capella]]

Guess I should have posted the rest of that:

  File "/home/master/mozilla-central/testing/mochitest/mach_commands.py", line 907, in run_robocop
    '--xre-path=' + os.environ.get('MOZ_HOST_BIN'),

So basically it fails here:

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/mach_commands.py?rev=f397b3559467&mark=907-907#887

Comment 6

[Giovanny Gongora [:gioyik]]

Could this be a solution:

  if MOZ_HOST_BIN == '':
      print('environment variable MOZ_HOST_BIN must be set')

I ask before attach a possible patch. Let me know if I am correct.

Comment 7

[Mark Capella [:capella]]

Hi Giovanny!

   While I filed this bug, it's being mentored by nalexander. I know he's got some additional background plans in the works here, so I'll let him respond from now on.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Capella [:capella] from comment #7)
> Hi Giovanny!
> 
>    While I filed this bug, it's being mentored by nalexander. I know he's
> got some additional background plans in the works here, so I'll let him
> respond from now on.

Giovanny: yes, something like this.  I think we should implement exactly the same checks as https://bugzilla.mozilla.org/show_bug.cgi?id=1126057#c2 does, but in Python at the relevant point.  Flag me for feedback/review on a patch.  Thanks!

Comment 9

[Giovanny Gongora [:gioyik]]

I am working on this but I have questions about the use of "! -d" and "! -f". Could you tell me how can be ported to python?

This is the work I had done before create a patch for review:

if MOZ_HOST_BIN == '':
    print('environment variable MOZ_HOST_BIN must be set to a directory containing host xpcshell')
elif ! MOZ_HOST_BIN:
    print('MOZ_HOST_BIN does not specify a directory')
elif ! MOZ_HOST_BIN/xpcshell:
    print('xpcshell not found in MOZ_HOST_BIN')
elif TEST_DEVICE == '' & DM_TRANS != 'adb':
    print('please prepare your host with the environment variable TEST_DEVICE')
else:
    RUN_MOCHITEST_REMOTE

Comment 10

[Nick Alexander :nalexander [he/him]]

(In reply to Giovanny Gongora [:gioyik] from comment #9)
> I am working on this but I have questions about the use of "! -d" and "!
> -f". Could you tell me how can be ported to python?
> 
> This is the work I had done before create a patch for review:

You'll need to have

MOZ_HOST_BIN = os.environ.get('MOZ_HOST_BIN')

> if MOZ_HOST_BIN == '':
>     print('environment variable MOZ_HOST_BIN must be set to a directory
> containing host xpcshell')

if not MOZ_HOST_BIN:

> elif ! MOZ_HOST_BIN:
>     print('MOZ_HOST_BIN does not specify a directory')

Use os.path.isdir: http://devdocs.io/python2/library/os.path#os.path.isdir

> elif ! MOZ_HOST_BIN/xpcshell:
>     print('xpcshell not found in MOZ_HOST_BIN')

Use os.path.exists: http://devdocs.io/python2/library/os.path#os.path.exists and maybe use the helper function at

https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozpack/executables.py#57

to test for executable.

> elif TEST_DEVICE == '' & DM_TRANS != 'adb':
>     print('please prepare your host with the environment variable
> TEST_DEVICE')
> else:
>     RUN_MOCHITEST_REMOTE

This doesn't make sense here.

Comment 11

[Giovanny Gongora [:gioyik]]

Thanks for the quickly feedback. I made changes and this is a second possible solution:

MOZ_HOST_BIN = os.environment.get('MOZ_HOST_BIN')
RUN_MOCHITEST_REMOTE = os.environment.get('RUN_MOCHITEST_REMOTE')

if not MOZ_HOST_BIN:
	print('environment variable MOZ_HOST_BIN must be set to a directory containing host xpcshell')
elif os.path.isdir(MOZ_HOST_BIN):
    print('MOZ_HOST_BIN does not specify a directory')
elif is_executable(MOZ_HOST_BIN + '/xpcshell'):
    print('xpcshell not found in MOZ_HOST_BIN')
elif os.environment.get('TEST_DEVICE') == '' && os.environment.get('DM_TRANS') not 'adb':
	print('please prepare your host with the environment variable TEST_DEVICE')
else:
	open(RUN_MOCHITEST_REMOTE)

I have a question with the "-a" option in the last part. I think is an "AND" operator but I could be wrong.

Comment 12

[Giovanny Gongora [:gioyik]]

Sorry I made a change in the first lines:

MOZ_HOST_BIN = os.environment.get('MOZ_HOST_BIN')
RUN_MOCHITEST_REMOTE = os.environment.get('RUN_MOCHITEST_REMOTE')

if MOZ_HOST_BIN == '':
	print('environment variable MOZ_HOST_BIN must be set to a directory containing host xpcshell')
elif not os.path.isdir(MOZ_HOST_BIN):
    print('MOZ_HOST_BIN does not specify a directory')
elif is_executable(MOZ_HOST_BIN + '/xpcshell'):
    print('xpcshell not found in MOZ_HOST_BIN')
elif os.environment.get('TEST_DEVICE') == '' && os.environment.get('DM_TRANS') not 'adb':
	print('please prepare your host with the environment variable TEST_DEVICE')
else:
	open(RUN_MOCHITEST_REMOTE)

Comment 13

[Nick Alexander :nalexander [he/him]]

Sorry for the delayed reply.

Throughout, prefer os.path.join to +.

(In reply to Giovanny Gongora [:gioyik] from comment #12)
> Sorry I made a change in the first lines:
> 
> MOZ_HOST_BIN = os.environment.get('MOZ_HOST_BIN')
> RUN_MOCHITEST_REMOTE = os.environment.get('RUN_MOCHITEST_REMOTE')

You don't need RUN_MOCHITEST_REMOTE.

> if MOZ_HOST_BIN == '':

if not MOZ_HOST_BIN:

> 	print('environment variable MOZ_HOST_BIN must be set to a directory
> containing host xpcshell')
> elif not os.path.isdir(MOZ_HOST_BIN):
>     print('MOZ_HOST_BIN does not specify a directory')

add

elif not os.path.isfile(os.path.join(MOZ_HOST_BIN, 'xpcshell')):
    print('xpcshell not found in MOZ_HOST_BIN')

> elif is_executable(MOZ_HOST_BIN + '/xpcshell'):

elif not is_executable(os.path.join(MOZ_HOST_BIN, 'xpcshell')):

We want to be able to run this!

>     print('xpcshell not found in MOZ_HOST_BIN')

print('xpcshell in MOZ_HOST_BIN is not executable')

> elif os.environment.get('TEST_DEVICE') == '' &&
> os.environment.get('DM_TRANS') not 'adb':
> 	print('please prepare your host with the environment variable TEST_DEVICE')
> else:
> 	open(RUN_MOCHITEST_REMOTE)

Get rid of this stuff.  Once you have the tests above, we're ready to go!  Can I see a patch?  Where are you importing is_executable from?

Comment 14

[Giovanny Gongora [:gioyik]]

Attachment: 1126057.patch

Comment 15

[Giovanny Gongora [:gioyik]]

Sorry for the late answer. I was organizing my schedule with my University classes. I attached a first patch, please, could you review it? Also, could you tell me the computed syntax you could use for this bug to try it on treeherder? I have commit access level 1, so I can use try server with this.

Comment 16

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8567727 [details] [diff] [review]
1126057.patch

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

Sorry for the extremely delayed feedback :/  Feel free to ping if I'm lagging.

::: testing/mochitest/mach_commands.py
@@ +1130,5 @@
> +        elif not os.path.isdir(MOZ_HOST_BIN):
> +            print('MOZ_HOST_BIN does not specify a directory')
> +        elif not os.path.isfile(os.path.join(MOZ_HOST_BIN, 'xpcshell')):
> +            print('xpcshell not found in MOZ_HOST_BIN')
> +        elif not is_executable(os.path.join(MOZ_HOST_BIN, 'xpcshell')):

I don't think this will work, since the function won't be found.  You might want

mozpack.executables.is_executable

@@ +1132,5 @@
> +        elif not os.path.isfile(os.path.join(MOZ_HOST_BIN, 'xpcshell')):
> +            print('xpcshell not found in MOZ_HOST_BIN')
> +        elif not is_executable(os.path.join(MOZ_HOST_BIN, 'xpcshell')):
> +            print('xpcshell in MOZ_HOST_BIN is not executable')
> +        elif os.environment.get('TEST_DEVICE') == '' && os.environment.get('DM_TRANS') not 'adb':

kill this clause.

@@ +1134,5 @@
> +        elif not is_executable(os.path.join(MOZ_HOST_BIN, 'xpcshell')):
> +            print('xpcshell in MOZ_HOST_BIN is not executable')
> +        elif os.environment.get('TEST_DEVICE') == '' && os.environment.get('DM_TRANS') not 'adb':
> +            print('please prepare your host with the environment variable TEST_DEVICE')
> +        else:

and this clause.  At this point, we're good to go!

Comment 17

[Nick Alexander :nalexander [he/him]]

(In reply to Giovanny Gongora [:gioyik] from comment #15)
> Sorry for the late answer. I was organizing my schedule with my University
> classes. I attached a first patch, please, could you review it? Also, could
> you tell me the computed syntax you could use for this bug to try it on
> treeherder? I have commit access level 1, so I can use try server with this.

This code won't be exercised on the try server -- you'll have to test locally.  (I'll test locally too.)

Comment 18

[Giovanny Gongora [:gioyik]]

Attachment: 1126057-v2.patch

Patch v2

Comment 19

[Giovanny Gongora [:gioyik]]

Nick don't worry about the time. :)

I attached a patch with the changes you mention. Let me know if there's something to change and fix.

Comment 20

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/fa4247a2db1b

Comment 21

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8570012 [details] [diff] [review]
1126057-v2.patch

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

I'll land a slightly modified version of this.  Thanks, Giovanny!

Comment 22

[Giovanny Gongora [:gioyik]]

Sounds good, Thank you!

Comment 23

[Carsten Book [:Tomcat]]

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