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

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Andreas Tolfsen ❲:ato❳]

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"))

Comment 1

[Andreas Tolfsen ❲:ato❳]

try: https://tbpl.mozilla.org/?tree=Try&rev=2249e0c00b5a

Comment 2

[Andreas Tolfsen ❲:ato❳]

Attachment: 0001-Bug-958024-Wrap-last-exception-from-Marionette-s-Wai.patch

Comment 3

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

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.

Comment 4

[Malini Das [:mdas] - Away, not checking bugmail]

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.

Comment 5

[Andreas Tolfsen ❲:ato❳]

Attachment: 0001-Bug-958024-Wrap-last-exception-from-Marionette-s-Wai.patch

Well spotted, thanks!  I've removed the extra raise and carried over the r+.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c5f276268e8

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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