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)
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)
4.45 KB,
patch
|
ato
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Updated•10 years ago
|
Assignee: nobody → ato
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8372298 -
Flags: review?(mdas)
Comment 2•10 years ago
|
||
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-
Assignee | ||
Comment 3•10 years ago
|
||
(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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
Right you are. Updated documentation. Carrying on r+ from mdas.
Attachment #8378310 -
Attachment is obsolete: true
Attachment #8378949 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 6•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b1c8bd01d3d
Flags: in-testsuite+
Keywords: checkin-needed
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4b1c8bd01d3d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Updated•10 years ago
|
status-b2g-v1.3:
--- → fixed
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•