Fix hostnames #9480

jugglinmike · 2018-02-12T15:35:27Z

I believe there was an error in gh-8614. Since it removed the hosts property, the templated config.json file which referenced that property can no longer be expanded. Here's an example of the error I encountered when running ./wpt run chrome locally:

Traceback (most recent call last):
  File "./wpt", line 5, in <module>
    wpt.main()
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wpt/wpt.py", line 132, in main
    rv = script(*args, **kwargs)
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wpt/run.py", line 419, in run
    rv = run_single(venv, **kwargs) > 0
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wpt/run.py", line 426, in run_single
    return wptrunner.start(**kwargs)
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wptrunner/wptrunner/wptrunner.py", line 295, in start
    return not run_tests(**kwargs)
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wptrunner/wptrunner/wptrunner.py", line 185, in run_tests
    env_extras) as test_environment:
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wptrunner/wptrunner/environment.py", line 96, in __enter__
    self.config = self.load_config()
  File "/home/mike/projects/bocoup/google-wpt/web-platform-tests/tools/wptrunner/wptrunner/environment.py", line 132, in load_config
    local_config = json.loads(data % self.options)
KeyError: 'host'

This hasn't interrupted things on CI because the Firefox configuration object still includes the hosts
property, and the builds for the effected browsers are configured as "allowed failures". Those builds have recently been failing for this same reason.

The value in the Firefox configuration (127.0.0.1) differs from the value shared by all other browsers (web-platform.test). gh-8614 is fairly clear in its goal to reduce variability, so I'm assuming this disparity is not necessary/desirable. Since I can't think of any reason why the host should be different for Firefox, I've authored the patch to normalize on web-platform.test). If I'm wrong about that, then we can omit the second commit on this branch and apply only the first commit (which reverts the
original change).

This change is

w3c-bots · 2018-02-12T15:47:56Z

Build PASSED

Started: 2018-02-16 00:27:10
Finished: 2018-02-16 00:50:16

View more information about this build on:

jugglinmike · 2018-02-12T16:31:13Z

It appears that gh-8614 introduced an additional regression for the Sauce Labs integration. I'm investigating that now. In the mean time, the passing TravisCI build for Chrome demonstrates that this patch has the desired effect.

jugglinmike · 2018-02-12T18:23:29Z

Okay, I think I've got that bug licked. I've submitted a patch at gh-9484. It depends on this patch.

gsnedders · 2018-02-13T16:30:27Z

tools/wptrunner/wptrunner/browsers/firefox.py

@@ -117,8 +117,7 @@ def env_extras(**kwargs):


 def env_options():
