Closed Bug 969387 Opened 10 years ago Closed 10 years ago

Add guards and tests for wrong arguments to MarionetteException

Categories

(Remote Protocol :: Marionette, defect)

x86_64
Linux
defect
Not set
normal

Tracking

(b2g-v1.3 fixed)

RESOLVED FIXED
mozilla30
Tracking Status
b2g-v1.3 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

Attachments

(1 file, 2 obsolete files)

As exemplified by integration investigation conducted by the sheriffs in bug 940554, passing the wrong arguments to the MarionetteException class, in particular the cause keyword, can have fatal consequences.

To avoid this wrong happening we should add type guards to it's __str__ method in particular as exceptions in an exception's string representation can be hard to detect.

Tests would also be good.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment on attachment 8372298 [details] [diff] [review]
0001-Bug-969387-Guard-against-wrong-arguments-to-Marionet.patch

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

r-, since I'd like to either force a user to use the right parameters, or give as much visibility as possible to the source of error.

::: testing/marionette/client/marionette/errors.py
@@ +66,2 @@
>              msg += ", caused by %r" % self.cause[0]
>              tb = self.cause[2]

If someone passes a cause that isn't a tuple, we may still want to display it. If we don't, this would lead to tests failing for unclear reasons when we have the opportunity to present more information. 

If we want to strongly type it to only accept tuples, then we should raise an error if cause is passed but of an unexpected type... but I'd actually prefer that we accept any type, but force it to a string if it isn't a tuple. Ie:

if self.cause:
    if type(self.cause) is tuple:
        # do the usual logic
    else:
        msg += ", caused by %s" % self.cause

::: testing/marionette/client/marionette/tests/unit/test_errors.py
@@ +52,5 @@
> +        self.assertIn("\nstacktrace:\n\tfirst\n\tsecond\n", r)
> +        self.assertIn("MarionetteException:", r)
> +
> +    def test_invalid_cause_exception(self):
> +        exc = errors.MarionetteException(cause=invalid_cause)

instead of creating the invalid_cause function to return an exception object, you can just do

invalid_cause = ValueError("invalid")
exc = errors.MarionetteException(cause=invalid_cause)
Attachment #8372298 - Flags: review?(mdas) → review-
(In reply to Malini Das [:mdas] from comment #2)
> ::: testing/marionette/client/marionette/errors.py
> @@ +66,2 @@
> >              msg += ", caused by %r" % self.cause[0]
> >              tb = self.cause[2]
> 
> If someone passes a cause that isn't a tuple, we may still want to display
> it. If we don't, this would lead to tests failing for unclear reasons when
> we have the opportunity to present more information. 

I agree completely and I've submitted a revised patch to address this like you suggested.
Attachment #8372298 - Attachment is obsolete: true
Attachment #8378310 - Flags: review?(mdas)
Comment on attachment 8378310 [details] [diff] [review]
0001-Bug-969387-Guard-against-wrong-arguments-to-Marionet.patch

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

thanks!

::: testing/marionette/client/marionette/errors.py
@@ +49,5 @@
> +
> +        :param cause: An optional tuple of three values giving
> +            information about the root exception cause.  Expected
> +            tuple values are (type, value, traceback).
> +

shouldn't we also describe the 'stacktrace' parameter?
Attachment #8378310 - Flags: review?(mdas) → review+
Right you are.  Updated documentation.  Carrying on r+ from mdas.
Attachment #8378310 - Attachment is obsolete: true
Attachment #8378949 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4b1c8bd01d3d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: