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

Fix hostnames #9480

Merged
merged 1 commit into from Feb 16, 2018

Conversation

Projects
None yet
4 participants
@jugglinmike
Copy link
Contributor

jugglinmike commented Feb 12, 2018

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 Reviewable

@jugglinmike jugglinmike requested review from gsnedders and jgraham Feb 12, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Feb 12, 2018

Build PASSED

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

View more information about this build on:

@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Feb 12, 2018

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

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Feb 12, 2018

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

@@ -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",

This comment has been minimized.

Copy link
@gsnedders

gsnedders Feb 13, 2018

Contributor

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.

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

This comment has been minimized.

Copy link
@gsnedders

gsnedders Feb 13, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Feb 14, 2018

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

This comment has been minimized.

Copy link
Contributor

gsnedders commented Feb 14, 2018

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.

@jugglinmike jugglinmike force-pushed the bocoup:fix-hostnames branch from b47c410 to aa44f39 Feb 14, 2018

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Feb 15, 2018

@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).

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

This comment has been minimized.

Copy link
@gsnedders

gsnedders Feb 15, 2018

Contributor

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

This comment has been minimized.

Copy link
Contributor

gsnedders commented Feb 15, 2018

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

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Feb 15, 2018

  • 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

This comment has been minimized.

Copy link
Contributor

gsnedders commented Feb 15, 2018

@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

This comment has been minimized.

Copy link
Contributor

gsnedders commented Feb 15, 2018

@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 jugglinmike force-pushed the bocoup:fix-hostnames branch from 4f0a6b3 to bc82c5e Feb 15, 2018

@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Feb 15, 2018

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

This comment has been minimized.

Copy link
Contributor

gsnedders commented Feb 15, 2018

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

wptrunner: Fix regression in config, interpreter
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

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Feb 15, 2018

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

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Feb 16, 2018

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

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

gsnedders added a commit to gsnedders/web-platform-tests that referenced this pull request Mar 2, 2018

Fix web-platform-tests#9750: bind_hostname needs to be a Python bool
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).

gsnedders added a commit to gsnedders/web-platform-tests that referenced this pull request Mar 5, 2018

Fix web-platform-tests#9750: bind_hostname needs to be a Python bool
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).

gsnedders added a commit to gsnedders/web-platform-tests that referenced this pull request Mar 5, 2018

Fix web-platform-tests#9750: bind_hostname needs to be a Python bool
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).

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

Fix #9750: bind_hostname needs to be a Python bool
db55c16 fixed this for Servo;
"true"/"false" worked when it was interpolated as JSON, but that is no
longer the case since cb2d75f (#9480).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.