[wdspec] add network interception invalidation tests for all remaining methods by thiagowfx · Pull Request #42667 · web-platform-tests/wpt

thiagowfx · 2023-10-20T14:42:51Z

thiagowfx · 2023-10-23T08:56:30Z

Note the TODOs:

# TODO: Add body.
# TODO: Add cookies.
# TODO: Add headers.

Because we are type-annotating everything, these will require extra work. Deferring them to another PR.

juliandescottes

Looks good thanks! I need to take another look, but sharing some comments for now

webdriver/tests/bidi/network/continue_response/invalid.py

webdriver/tests/bidi/network/continue_request/invalid.py

webdriver/tests/bidi/network/continue_response/invalid.py

tools/webdriver/webdriver/bidi/modules/network.py

…ject

juliandescottes · 2023-12-07T18:00:36Z

Noticed another issue in continue_with_auth when adding "valid" tests locally. The credentials should be in a separate object. Instead they were at the top level of the command params. Re-flagging for review.

thiagowfx · 2023-12-11T14:38:50Z

@sadym-chromium would you take this PR over?

sadym-chromium · 2023-12-11T14:50:02Z

@sadym-chromium would you take this PR over?

yes, thanks!

tools/webdriver/webdriver/bidi/modules/network.py

webdriver/tests/bidi/network/continue_with_auth/invalid.py

webdriver/tests/bidi/network/provide_response/invalid.py

…ments

whimboo · 2023-12-11T22:01:50Z

webdriver/tests/bidi/network/fail_request/invalid.py

    await fetch(text_url)

-    response_completed_event = await wait_for_future_safe(on_response_completed)
+    response_completed_event = await on_response_completed


Could this change maybe be the reason why the wpt.fyi results show a timeout for this test file now with Chrome?

No, the job seems to timeout on the invalid phase test.
The spec is quite clear about the fact that this should be an error:

If phase is "authRequired", then return error with error code invalid argument.

But even if they don't implement the phase check this shouldn't timeout.

Chrome also seems to timeout on various other tests:

continue_with_auth/invalid.py::test_params_request_invalid_phase[responseStarted]

continue_response/invalid.py::test_params_reason_phrase_invalid_type[False]

All those tests have in common that they need to block a request in another phase than "beforeRequest". I remember that during the initial review of this patch, I requested several times to change the phase of tests from "beforeRequest" to the appropriate phase but this was not addressed, so it's possible that chrome doesn't support blocking the request in phases after "beforeRequest".

Looking at wpt.fyi for other intercept test, my guess is that chrome is always adding intercepts in the beforeRequest phase, even if you are specifying another phase. This would be enough to make the setup_blocked_request helper timeout.

cc @sadym-chromium can you confirm or tell me where the code handling intercepts is located so that I can check?

Unfortunately, for now our network interception implementation is racy and sometimes hangs forever. We are working on fixing it.

Wait. This test does not set interceptions, so it should work just fine. Let me check.

Sorry it's a bit misleading :)

The question from @whimboo was added for this test (test_params_request_no_such_request), but the actual timeout happens in test_params_request_invalid_phase (right above this one) which indirectly uses add_intercept via setup_blocked_request

tools/webdriver/webdriver/bidi/modules/network.py

whimboo

Thanks @juliandescottes. From my point of view this looks good now.

When merging it upstream we might have to wait for the next downstream sync. Maybe we can land on mozilla-central and then upstream sync, which is clearly faster?

juliandescottes · 2023-12-14T12:54:40Z

Thanks @juliandescottes. From my point of view this looks good now.

When merging it upstream we might have to wait for the next downstream sync. Maybe we can land on mozilla-central and then upstream sync, which is clearly faster?

That would work for me. @sadym-chromium any objection or additional review feedback ?

juliandescottes · 2023-12-14T13:43:19Z

Thanks for the review, pushed on phabricator at https://phabricator.services.mozilla.com/D196424 FYI.

…g methods Synced from #42667 Differential Revision: https://phabricator.services.mozilla.com/D196424 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1870032 gecko-commit: d3a29d02b3dd8d8cfbfff6fcad00d94226e102e8 gecko-reviewers: webdriver-reviewers, whimboo

juliandescottes · 2023-12-15T11:16:33Z

This is now a duplicate of #43688 , closing.

…r all remaining methods r=webdriver-reviewers,whimboo Synced from web-platform-tests/wpt#42667 Differential Revision: https://phabricator.services.mozilla.com/D196424

…g methods Synced from #42667 Differential Revision: https://phabricator.services.mozilla.com/D196424 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1870032 gecko-commit: d3a29d02b3dd8d8cfbfff6fcad00d94226e102e8 gecko-reviewers: webdriver-reviewers, whimboo

…r all remaining methods r=webdriver-reviewers,whimboo Synced from web-platform-tests/wpt#42667 Differential Revision: https://phabricator.services.mozilla.com/D196424 UltraBlame original commit: d3a29d02b3dd8d8cfbfff6fcad00d94226e102e8

…r all remaining methods r=webdriver-reviewers,whimboo Synced from web-platform-tests/wpt#42667 Differential Revision: https://phabricator.services.mozilla.com/D196424

…g methods Synced from #42667 Differential Revision: https://phabricator.services.mozilla.com/D196424 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1870032 gecko-commit: d3a29d02b3dd8d8cfbfff6fcad00d94226e102e8 gecko-reviewers: webdriver-reviewers, whimboo

thiagowfx force-pushed the thiagowfx/fail-request branch from fb2fcbd to aa4bba2 Compare October 20, 2023 14:43

thiagowfx force-pushed the thiagowfx/continue-request branch from 304dfa0 to 177e311 Compare October 20, 2023 14:49

moz-wptsync-bot added the mozilla:gecko-blocked label Oct 20, 2023

thiagowfx force-pushed the thiagowfx/fail-request branch from aa4bba2 to a97349b Compare October 23, 2023 08:42

thiagowfx force-pushed the thiagowfx/continue-request branch 4 times, most recently from df14886 to d09d5bd Compare October 23, 2023 08:55

thiagowfx requested review from juliandescottes and sadym-chromium October 23, 2023 08:55

thiagowfx assigned juliandescottes and sadym-chromium Oct 23, 2023

thiagowfx marked this pull request as ready for review October 23, 2023 08:55

wpt-pr-bot added infra webdriver webdriver-bidi wg-testing_browser labels Oct 23, 2023

wpt-pr-bot requested review from AutomatedTester, DanielRyanSmith, foolip, jgraham, jrandolf-google, shs96c and whimboo October 23, 2023 08:55

Base automatically changed from thiagowfx/fail-request to master October 23, 2023 09:04

thiagowfx force-pushed the thiagowfx/continue-request branch 2 times, most recently from ad8f6fe to a884b62 Compare October 24, 2023 00:01

juliandescottes reviewed Oct 24, 2023

View reviewed changes

thiagowfx changed the title ~~test: add network interception continue request invalidation tests~~ test: add network interception "continue request" and "continue response" invalidation tests Oct 24, 2023

moz-wptsync-bot added mozilla:gecko-blocked and removed mozilla:gecko-blocked labels Dec 6, 2023

[wdspec] continueWithAuth parameters should have credentials in an ob…

e4b083e

…ject

juliandescottes requested a review from whimboo December 7, 2023 18:00

whimboo requested changes Dec 11, 2023

View reviewed changes

tools/webdriver/webdriver/bidi/modules/network.py Show resolved Hide resolved

webdriver/tests/bidi/network/continue_with_auth/invalid.py Show resolved Hide resolved

webdriver/tests/bidi/network/provide_response/invalid.py Show resolved Hide resolved

juliandescottes added 2 commits December 11, 2023 19:13

[wdspec] Add invalid type and value tests for credentials details

6ba557b

[wdspec] Add TODOs in bidi network continue commands for missing argu…

e0ed5ca

…ments

whimboo reviewed Dec 11, 2023

View reviewed changes

whimboo requested changes Dec 13, 2023

View reviewed changes

tools/webdriver/webdriver/bidi/modules/network.py Outdated Show resolved Hide resolved

[wdspec] Add AuthCredentials class for continueWithAuth credentials

2bee456

whimboo approved these changes Dec 13, 2023

View reviewed changes

sadym-chromium approved these changes Dec 14, 2023

View reviewed changes

moz-wptsync-bot mentioned this pull request Dec 15, 2023

[Gecko Bug 1870032] [wdspec] add network interception invalidation tests for all remaining methods #43688

Merged

juliandescottes closed this Dec 15, 2023

moz-wptsync-bot removed the mozilla:gecko-blocked label Dec 15, 2023

thiagowfx deleted the thiagowfx/continue-request branch February 1, 2024 05:46

Conversation

thiagowfx commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thiagowfx commented Oct 23, 2023

Uh oh!

juliandescottes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

juliandescottes commented Dec 7, 2023

Uh oh!

thiagowfx commented Dec 11, 2023

Uh oh!

sadym-chromium commented Dec 11, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

whimboo Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

juliandescottes Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

sadym-chromium Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

sadym-chromium Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

juliandescottes Dec 12, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

whimboo left a comment

Choose a reason for hiding this comment

Uh oh!

juliandescottes commented Dec 14, 2023

Uh oh!

juliandescottes commented Dec 14, 2023

Uh oh!

juliandescottes commented Dec 15, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

thiagowfx commented Oct 20, 2023 •

edited

Loading