web-platform-tests / rfcs

web-platform-tests RFCs
75 stars 63 forks source link

RFC 112: Proxy for connection timing tests #112

Closed noamr closed 2 years ago

noamr commented 2 years ago

Use an http proxy and a PAC file to create artificial connection delays and use fresh domain names for tests.

Can help test many features that are currently untested, see for example https://github.com/whatwg/fetch/pull/1429, https://github.com/web-platform-tests/wpt/issues/27935, and https://github.com/web-platform-tests/wpt/issues/13465

noamr commented 2 years ago

/cc @jgraham @annevk @yoavweiss

jgraham commented 2 years ago

I think I'd like some more details here.

AFAICT the intent is not exactly to use a proxy, but to have a PAC script that's installed for certain tests and does things like spins a js loop to delay the connection by some amount of time? Have you tested this approach across multiple browsers? It doesn't seem implausible that browsers would detect such a PAC script as misbehaving and disable it (I have no idea if that's true or not).

I also don't understand what the proposal is in terms of how this integrates with the rest of wpt. Do you get to specify a PAC script per test? How is that specified? Or are you thinking of a single PAC for all tests?

What's the use case that motivates allowing connections to arbitary domains?

I also wonder if we are able to use PAC files we could also use that for tests that want to test the behaviour of default ports, even though we can't guarantee the actual server is available on those ports.

noamr commented 2 years ago

I think I'd like some more details here.

AFAICT the intent is not exactly to use a proxy, but to have a PAC script that's installed for certain tests and does things like spins a js loop to delay the connection by some amount of time? Have you tested this approach across multiple browsers? It doesn't seem implausible that browsers would detect such a PAC script as misbehaving and disable it (I have no idea if that's true or not).

I haven't encountered this problem in my initial tests of this.

I also don't understand what the proposal is in terms of how this integrates with the rest of wpt. Do you get to specify a PAC script per test? How is that specified? Or are you thinking of a single PAC for all tests?

I was thinking to have one PAC for all of WPT, with specific HOST patterns(e.g. *.wpt.proxy?) that have an effect, and with everything else going to DIRECT, which means current tests would not be effected, and tests that want to use it would have to extend it and potentially use their own host patterns.

What's the use case that motivates allowing connections to arbitary domains?

Those are discussed extensively in https://github.com/web-platform-tests/wpt/issues/13465.

I also wonder if we are able to use PAC files we could also use that for tests that want to test the behaviour of default ports, even though we can't guarantee the actual server is available on those ports.

Yes I think it's a viable use case.

jgraham commented 2 years ago

Thanks. Can you write up those points as part of the RFC; the intent is that it should stand alone in terms of describing use cases, rather than needing to refer to other discussions for key parts of the proposal.

One concern is that a global PAC file might end up really hard to maintain, since it could be handling the requirements of many different tests. Being able to specify the PAC per test file might allow each PAC file to be simpler and be updatable without having to worry about unexpectedly breaking tests elsewhere.

noamr commented 2 years ago

Thanks. Can you write up those points as part of the RFC; the intent is that it should stand alone in terms of describing use cases, rather than needing to refer to other discussions for key parts of the proposal.

One concern is that a global PAC file might end up really hard to maintain, since it could be handling the requirements of many different tests. Being able to specify the PAC per test file might allow each PAC file to be simpler and be updatable without having to worry about unexpectedly breaking tests elsewhere.

Agreed, but that would require running WPT separately for those tests, which creates other complexities. Maybe this can be done if we need it in the future? I'll add this as a risk to the RFC.

jgraham commented 2 years ago

wptrunner can handle browser configuration that changes for different tests (e.g. in gecko we're able to set prefs per-test). So as long as you're running through that then the need for multiple PAC files isn't a big problem. It does make it harder to manually configure the browser, but that's generally not a well-supported use case (e.g. testdriver typically relies on the runner).

noamr commented 2 years ago

wptrunner can handle browser configuration that changes for different tests (e.g. in gecko we're able to set prefs per-test). So as long as you're running through that then the need for multiple PAC files isn't a big problem. It does make it harder to manually configure the browser, but that's generally not a well-supported use case (e.g. testdriver typically relies on the runner).

OK, I can give that a try. I think it's OK to run the browser manually with e.g. --proxy-pac-url when that is needed.

noamr commented 2 years ago

Thanks. Can you write up those points as part of the RFC; the intent is that it should stand alone in terms of describing use cases, rather than needing to refer to other discussions for key parts of the proposal.

One concern is that a global PAC file might end up really hard to maintain, since it could be handling the requirements of many different tests. Being able to specify the PAC per test file might allow each PAC file to be simpler and be updatable without having to worry about unexpectedly breaking tests elsewhere.

RFC updated with all these points.

noamr commented 2 years ago

Hi, anything still missing here? Need comments from more people?

jgraham commented 2 years ago

I think this needs to be updated for the certificate discussion we had on matrix; the fact that we don't have a way to support https on generic domains seems to me to be a significant problem with using this proposal for that use case.

I'd also like to see some feedback from others before proceeding.

noamr commented 2 years ago

I think this needs to be updated for the certificate discussion we had on matrix; the fact that we don't have a way to support https on generic domains seems to me to be a significant problem with using this proposal for that use case.

I'd also like to see some feedback from others before proceeding.

Updated. Would be happy to get more feedback, I'll ask on Matrix and see if more people respond.

foolip commented 2 years ago

@DanielRyanSmith can you review this, evaluating how it would work in Chromium's infrastructure? In particular, would it require us to switch to wptrunner for tests using this to work in Chromium CI?

DanielRyanSmith commented 2 years ago

Sorry I missed this notification but I'll look into this later today!

Edit: Haven't just blown this off - just trying to get a clearer picture of this functionality being available for Chromium testing outside of using wptrunner

DanielRyanSmith commented 2 years ago

Without switching to wptrunner, we would have to add PAC file functionality to our current process for running Chromium CI tests (run_web_tests using content_shell). First interactions with the team indicate that this implementation would not be terribly difficult, but would require extra time and effort. I've been told that this is a question to pose to you @noamr - is taking on this work to add PAC support for our current Chromium CI process something you'd feel comfortable adding to your bandwidth?

noamr commented 2 years ago

Without switching to wptrunner, we would have to add PAC file functionality to our current process for running Chromium CI tests (run_web_tests using content_shell). First interactions with the team indicate that this implementation would not be terribly difficult, but would require extra time and effort. I've been told that this is a question to pose to you @noamr - is taking on this work to add PAC support for our current Chromium CI process something you'd feel comfortable adding to your bandwidth?

Sure! I'm working on several of these things in parallel but if we agree on this as the direction I can take on the Chromium CI bit as part of it.

foolip commented 2 years ago

Thanks @DanielRyanSmith for digging into this, and it's great to hear @noamr that you'd be able to set this up in Chromium CI.

The proposal makes sense to me, there are limitations (HTTPS) but even with those it seems like a great addition.

Is your intention that for <meta name="pac" content="resources/delay.js"> that "resources/delay.js" would go into the manifest? Would all tests that use the same PAC file then be run as a single group to avoid restarts? In https://github.com/web-platform-tests/rfcs/pull/112#issuecomment-1110854728 you're suggesting one global PAC file, so does that change the proposal, should the "resources/delay.js" bit not be included?

noamr commented 2 years ago

Thanks @DanielRyanSmith for digging into this, and it's great to hear @noamr that you'd be able to set this up in Chromium CI.

Yea I will give it a shot once the RFC is accepted :)

The proposal makes sense to me, there are limitations (HTTPS) but even with those it seems like a great addition.

Is your intention that for <meta name="pac" content="resources/delay.js"> that "resources/delay.js" would go into the manifest?

I only started to look at the mechanics, but in general it would work similar to how the dpi attribute is supposed to work.

Would all tests that use the same PAC file then be run as a single group to avoid restarts? In #112 (comment) you're suggesting one global PAC file, so does that change the proposal, should the "resources/delay.js" bit not be included?

The proposal has changed, and does no longer contain a global proxy file. My first private prototype was using browser settings rather than manifest, which made it so that whenever a test with a different PAC is found the browser restarts, but I guess manifests would be more optimal. Could use help with this bit of the implementation, the python code is far from trivial.

I saw that manifests are generally used for expectation management, is there any current example where they're used for grouping based on environment?

noamr commented 2 years ago

Thanks @DanielRyanSmith for digging into this, and it's great to hear @noamr that you'd be able to set this up in Chromium CI.

The proposal makes sense to me, there are limitations (HTTPS) but even with those it seems like a great addition.

Is your intention that for <meta name="pac" content="resources/delay.js"> that "resources/delay.js" would go into the manifest? Would all tests that use the same PAC file then be run as a single group to avoid restarts? In #112 (comment) you're suggesting one global PAC file, so does that change the proposal, should the "resources/delay.js" bit not be included?

I checked the implementation a bit, and it seems like I can use the existing proxyAutoconfigUrl capability exposed by WebDriver, which means:

noamr commented 2 years ago

WIP implementation: https://github.com/web-platform-tests/wpt/pull/34145, since some implementation questions were brought up. Will move on with staging it once the RFC is merged.

noamr commented 2 years ago

Although I'm still worried by the limitations around https support, I think this is something that is at least worth trying and has the potential to unlock some testing scenarios that we've been unable to support until now.

Another risk that should be mentioned is that these tests will not be able to run on e.g. w3c-test.live unless the user manually installs the relevant proxy file. It's arguably a new limitation that tests can't run directly from the server, although in practice running testdriver-using tests without wptrunner is often not possible.

Exactly, it's a wptrunner test. Perhaps those tests can include a text paragraph that manual testers can know to install the pac file manually.

Related to that I think we need to use the subsitution feature for the pac files so they can depend on the configured server setup and not hardcode specific addresses or ports.

I agree re using sub for the pac files, sure!

noamr commented 2 years ago

@jgraham: I don't have merge access, would you merge at your convenience please? Thanks!

noamr commented 1 year ago

OK something I haven't considered and didn't know is that Mac doesn't support programatically setting a PAC file - not just for Safari but also for Chrome. So I think instead of doing this we should allow a simple setting that proxies everything to the WPT server - this would cover most required tests and should work on Mac.