-    return {"host": "127.0.0.1",
-            "external_host": "web-platform.test",
+    return {"external_host": "web-platform.test",


The host is deliberately not the default on Firefox, because Firefox is set to always resolve the specified domains to localhost through its network.dns.localDomains pref.

gsnedders · 2018-02-13T17:17:15Z

tools/wptrunner/wptrunner/config.json

@@ -1,4 +1,4 @@
-{"host": "%(host)s",
+{"host": "web-platform.test",


Given the goal was to avoid hardcoding the domain everywhere; pretty sure the right fix for this is to make sure we always have an appropriate host provided when we evaluate it (or just drop it if we don't).

jugglinmike · 2018-02-14T00:18:53Z

Given the goal was to avoid hardcoding the domain everywhere;

I hope I didn't give the impression that I wasn't paying attention. I
interpreted the original goal to be reducing duplication, which is how I landed
on the change I'm proposing here. Now that I know the variation in the Firefox
configuration was intentional, I can see that this is an inappropriate
solution.

pretty sure the right fix for this is to make sure we always have an
appropriate host provided when we evaluate it (or just drop it if we don't).

I'm not sure what you mean here. Are you suggesting:

Omit the host property from the "config" JSON template, and
Dynamically extend the parsed object value with a host property if it is
specified in the "environment options"

Or:

specify 'web-platform.test' as the default value of the host property in
the "config" object, and
allow that value to be optionally overridden via an "environment option"
named host?

Or maybe something else?

gsnedders · 2018-02-14T15:20:21Z

I'm not sure what you mean here. Are you suggesting:

I'm not sure which I'm suggesting either. :) I think I had omitting it from the JSON and add it in the Python code in mind? Honestly I'm not sure what benefit the JSON file brings versus just doing it all in the Python code.

gsnedders · 2018-02-15T00:11:32Z

@jugglinmike we (as in one of us!) should check why https://github.com/w3c/web-platform-tests/blob/master/tools/wpt/tests/test_wpt.py#L94 didn't catch this BTW (I presume, as I was talking to @jgraham about recently, the fact that the exit code is almost always zero, regardless of whether we've had a critical error or not).

gsnedders · 2018-02-15T00:15:32Z

tools/wptrunner/wptrunner/browsers/firefox.py

+    # > set to always resolve the specified domains to localhost through its
+    # > `network.dns.localDomains` pref.
+    #
+    # https://github.com/w3c/web-platform-tests/pull/9480


Let's shorten this comment:

The server host is set to 127.0.0.1 as Firefox is configured (through the network.dns.localDomains preference set below) to resolve the test domains to localhost without relying on the network stack.

Or something like that. I'm not entirely sure how to word this?

gsnedders · 2018-02-15T00:20:01Z

I'll take a closer look tomorrow, but:

config.json needs to go from tools/wptrunner/MANIFEST.in and tools/wptrunner/setup.py too (grep tools/wptrunner to make sure I haven't missed anything else)
can you write some tests for TestEnvironment.load_config?

jugglinmike · 2018-02-15T17:01:27Z

config.json needs to go from tools/wptrunner/MANIFEST.in and
tools/wptrunner/setup.py too (grep tools/wptrunner to make sure I haven't
missed anything else)

Sure. There's a new "fixup!" commit with those deletions.

can you write some tests for TestEnvironment.load_config?

I've attempted to do so, but I ran up against a confusing error almost immediately (attempting to run using the command tox from the tools/wptrunner directory):

_______________________________________________________________________________ ERROR collecting wptrunner/tests/test_environment.py _______________________________________________________________________________
ImportError while importing test module '/home/mike/projects/web-platform-tests/tools/wptrunner/wptrunner/tests/test_environment.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
wptrunner/tests/test_environment.py:3: in <module>
    from .. import environment
wptrunner/environment.py:12: in <module>
    from wptserve.handlers import StringHandler
E   ImportError: No module named wptserve.handlers

I appreciate your interest in improving test coverage for the WPT CLI. I'd like to help out, but as you mentioned (and as my confusion above demonstrates), that will take some time. As we approach a week since the regression was introduced (and as we receive issue reports like gh-9520), I am feeling increased urgency about restoring the original functionality.

I submitted this patch as a way to restore functionality while honoring the refactoring, but maybe that is the wrong tack for this situation. If you would like to see this fix landed with tests, can we begin by reverting the regression in master as a stop gap?

gsnedders · 2018-02-15T17:18:28Z

@jugglinmike I'd been hoping it wouldn't be too hard to write tests and that we'd get it landed today. The ImportError is odd…

gsnedders · 2018-02-15T17:52:40Z

@jugglinmike I guess my suggestion is you clean up the history of this branch, get rid of the demo test file, and we land it after it runs through Travis?

jugglinmike · 2018-02-15T18:01:29Z

You got it. I almost forgot to update the in-line comment as you requested. The new text is now available on this branch.

The original branch is available at https://github.com/bocoup/web-platform-tests/tree/fix-hostnames-prebase for posterity.

gsnedders · 2018-02-15T18:07:52Z

@jugglinmike the first two commits are just a commit and a revert, no? so they can be dropped too.

Prior to the application of this patch, the configuration property named `host` was required to expand the templated JSON file maintained within the WPT CLI. By removing the property from some browser definitions without updating that template, commit 1b4f253 introduced failures when using the `run` sub-command with the effected browsers. Refactor the logic for generating the configuration to use an in-memory representation (instead of a file-based template) and conditionally extend this object with the `host` option for browsers which specify it. Document the rationale for the browser which requires this functionality.

jugglinmike · 2018-02-15T18:30:32Z

That's right. I've removed them and squashed the remaining commits with a new message so this changeset has the proper context in the git history.

jugglinmike · 2018-02-16T15:28:10Z

@gsnedders The TravisCI build passed. Could you please merge this patch on my behalf?

db55c16 fixed this for Servo; "true"/"false" worked when it was interpolated as JSON, but that is no longer the case since cb2d75f (web-platform-tests#9480).

db55c16 fixed this for Servo; "true"/"false" worked when it was interpolated as JSON, but that is no longer the case since cb2d75f (#9480).

jugglinmike requested review from gsnedders and jgraham February 12, 2018 15:35

wpt-pr-bot added infra demo labels Feb 12, 2018

jugglinmike mentioned this pull request Feb 12, 2018

Correct integration with Sauce Labs service #9484

Merged

mattl mentioned this pull request Feb 12, 2018

wpt cli changes breaks non-Firefox and Sauce Labs browsers web-platform-tests/results-collection#453

Closed

gsnedders requested changes Feb 13, 2018

View reviewed changes

jugglinmike force-pushed the fix-hostnames branch from b47c410 to aa44f39 Compare February 14, 2018 18:09

gsnedders reviewed Feb 15, 2018

View reviewed changes

gsnedders mentioned this pull request Feb 15, 2018

Travis CI is failing on Chrome, Safari, and Edge #9520

Closed

jugglinmike force-pushed the fix-hostnames branch from 4f0a6b3 to bc82c5e Compare February 15, 2018 17:57

gsnedders mentioned this pull request Feb 15, 2018

Some wpt integration tests being skipped because port in use #9546

Open

jugglinmike force-pushed the fix-hostnames branch from bc82c5e to c6d99f0 Compare February 15, 2018 18:21

jugglinmike mentioned this pull request Feb 16, 2018

recent changes to browsers/*.py broke running tests on Windows #9550

Closed

gsnedders approved these changes Feb 16, 2018

View reviewed changes

gsnedders merged commit cb2d75f into web-platform-tests:master Feb 16, 2018

gsnedders mentioned this pull request Mar 2, 2018

bind_hostname is always True (should be False in Firefox) #9750

Closed

gsnedders added a commit that referenced this pull request Mar 5, 2018

Fix #9750: bind_hostname needs to be a Python bool

8fd4164

db55c16 fixed this for Servo; "true"/"false" worked when it was interpolated as JSON, but that is no longer the case since cb2d75f (#9480).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hostnames #9480

Fix hostnames #9480

jugglinmike commented Feb 12, 2018 •

edited by wpt-reviewable-bot

w3c-bots commented Feb 12, 2018 •

edited

jugglinmike commented Feb 12, 2018

jugglinmike commented Feb 12, 2018

gsnedders Feb 13, 2018

gsnedders Feb 13, 2018

jugglinmike commented Feb 14, 2018

gsnedders commented Feb 14, 2018 •

edited

gsnedders commented Feb 15, 2018

gsnedders Feb 15, 2018 •

edited

gsnedders commented Feb 15, 2018

jugglinmike commented Feb 15, 2018

gsnedders commented Feb 15, 2018

gsnedders commented Feb 15, 2018

jugglinmike commented Feb 15, 2018

gsnedders commented Feb 15, 2018

jugglinmike commented Feb 15, 2018

jugglinmike commented Feb 16, 2018

		@@ -1,4 +1,4 @@
		{"host": "%(host)s",
		{"host": "web-platform.test",

Fix hostnames #9480

Fix hostnames #9480

Conversation

jugglinmike commented Feb 12, 2018 • edited by wpt-reviewable-bot

w3c-bots commented Feb 12, 2018 • edited

Build PASSED

jugglinmike commented Feb 12, 2018

jugglinmike commented Feb 12, 2018

gsnedders Feb 13, 2018

Choose a reason for hiding this comment

gsnedders Feb 13, 2018

Choose a reason for hiding this comment

jugglinmike commented Feb 14, 2018

gsnedders commented Feb 14, 2018 • edited

gsnedders commented Feb 15, 2018

gsnedders Feb 15, 2018 • edited

Choose a reason for hiding this comment

gsnedders commented Feb 15, 2018

jugglinmike commented Feb 15, 2018

gsnedders commented Feb 15, 2018

gsnedders commented Feb 15, 2018

jugglinmike commented Feb 15, 2018

gsnedders commented Feb 15, 2018

jugglinmike commented Feb 15, 2018

jugglinmike commented Feb 16, 2018

jugglinmike commented Feb 12, 2018 •

edited by wpt-reviewable-bot

w3c-bots commented Feb 12, 2018 •

edited

gsnedders commented Feb 14, 2018 •

edited

gsnedders Feb 15, 2018 •

edited