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

Correct integration with Sauce Labs service #9484

Merged

Conversation

Projects
None yet
5 participants
@jugglinmike
Copy link
Contributor

jugglinmike commented Feb 12, 2018

A recently merged patch centralized configuration values which defined the domain names available during test execution [1]. That patch was based on a mistaken assumption regarding the location of that value and the stage of the test execution at which it would be available. (Specifically: the value is not present in the environment configuration object when the "environment extras" are initialized.) This interfered with the initialization of the "environment" for the Sauce Labs service, making it impossible to run the tests there.

Defer the retrieval of the environment configuration object to the time that object's context manager is activated. Update the call sites to provide the environment configuration at this moment. Also update the signature of the context manager for the FontInstaller "environment extra" to accept this second parameter.

[1] #8614

This depends on gh-9480


This change is Reviewable

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

@jugglinmike jugglinmike referenced this pull request Feb 12, 2018

Merged

Fix hostnames #9480

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Feb 12, 2018

Build PASSED

Started: 2018-02-16 18:32:55
Finished: 2018-02-16 18:45:56

View more information about this build on:

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Feb 13, 2018

A recently merged patch centralized configuration values which defined the domain names available during test execution [1]. That patch was based on a mistaken assumption regarding the location of that value and the stage of the test execution at which it would be available. (Specifically: the value is not present in the environment configuration object when the "environment extras" are initialized.)

What exactly is the problem here? That the config isn't in the **kwargs passed to env_extras? Can this not be fixed much more easily by changing that so config is there? The callsite (wptrunner.py:145) has the config so it just needs added to the dict passed.

@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Feb 13, 2018

(Also, ergh, need tests for wptrunner.run_tests presumably to catch this in future.)

Somewhat sadly, I'm not going to ask you to write those tests to land this because that's easily a day or more's work, though if you're happy to please do!

@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Feb 14, 2018

As I understand it, the "config" we're interested in is not the value of the like-named parameter of the run_tests function (which is indeed available at line 145). The "config" we need here is actually created (and subsequently normalized) within the context manager for the TestEnvironment instance.

I haven't studied the surrounding code enough to understand the responsibilities of the two "config" objects, but adding some primitive debugging statements shows that the latter object has the properties we're interested in (see below).

Maybe we could move that code out of the context manager and into the constructor, and then instantiate the TestEnvironment before invoking get_env_extras on line 145. But that seems like a riskier alternative.

diff --git a/tools/wptrunner/wptrunner/environment.py b/tools/wptrunner/wptrunner/environment.py
index 764b479..f91d56f 100644
--- a/tools/wptrunner/wptrunner/environment.py
+++ b/tools/wptrunner/wptrunner/environment.py
@@ -96,6 +96,8 @@ class TestEnvironment(object):
         ports = serve.get_ports(self.config, self.ssl_env)
         self.config = serve.normalise_config(self.config, ports)

+        print 'TestEnvironment.enter: config = %s' % json.dumps(self.config, indent=2, sort_keys=True)
+
         for cm in self.env_extras:
             cm.__enter__(self.options, self.config)

diff --git a/tools/wptrunner/wptrunner/wptrunner.py b/tools/wptrunner/wptrunner/wptrunner.py
index d3d9d07..1577d78 100644
--- a/tools/wptrunner/wptrunner/wptrunner.py
+++ b/tools/wptrunner/wptrunner/wptrunner.py
@@ -142,6 +142,7 @@ def run_tests(config, test_paths, product, **kwargs):
          env_options, get_env_extras, run_info_extras) = products.load_product(config, product)

         ssl_env = env.ssl_env(logger, **kwargs)
+        print 'run_tests: config = %s' % json.dumps(config, indent=2, sort_keys=True)
         env_extras = get_env_extras(**kwargs)

         check_args(**kwargs)

Produces this output:

run_tests: config = {
  "manifest:default": {
    "metadata": "/home/mike/projects/web-platform-tests/meta",
    "tests": "/home/mike/projects/web-platform-tests/tests",
    "url_base": "/"
  },
  "products": {},
  "web-platform-tests": {
    "branch": "master",
    "remote_url": "https://github.com/w3c/web-platform-tests.git",
    "sync_path": "/home/mike/projects/web-platform-tests/sync"
  }
}

