feat: add support for userContext in WebDriver BiDi events#56744
feat: add support for userContext in WebDriver BiDi events#56744jimevans wants to merge 17 commits into
Conversation
whimboo
left a comment
There was a problem hiding this comment.
@jimevans I'm sorry that I completely missed your PR. I just stumbled over it. Maybe next time please send a ping if you don't get a review within a week?
Due to the changes to make the userContext optional for some time first, this PR needs some updates. Would you still be happy to get it updated?
|
@whimboo Please feel free to take another look now. |
|
Thank you @jimevans. I'll take a look next week. It would be good for verification when I could quickly check with an implementation for it on our side. |
|
@whimboo From a test authoring perspective, what's left for getting these merged? |
whimboo
left a comment
There was a problem hiding this comment.
Sorry for the late reply but I actually missed to submit my review comments :(
@juliandescottes might be able to help with these updates.
It would be good to see the failures fixed before we merge this PR.
|
+1 to what was mentioned by @whimboo, let's at least fix the errors here. I tried to validate this against my implementation for https://bugzilla.mozilla.org/show_bug.cgi?id=2018611, but since the new fields are all considered as optional, it's hard to know whether I'm really passing the userContext tests or simply falling back to the "optional" branch. This is unfortunate because that will expose us to regressions until we make the fields mandatory. So I would suggest adding at least one smoke test per event / return value where we expect unconditionally the userContext field. Browsers which don't implement it yet will fail the additional test for now, but at least it would give us some indication that the feature is working. Edit: in the interest of keeping things moving forward, I can add such tests in https://bugzilla.mozilla.org/show_bug.cgi?id=2018611. I need this PR to land and be synced before landing my changes. |
That's a good idea. Lets only make sure that the changes on this PR actually allow both Firefox and Chrome to pass the tests, and add additional tests like a |
|
@whimboo, @juliandescottes, I think I've fixed the extant issues in the PR. Let me know what other issues there may be. |
I'll pull and test again, thanks for the update! |
juliandescottes
left a comment
There was a problem hiding this comment.
Just one issue spotted, other than this the tests seem to run fine.
|
The other thing which might be worth adding in this changeset is a small validation on the browsingContext.create result: Ideally we would validate that if present, it's a string. |
Co-authored-by: Julian Descottes <jdescottes@mozilla.com>
|
@juliandescottes I've added the conditional assertion of |
| assert_console_entry(event_data, method="log") | ||
|
|
||
|
|
||
| async def test_new_context_with_new_window(bidi_session, subscribe_events, top_context, wait_for_event, wait_for_future_safe): |
There was a problem hiding this comment.
Hm, this test still fails in both Firefox and Chrome and it looks like to be permanent. The failure message is message: KeyError: 'userContext'. The changes look valid to me so not sure which of the two assert_console_entry calls fails.
@juliandescottes can you reproduce it locally? If not we could ignore.
There was a problem hiding this comment.
@whimboo I've pushed a change to attempt to resolve this issue. Let's see how we do.
Adds tests for WebDriver BiDi spec PR 1046.