[webdriver]: add checks for execute_script tests recognising promises

[christian-bromann]

I've been come across point 4, 5, 6 and 7 of the [execute_script](https://w3c.github.io/webdriver/#execute-script) recognising promises if returned as such and apparently Geckodriver does resolve/reject a promise properly but don't recognise script timeouts for it (see #13781 (comment)). Chromedriver always returns with status code 200 and value: {}.

This is my first stab at this and would love to get initial feedback as it seems that drivers behave differently/inconsistent here which can be either a bug in the driver or in the protocol definition.

[whimboo]

In case of the rejection of a promise a Javascript error has to be thrown. Given the error table it needs a HTTP response of 500. If geckodriver responds with 200, then this is clearly a bug in our implementation. @christian-bromann can you please raise a geckodriver bug?

[whimboo]

Thank you @christian-bromann for having a look at those steps of Execute Script. Your contribution is welcome! I added a couple of inline comments you want to have a look at. Also I think it might be ok, to just add those tests into execute.py directly.

Once all issues have been clarified maybe you also want to add similar tests for Execute Async Script?

[andreastt]

Thanks for writing these tests! 👍🏻

We have https://bugzilla.mozilla.org/show_bug.cgi?id=1398095 tracking the implementation of this in Gecko, and we are committed to implementing this soon-ish.

Notifying @jleyba of this change, since he added promises to the WebDriver spec in the first place.

[christian-bromann]

@whimboo @andreastt I applied your feedback and realised that Geckodriver is actually already handling rejected promises well as unknown error. However I find it confusing is that execute_script is not recognising script timeouts when returning a promise. I guess this should be mentioned in the spec or fixed in the driver. A script can therefor stale the command when the promise is never resolved. Will update PR text.

So right now the following tests are failing:

• test_promise_resolve_timeout
• test_promise_reject_timeout

[christian-bromann]

Also question: the reason why I initial thought that Geckodriver doesn't handle rejected promises correctly is because I rejected it with a string reject('error') instead an error reject(new Error('error')). Do we want to have a check for this in the spec to transform it to an error or resolve it and treat it as resolved promises which definitely can cause confusion.

Running a small test showed that Chrome is still rejecting the promise whatsoever:

Promise.reject('hi').then(
  () => console.log("a"),
  () => console.log("b"))
> "b"

So we might want to also return with 500 and whatever was put into the reject method as payload.

[andreastt]

One thing that occurred to me is that we also want to test if awaiting promises internally in a script works:

execute_script("""
    await new Promise(…);
    // do something else
    return "foobar";
    """)

This means the wrapping function that WebDriver applies internally needs to be marked as an asynchronous function, but this may not be called out in the specification currently. Perhaps this should be solved in a separate patch as it likely requires prose changes as well.

As @whimboo pointed out, I’d also like you to address changes to the Execute Async Script command.

[andreastt]

@christian-bromann In your latest update, making a distinction between delayed and timed out promises is good.

Also question: the reason why I initial thought that Geckodriver doesn't handle rejected promises correctly is because I rejected it with a string reject('error') instead an error reject(new Error('error')). Do we want to have a check for this in the spec to transform it to an error or resolve it and treat it as resolved promises which definitely can cause confusion.

What the spec says is that the rejected value should be placed in a data field, so we should have a test for that.

What is not called out in the spec is actually how to serialise the rejected data. For example, how do we know what to conver the Error into? I think this may be a different bug that we should address separately of this PR.

Does that answer your question?

[christian-bromann]

@andreastt I added two more test types to this:

• unveiling of Promise.all
• proper internal wrapping into async function (fails in Geckodriver)

I see the following things need to get spec'ed out:

• wrapping reject reasons into an error, I suggest the following:
  • if reason instance of Error return with javascript error and WebDriver error message containing error message and WebDriver error stack property containing error stack of given error object
  • if reason is type of string return with javascript error and WebDriver error message as that string
  • anything else return unknown error
• function body of script argument has to be executed within an async context (https://w3c.github.io/webdriver/#dfn-extract-the-script-arguments-from-a-request needs enhancement)

I would love to create an issue for both in the Webdriver repo if you think that is the right way to move forward. Then I can split this PR up into multiple ones.

[andreastt]

I would like to see a similar test where the promise rejects, to test that the return value isn’t what is being returned. But happy to see a follow-up PR for that!

[andreastt]

This looks good now, with two caveats:

• Missing test for data field in error object.
• Test that "foo" isn’t returned in await Promise.reject(…); return "foo";.

@christian-bromann Do you want to take on fixing these up as well in follow-up PRs?

old review

[andreastt]

Before we can land this, you first need to fix the lints: https://travis-ci.org/web-platform-tests/wpt/jobs/448776034

[christian-bromann]

@andreastt I fixed lint error and added a return value to the await Promise.reject() test we already had. I am not sure though what you mean by:

Missing test for data field in error object.

[andreastt]

The error object can contain a data field:

If the error data dictionary contains any entries, set the data field on body to a new JSON Object populated with the dictionary.

However, it’s definition says it is optional:

An error data dictionary is a mapping of string keys to JSON serializable values that can optionally be included with error objects.

So as long as it’s optional there’s nothing we can test here, I guess.

[andreastt]

@christian-bromann Landed your changes, finally! Do you want to have a look at writing promise tests for Execute Async Script?

[christian-bromann]

@andreastt yes