Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WPT test harness crashes if test receives "unknown error" from WebDriver #39617

Closed
cookiecrook opened this issue Apr 20, 2023 · 6 comments · Fixed by #45833
Closed

WPT test harness crashes if test receives "unknown error" from WebDriver #39617

cookiecrook opened this issue Apr 20, 2023 · 6 comments · Fixed by #45833
Labels
accessibility infra wptrunner The automated test runner, commonly called through ./wpt run

Comments

@cookiecrook
Copy link
Contributor

cookiecrook commented Apr 20, 2023

Test harness crashes if test receives unknown error from WebDriver.

You can reproduce this in HEAD by running either of the /html-aam/fragile tests in WebKit.

https://wpt.fyi/results/html-aam/fragile?label=experimental&label=master&aligned

At first I thought it may have been a bug in WebKit, but we did some further investigation and it looks like this is just an implementation difference that Chromium isn't hitting for this test.

In this case, there aren't backing accessibility objects for these elements (in WebKit), so attempting to get the computedrole is triggering WebDriver to return "unknown error" based on the error code definitions defined in https://w3c.github.io/webdriver/#dfn-error-code ... It seems possible that Chromium or Gecko may hit the same problem in a different scenario.

Implementation differences aside, the test harness should be able to handle any defined WebDriver error type (including unknown error) without crashing and preventing other tests in the same file from being run.

@gsnedders
Copy link
Member

To me the most concerning thing here is "how do we end up with CRASH if nothing crashes"

@rakuco
Copy link
Member

rakuco commented May 3, 2023

To me the most concerning thing here is "how do we end up with CRASH if nothing crashes"

That's because executorwebdriver.py treats WebDriver "unknown errors" as crashes ever since it was added in #12380, maybe modeled after executorselenium.py.

@gsnedders
Copy link
Member

To me the most concerning thing here is "how do we end up with CRASH if nothing crashes"

That's because executorwebdriver.py treats WebDriver "unknown errors" as crashes ever since it was added in #12380, maybe modeled after executorselenium.py.

Ah, thanks for looking into this!

@jgraham do you have any opinion here?

@jgraham
Copy link
Contributor

jgraham commented Nov 7, 2023

@jgraham do you have any opinion here?

Not really. Gecko isn't using this, but doesn't treat unknown marionette errors as if the browser crashed. In general trying to figure out when a crash, particularly a content process crash, occurred depends a bit on browser-specific heuristics, so it may be necessary to change this code, or to make the exact set of exceptions product-specific (i.e. different for Safari, Chrome, etc.)

@cookiecrook
Copy link
Contributor Author

cookiecrook commented Nov 9, 2023

Seems like the test harness is no longer crashing. Now the test vends the result:

FAIL message: promise_test: Unhandled rejection with value: "error: Action get_computed_role failed"

Which I think is probably okay? @gsnedders?

@gsnedders
Copy link
Member

Seems like the test harness is no longer crashing. Now the test vends the result:

FAIL message: promise_test: Unhandled rejection with value: "error: Action get_computed_role failed"

Which I think is probably okay? @gsnedders?

It is, but we should still maybe reconsider the wptrunner behaviour here.

@gsnedders gsnedders added infra wptrunner The automated test runner, commonly called through ./wpt run and removed webdriver labels Nov 9, 2023
jonathan-j-lee added a commit to jonathan-j-lee/wpt that referenced this issue Apr 22, 2024
This aligns with how the base class `TimedRunner` differentiates
`CRASH` from other failure modes [0].

An `InvalidSessionIdException` after a browser death (e.g., [1]) is now
coerced to `CRASH`, not the generic `INTERNAL-ERROR` for harness
exceptions.

Fixes web-platform-tests#39617 and https://crbug.com/41485463.

[0]: https://github.com/web-platform-tests/wpt/blob/c338ceff/tools/wptrunner/wptrunner/executors/base.py#L216-L231
[1]: https://github.com/web-platform-tests/wpt/blob/c338ceff/tools/wptrunner/wptrunner/executors/base.py#L773
jonathan-j-lee added a commit that referenced this issue Apr 22, 2024
This aligns with how the base class `TimedRunner` differentiates
`CRASH` from other failure modes [0].

An `InvalidSessionIdException` after a browser death (e.g., [1]) is now
coerced to `CRASH`, not the generic `INTERNAL-ERROR` for harness
exceptions.

Fixes #39617 and https://crbug.com/41485463.

[0]: https://github.com/web-platform-tests/wpt/blob/c338ceff/tools/wptrunner/wptrunner/executors/base.py#L216-L231
[1]: https://github.com/web-platform-tests/wpt/blob/c338ceff/tools/wptrunner/wptrunner/executors/base.py#L773
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility infra wptrunner The automated test runner, commonly called through ./wpt run
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants