Add tests for thenables returned from async script

[jugglinmike]

No description provided.

[jugglinmike]

wpt.fyi results: https://wpt.fyi/results/webdriver/tests/classic/execute_async_script/promise.py?sha=160437b

The intent of this patch is to surface a discrepancy between the spec and the implementations, namely: that promise fulfillment does not actually influence the command. At the moment, wpt.fyi only includes data for Chrome, but I've observed the same in Firefox and Safari.

[jugglinmike]

Relevant spec patch: w3c/webdriver#1939

[whimboo]

Thank you @jugglinmike. Overall I think this looks good. I've some inline comments to check and to apply. Please take a look.

Also we should have the same / similar tests for execute_script as well (except for the resolve() case).

[jugglinmike]

Also we should have the same / similar tests for execute_script as well (except for the resolve() case).

I believe the only test that needs replicating is test_returned_poisoned_thenable since the "fulfilled promise case is covered by the existing test named test_await_promise_resolve and the "rejected promise" case is covered by the existing test named test_promise_reject. I'm happy to write more if I'm misunderstanding something, though!

[jugglinmike]

@whimboo I think I've addressed all your feedback. Should we get another review, or do you think this is ready to merge?

[whimboo]

Shouldn't we do it actually similar to the above case for fulfillment and resolve the promise with a different error? That way Promise.reject should still have precedence, right?

[jugglinmike]

Shouldn't we do it actually similar to the above case for fulfillment and resolve the promise with a different error? That way Promise.reject should still have precedence, right?

Ah, yes. I didn't recognize how renaming the test made the callback relevant here. I've made that change and pushed it to this branch.

Doing so helped me recognize that we're not being completely systematic here. There are three variables: the callback result (which should be considered an error if it is a rejected promise), the promise result, and the sequence in which they occur.

I think we actually need 8 tests, not just two:

callback result	promise result	sequence	subtest title	expected
"callback"	"promise"	callback then promise	test_callback_success_then_promise_success	"callback"
"callback"	"promise"	promise then callback	test_promise_success_then_callback_success	"promise"1
error	"promise"	callback then promise	test_callback_error_then_promise_success	error
error	"promise"	promise then callback	test_promise_success_then_callback_error	"promise"1
"callback"	error	callback then promise	test_callback_success_then_promise_error	"callback"
"callback"	error	promise then callback	test_promise_error_then_callback_success	error
error	error	callback then promise	test_callback_error_then_promise_error	error
error	error	promise then callback	test_callback_error_then_promise_error	error

Though, given that we can't differentiate between script errors, we might skip those final two tests.

What do you think?

Footnotes

1. We'll change these if-and-when https://github.com/w3c/webdriver/pull/1939 lands ↩ ↩²