Closed
Bug 1126057
Opened 9 years ago
Closed 9 years ago
Provide better error message when ./mach robocop is run w/no MOZ_HOST_BIN
Categories
(Firefox for Android Graveyard :: Testing, defect)
Tracking
(firefox39 fixed)
RESOLVED
FIXED
Firefox 39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: capella, Assigned: gioyik, Mentored)
References
(Blocks 1 open bug)
Details
(Whiteboard: [lang=python][good first bug])
Attachments
(1 file, 1 obsolete file)
2.05 KB,
patch
|
nalexander
:
review+
|
Details | Diff | Splinter Review |
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 :-)
Reporter | ||
Comment 1•9 years ago
|
||
And maybe update the wiki page? Not sure how you want that worded... I still execute manually ( the "deprecated" way :) )
Comment 2•9 years ago
|
||
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
Assignee | ||
Comment 3•9 years ago
|
||
Hi, I want to work on this issue. Could you tell me on which file I need to make the changes? Thanks
Flags: needinfo?(markcapella)
Reporter | ||
Comment 4•9 years ago
|
||
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
Flags: needinfo?(markcapella)
Assignee | ||
Comment 6•9 years ago
|
||
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.
Flags: needinfo?(markcapella)
Reporter | ||
Comment 7•9 years ago
|
||
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.
Flags: needinfo?(markcapella) → needinfo?(nalexander)
Comment 8•9 years ago
|
||
(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!
Flags: needinfo?(nalexander)
Assignee | ||
Comment 9•9 years ago
|
||
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
Assignee | ||
Updated•9 years ago
|
Flags: needinfo?(nalexander)
Comment 10•9 years ago
|
||
(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.
Flags: needinfo?(nalexander)
Assignee | ||
Comment 11•9 years ago
|
||
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.
Flags: needinfo?(nalexander)
Assignee | ||
Comment 12•9 years ago
|
||
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•9 years ago
|
||
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?
Flags: needinfo?(nalexander)
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8567727 -
Flags: review?(nalexander)
Assignee | ||
Comment 15•9 years ago
|
||
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•9 years ago
|
||
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!
Attachment #8567727 -
Flags: review?(nalexander) → feedback+
Comment 17•9 years ago
|
||
(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.)
Assignee | ||
Comment 18•9 years ago
|
||
Patch v2
Attachment #8567727 -
Attachment is obsolete: true
Attachment #8570012 -
Flags: review?(nalexander)
Assignee | ||
Comment 19•9 years ago
|
||
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 21•9 years ago
|
||
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!
Attachment #8570012 -
Flags: review?(nalexander) → review+
Updated•9 years ago
|
Assignee: nobody → gioyik
Status: NEW → ASSIGNED
Assignee | ||
Comment 22•9 years ago
|
||
Sounds good, Thank you!
Comment 23•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fa4247a2db1b
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 39
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•