961648 - Provide a helpful error message when test variables JSON file cannot be parsed

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Currently the exception thrown when reading in an invalid JSON file isn't particularly helpful. We should at least output the path to the file being read and indicate that it is not of the expected format.

To replicate:
1. Clone http://hg.mozilla.org/mozilla-central/
2. cd mozilla-central/testing/marionette/client/marionette
3. Create an invalid testvars.json file (example attached)
4. ./venv_automation.sh python --testvars=testvars.json --binary=/path/to/firefox-bin tests/unit/unit-tests.ini

Expected result:

A message indicating the path to the JSON file and that it was not properly formatted. The exception message should be retained to indicate what's wrong with the file.

Actual result:

Traceback (most recent call last):
  File "runtests.py", line 33, in <module>
    cli()
  File "runtests.py", line 28, in cli
    runner = startTestRunner(runner_class, options, tests)
  File "runtests.py", line 18, in startTestRunner
    runner = runner_class(**vars(options))
  File "runtests.py", line 13, in __init__
    BaseMarionetteTestRunner.__init__(self, **kwargs)
  File "/Users/dhunt/workspace/mozilla/mozilla-central/testing/marionette/client/marionette/runner/base.py", line 541, in __init__
    self.testvars = json.loads(f.read())
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/__init__.py", line 338, in loads
    return _default_decoder.decode(s)
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/decoder.py", line 365, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/json/decoder.py", line 381, in raw_decode
    obj, end = self.scan_once(s, idx)
ValueError: Expecting property name: line 2 column 2 (char 3)

Comment 1

[adu]

Can i pick this Bug?

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Sure, I've assigned it to you. Let me know if you need any help by leaving a comment, or dropping into #ateam on irc.mozilla.org!

Comment 3

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Attachment: Example invalid testvars.json

Comment 4

[adu]

Attachment: firstpatch.diff

Please review my first patch.

Comment 5

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8362616 [details] [diff] [review]
firstpatch.diff

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

Thanks for the quick turnaround. Just a couple of comments, but this is looking good!

::: testing/marionette/client/marionette/runner/base.py
@@ +539,5 @@
>              import json
> +            try:
> +                with open(testvars) as f:
> +                    self.testvars = json.loads(f.read())
> +            except Exception, e:

We should only be catching the specific exceptions related to an invalid JSON format. See http://docs.python.org/2/library/json.html#encoders-and-decoders "If the data being deserialized is not a valid JSON document, a ValueError will be raised."

Also, please use the following style: except E as N:

@@ +541,5 @@
> +                with open(testvars) as f:
> +                    self.testvars = json.loads(f.read())
> +            except Exception, e:
> +	        raise Exception("JSON file (%s) is not properly formatted: %s" %(os.path.abspath(testvars), e))
> +	        

Please remove this extraneous whitespace.

Comment 6

[adu]

Attachment: second_patch.diff

1) Raising Exception if ValueError caught.
2) Following new style of excep (PEP 3110).
3) Removed the whitespace from last patch.

Comment 7

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8363000 [details] [diff] [review]
second_patch.diff

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

Looks great, just a couple of nits I missed last time. With these fixed I think we can land it.

::: testing/marionette/client/marionette/runner/base.py
@@ +540,5 @@
> +            try:
> +                with open(testvars) as f:
> +                    self.testvars = json.loads(f.read())
> +            except ValueError as e:
> +                raise Exception("JSON file (%s) is not properly formatted: %s" %(os.path.abspath(testvars), e.message))

Nit: Please add a space after the % and before (os.path
Nit: Could you limit the line length to as close to 80 characters as possible.

Comment 8

[adu]

Attachment: final_patch.diff

1) Limited the line length to 80

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8363620 [details] [diff] [review]
final_patch.diff

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

Looks good and works well. Can you please generate your patch according to https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F so that I can land it. Thanks!

Comment 10

[adu]

Attachment: bug961648.patch

Patch generated according to https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Its been a pleasure working with you.
Thanks

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8365625 [details] [diff] [review]
bug961648.patch

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

Thanks! Landed on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e930acf2697

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3e930acf2697