TestEnvironment.enter: config = {
  "aliases": [],
  "bind_hostname": false,
  "certificate": "/home/mike/projects/web-platform-tests/tools/certs/web-platform.test.pem",
  "check_subdomains": false,
  "doc_root": "/home/mike/projects/web-platform-tests",
  "domains": {
    "": "web-platform.test",
    "www": "www.web-platform.test",
    "www1": "www1.web-platform.test",
    "www2": "www2.web-platform.test",
    "\u00e9l\u00e8ve": "xn--lve-6lad.web-platform.test",
    "\u5929\u6c17\u306e\u826f\u3044\u65e5": "xn--n8j6ds53lwwkrqhv28a.web-platform.test"
  },
  "external_host": "web-platform.test",
  "host": "web-platform.test",
  "key_file": "/home/mike/projects/web-platform-tests/tools/certs/web-platform.test.key",
  "log_level": "debug",
  "not_domains": {
    "nonexistent-origin": "nonexistent-origin.web-platform.test"
  },
  "ports": {
    "http": [
      8000,
      8001
    ],
    "https": [
      8443
    ],
    "ws": [
      8888
    ],
    "wss": [
      34279
    ]
  },
  "ssl": {
    "encrypt_after_connect": false,
    "none": {},
    "openssl": {
      "base_conf_path": null,
      "base_path": "_certs",
      "force_regenerate": false,
      "openssl_binary": "openssl"
    },
    "pregenerated": {
      "host_cert_path": "tools/certs/web-platform.test.pem",
      "host_key_path": "tools/certs/web-platform.test.key"
    },
    "type": "pregenerated"
  },
  "ws_doc_root": "/home/mike/projects/web-platform-tests/websockets/handlers"
}
@gsnedders

This comment has been minimized.

Copy link
Contributor

gsnedders commented Feb 14, 2018

Oh, well there's the bigger part of my mistake there. :(

self.env_extras_cms = []

for env in self.env_extras:
cm = env.use(self.options, self.config)

This comment has been minimized.

Copy link
@jgraham

jgraham Feb 15, 2018

Contributor

I think instead of requiring a class with a use method here, we should make the protocol that env_extras is an iterable of callables that return context managers, so this line would be:
cms = env(self.options, self.config)

and in SauceConnect we would s/def use/def call`

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Feb 15, 2018

Author Contributor

Speaking personally, I very much prefer callables to be named with verbs that
describe the effect of their invocation. It's hard to say what environment()
does. I know that use is a pretty generic term, but it does give a little
more indication that some resource is being utilized.

That said, my priority is fixing the regression, so I'm happy to oblige.

self.use_invoked = False

def use(self, env_options, env_config):
self.use_invoked = True

This comment has been minimized.

Copy link
@jgraham

jgraham Feb 15, 2018

Contributor

Presumably this flag should be unset on __exit__. But rather than having a dedicated flag, why not use the fact that env_config is initially unset?

This comment has been minimized.

Copy link
@jugglinmike

jugglinmike Feb 15, 2018

Author Contributor

The thought occurred to me, but I decided an explicit flag would be easier to understand and harder to accidentally break. This is pretty subjective, though, so I'll re-implement as you suggest. Would you like me to unset self.env_config in __exit__?

This comment has been minimized.

Copy link
@jgraham

jgraham Feb 15, 2018

Contributor

I'm happy with either.

@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Feb 15, 2018

Thanks @jgraham! The latest "fixup!" commit has an updated commit message

@jgraham
Copy link
Contributor

jgraham left a comment

OK to land once the comment and error message are fixed

return self

def __enter__(self):
# Because this class implements the context manager protocol, it is

This comment has been minimized.

Copy link
@jgraham

jgraham Feb 15, 2018

Contributor

You just need to change the comment and error message to account for the fact that use doesn't exist.

@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Feb 16, 2018

That was careless of me. Sorry about that!

@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Feb 16, 2018

@jgraham Thank you for the linting fix. Now the the TravisCI build has passed, would you mind merging this patch?

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Feb 16, 2018

Ah, sorry the reason I didn't was that there's a demo file still on this branch and I couldn't remove it with the GitHub UI.

@jugglinmike jugglinmike force-pushed the bocoup:fix-sauce-labs-integration branch from aee9eec to a72d7ed Feb 16, 2018

@jugglinmike

This comment has been minimized.

Copy link
Contributor Author

jugglinmike commented Feb 16, 2018

Got it. I've rebased this branch and removed that commit. (The original version of the branch is available at https://github.com/bocoup/web-platform-tests/tree/fix-sauce-labs-integration-prebase for posterity.)

Correct integration with Sauce Labs service
A recently merged patch centralized configuration values which defined
the domain names available during test execution [1]. That patch was
based on a mistaken assumption regarding the location of that value and
the stage of the test execution at which it would be available.
(Specifically: the value is not present in the environment configuration
object when the "environment extras" are initialized.) This interfered
with the initialization of the "environment" for the Sauce Labs service,
making it impossible to run the tests there.

Defer the retrieval of the environment configuration object to the time
that object's context manager is activated. Update the call sites to
provide the environment configuration at this moment. Also update the
signature of the context manager for the `FontInstaller` "environment
extra" to accept this second parameter.

[1] #8614

@jugglinmike jugglinmike force-pushed the bocoup:fix-sauce-labs-integration branch from a72d7ed to 76ab8c6 Feb 16, 2018

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.