1214152 - Test runner: print suitable error if handed a non-executable

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Lars T Hansen [:lth]]

Currently, if the required argument to jit_test.py and jstests.py is not an executable, the test harness will try to execute it once for each test, and print one stack trace for each test.  This is usually extremely confusing.  A better solution would be to check that the program is executable, and to fail with an error message if it is not.

Comment 1

[Lars T Hansen [:lth]]

Attachment: bug1214152-valid-shell.patch

Comment 2

[Terrence Cole [:terrence]]

Comment on attachment 8673085 [details] [diff] [review]
bug1214152-valid-shell.patch

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

::: js/src/tests/jstests.py
@@ +159,5 @@
>      # If we do not have a shell, we must be in a special mode.
>      if options.js_shell is None and not options.make_manifests:
>          op.error('missing JS_SHELL argument')
>  
> +    if not (options.js_shell is None):

"is not" is an operator in its own right, so it would be clearer to do:
if options.js_shell is not None:

@@ +160,5 @@
>      if options.js_shell is None and not options.make_manifests:
>          op.error('missing JS_SHELL argument')
>  
> +    if not (options.js_shell is None):
> +        if not (os.path.isfile(options.js_shell) and os.access(options.js_shell, os.X_OK)):

We already have the first two parts of this test right below the parse_args() call in main(). Can you just add the os.access test there to get the same effect?

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1214152-valid-shell.patch, v2

Changed jstests.py as suggested

Comment 4

[Terrence Cole [:terrence]]

Comment on attachment 8673631 [details] [diff] [review]
bug1214152-valid-shell.patch, v2

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

Thanks!

::: js/src/tests/jstests.py
@@ +307,5 @@
>  
>  
>  def main():
>      options, prefix, requested_paths, excluded_paths = parse_args()
> +    if options.js_shell is not None and not (isfile(options.js_shell) and os.access(options.js_shell, os.X_OK)):

Please wrap this after the second and to keep things to approximately 80 chars -- people occasionally sweep the tree with a pep8 linter and complain about violations.

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ce6e4af854a

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

this appear to have busted Windows jit tests:

https://treeherder.mozilla.org/logviewer.html#?job_id=15649235&repo=mozilla-inbound
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/613fcc68a20f

Comment 7

[Lars T Hansen [:lth]]

This will go unfixed until I can get my Windows build environment to work, which at the current rate of progress will be never.

Comment 8

[Lars T Hansen [:lth]]

The problem is that on Windows, the shell will frequently be eg dist/bin/js but the name of the file is actually dist/bin/js.exe.

Comment 9

[Lars T Hansen [:lth]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1649d7c7156b

Comment 10

[Lars T Hansen [:lth]]

Attachment: bug1214152-valid-shell.patch, v3

Comment 11

[Lars T Hansen [:lth]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6b34afb1a471b35f9c24945eb2ae094b77952526
Bug 1214152 - early check that shell is executable.  r=terrence

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6b34afb1a471