Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to run servers on different IP addresses. #28768

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

letitz
Copy link
Contributor

@letitz letitz commented Apr 30, 2021

This PR adds two new alternate_hosts to the default test configuration:

  • private: private-web-platform.test
  • public: public-web-platform.test

These are hosted by a collection of new servers bound to different IP addresses (127.1.0.1 and 127.2.0.1 respectively). All protocols and subdomains are supported. This results in a ~3x increase in the size of ./wpt make-hosts-file's output.

These new IP addresses are configurable using a new config property: alternate_ip_addresses, which maps host names to IP addresses.

Sending this for review now, but there is one last thing I need to fix: this probably does not work macOS, as noted in web-platform-tests/rfcs#79.

@letitz
Copy link
Contributor Author

letitz commented Apr 30, 2021

Seems that macOS indeed failed to start the additional servers. Windows on the other hand seems to have failed some tests due to them overriding alternate_hosts with a value that does not contain private and public keys.

@letitz
Copy link
Contributor Author

letitz commented May 4, 2021

Attempted to fix Windows test failures.

Along the way, I changed the way the server config is passed to WebDriver tests. Instead of serializing the whole config in an environment variable, I amended the code to dump the config as JSON in a temporary file (the tests are conveniently executed against a temporary directory) and pass the file path in the environment variable instead. This avoids repeatingly bumping into the 16-bit environment variable length limitation on Windows, and the need to special-case origin-policy subdomains.

I also added the py39 environment to tools/tox.ini so that I could run the tests on my machine, which does not have python 3.6-3.8 :)

@letitz
Copy link
Contributor Author

letitz commented May 4, 2021

Removed the tox.ini change, which had a merge conflict.

@jgraham
Copy link
Contributor

jgraham commented May 5, 2021

So my high level impression is that this looks quite reasonable. It might make sense to break the WebDriver config change into a seperate PR since that's somewhat orthogonal (and much less risky) than the rest of the changes. I think we're going to need to run this one through a full trigger run and in vendor CI to ensure we're not accidentially breaking anything.

@foolip
Copy link
Member

foolip commented May 6, 2021

@letitz do you know how you'd run this through Chromium's CI to look for regressions? If not let me know and I'm happy to chat. (We might have to update our copy of tools/ for the patch to apply and for the comparison to be meaningful.)

@letitz letitz mentioned this pull request May 6, 2021
@letitz
Copy link
Contributor Author

letitz commented May 6, 2021

@foolip I'm not sure.

My first thought was that I could download a patch version of this PR, apply it in a Chomium client and run CI jobs on the resulting patch. However, the tools/ folder does not exist in Chromium's mirror of the wpt repo: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/. I'm not sure how or even if Chromium relies on wptserve and wptrunner.

How do you maintainers usually do this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infra serve tox.ini webdriver wg-testing_browser wptrunner The automated test runner, commonly called through ./wpt run wptserve
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants