Closed Bug 958024 Opened 6 years ago Closed 6 years ago

Last exception in Marionette's Wait should be wrapped in TimeoutException

Categories

(Testing :: Marionette, defect)

x86
macOS
defect
Not set

Tracking

(b2g-v1.3 fixed)

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

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(1 file, 1 obsolete file)

The last raised exception (last_exc) from the condition passed to the marionette.wait.Wait class should be a TimeoutException containing the original exception's stack trace.

The current behaviour is confusing in a scenario like the following, where an exception is explicitly ignored yet raises if the p tag isn't found.

    el = Wait(marionette, ignored_exceptions=errors.NoSuchElementException).until(lambda m: m.find_element(By.TAG_NAME, "p"))
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment on attachment 8362479 [details] [diff] [review]
0001-Bug-958024-Wrap-last-exception-from-Marionette-s-Wai.patch

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

Looks good to me, but I'd be more comfortable with mdas given an additional review.
Attachment #8362479 - Flags: review?(mdas)
Attachment #8362479 - Flags: review?(dave.hunt)
Attachment #8362479 - Flags: review+
Keywords: checkin-needed
Keywords: checkin-needed
Comment on attachment 8362479 [details] [diff] [review]
0001-Bug-958024-Wrap-last-exception-from-Marionette-s-Wai.patch

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

::: testing/marionette/client/marionette/errors.py
@@ +52,3 @@
>          if self.stacktrace:
> +            stack = "".join(["\t%s\n" % x for x in self.stacktrace.splitlines()])
> +            msg += "\nstacktrace:\n%s" % stack

thanks, this is nicer.

::: testing/marionette/client/marionette/tests/unit/test_errors.py
@@ +13,5 @@
> +    try:
> +        raise ValueError("bar")
> +    except ValueError as e:
> +        return sys.exc_info()
> +    raise

is this 'raise' needed? Try/except blocks should implicitly raise any exception that isn't ValueError.
Attachment #8362479 - Flags: review?(mdas) → review+
Well spotted, thanks!  I've removed the extra raise and carried over the r+.
Attachment #8362479 - Attachment is obsolete: true
Attachment #8363707 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3c5f276268e8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Duplicate of this bug: 970673
You need to log in before you can comment on or make changes to this bug.