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

Make the normalized wptserve config a class #10231

Merged
merged 1 commit into from Apr 9, 2018

Conversation

Projects
None yet
4 participants
@gsnedders
Copy link
Contributor

gsnedders commented Mar 29, 2018

This should make it easier once we have static typing to make sure we don't end up with non-normalized configs anywhere. This is essentially a subset of #10166, so this can land faster. (Though hopefully that'll land soon too.)


This change is Reviewable

@gsnedders gsnedders requested a review from jgraham Mar 29, 2018

@wpt-pr-bot wpt-pr-bot added the infra label Mar 29, 2018

@w3c-bots

This comment has been minimized.

Copy link

w3c-bots commented Mar 29, 2018

Build PASSED

Started: 2018-04-05 00:36:30
Finished: 2018-04-05 00:49:57

View more information about this build on:

@gsnedders gsnedders force-pushed the gsnedders:config-class branch from 9602bee to 2ec36a9 Mar 29, 2018

@gsnedders gsnedders force-pushed the gsnedders:config-class branch 2 times, most recently from 3dc6cbf to 47c97a4 Apr 4, 2018

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Apr 5, 2018

Reviewed 7 of 7 files at r1, 1 of 1 files at r2.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


tools/serve/config.py, line 14 at r2 (raw file):

with open(os.path.join(repo_root, "config.default.json"), "rb") as _fp:

I'm not thrilled by having this IO at the top level. Put it on the class and cache the result.


tools/serve/config.py, line 41 at r2 (raw file):

    def __init__(self, logger=None, **kwargs):
        if logger is None:

I don't think a config class should eb setting up logging.


tools/serve/config.py, line 65 at r2 (raw file):

        if kwargs:
            k = next(iter(kwargs))
            raise TypeError("__init__() got an unexpected keyword argument '%s'" % k)

I would provide the full list of unexpected arguments (i.e. kwargs.keys() even though it's less like the built in message.


tools/serve/config.py, line 74 at r2 (raw file):

    def __iter__(self):
        return iter([x for x in dir(self) if not x.startswith("_")])

Isn't this also going to return logger? It seems pretty fragile. One option would be to reverse the setup so the config is still stored in a dict and we override the attribute getters and setters to use that dict if nothing is present on the class.


tools/serve/config.py, line 79 at r2 (raw file):

        return len([x for x in dir(self) if not x.startswith("_")])

    def load_overrides(self, override):

This could be called update


tools/serve/config.py, line 116 at r2 (raw file):

                    port = None
                if port == "auto":
                    port = serve.get_port()

Is this actually giving a consistent answer back if it's called multiple times?


tools/serve/config.py, line 156 at r2 (raw file):

    def domains(self):
        domains = {k: ".".join(v)
                   for k, v in serve.get_subdomains(self.browser_host).iteritems()}

I wonder whehter it makes sense to make these functions part of the config object.


tools/serve/config.py, line 173 at r2 (raw file):

    @property
    def domains_set(self):

I don't love having a separate property just to perform a type conversion.


Comments from Reviewable

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Apr 7, 2018

I'm increasingly convinced that this actually belongs in wptserve and not in serve? wptserve relies on the config, so it should probably live there rather than in serve.

However, we currently have a few things that rely on serve:

  • serve.get_subdomains (and get_not_subdomains), but these are easily moved to config.py and made private (there's no need for them to be public, I don't think?)
  • serve.get_port, which is used both by the config to return the generated ports and by serve when checking the hosts are set up and when starting the stash server. The latter of these could potentially be moved into the config as a default "auto" port, but that still leaves the former?

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


tools/serve/config.py, line 41 at r2 (raw file):

Previously, jgraham wrote…

I don't think a config class should eb setting up logging.

So where should we get the logger from before running __init__, given it depends on configuration options (log_level) and we want to log if you use a deprecated config option?


tools/serve/config.py, line 116 at r2 (raw file):

Previously, jgraham wrote…

Is this actually giving a consistent answer back if it's called multiple times?

No. 🙁


tools/serve/config.py, line 156 at r2 (raw file):

Previously, jgraham wrote…

I wonder whehter it makes sense to make these functions part of the config object.

IMO, yes. Where else does it make sense to put them?


tools/serve/config.py, line 173 at r2 (raw file):

Previously, jgraham wrote…

I don't love having a separate property just to perform a type conversion.

To be fair, they originally made more sense on the alternate-hosts branch, but I think now don't? So yeah, should probably go.


Comments from Reviewable

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Apr 8, 2018

Review status: all files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.


tools/serve/config.py, line 41 at r2 (raw file):

Previously, gsnedders (Geoffrey Sneddon) wrote…

So where should we get the logger from before running __init__, given it depends on configuration options (log_level) and we want to log if you use a deprecated config option?

OK, I guess. It still seems weird for something that looks like a pure view of data to set up logging, but I guess that makes sense.


tools/serve/config.py, line 156 at r2 (raw file):

Previously, gsnedders (Geoffrey Sneddon) wrote…

IMO, yes. Where else does it make sense to put them?

I mean serve.get_subdomains &c. Although I'm not sure what actually works well here; as you say this class feels like it belongs in wptserve. Possibly they should be passed in since presumably they get passed to the server as well.


Comments from Reviewable

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Apr 9, 2018

Review status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.


tools/serve/config.py, line 42 at r2 (raw file):

    def __init__(self, logger=None, **kwargs):
        if logger is None:
            logger = logging.getLogger("web-platform-tests")

@jgraham You mentioned on IRC you didn't like this, though I justified why. I think there's three other options: one is we make every warning into a (fatal) exception with no logger, another is we make everything into a Python warning (which IMO makes little sense), and the final is to just silently ignore the warning. I think I'm happy with the first or last? It does mean that usage of the Config class will need to create their own Logger, reading the object themself.


tools/serve/config.py, line 116 at r2 (raw file):

Previously, gsnedders (Geoffrey Sneddon) wrote…

No. 🙁

Also moving the class to wptserve means we add a dependency to serve for get_port, or perhaps more sensibly move all of the get_port stuff to somewhere in wptserve (where, though?).


tools/serve/config.py, line 156 at r2 (raw file):

Previously, jgraham wrote…

I mean serve.get_subdomains &c. Although I'm not sure what actually works well here; as you say this class feels like it belongs in wptserve. Possibly they should be passed in since presumably they get passed to the server as well.

I'm unconvinced by things that are essentially "get this computed config value", which get_subdomains is. My feeling is we have a subdomains and not_subdomains config option that serve sets for us (though nothing actually ever reads it; everything always just reads domains, but that's not unique—there's other places where the computed value is always the one used).


Comments from Reviewable

@gsnedders gsnedders force-pushed the gsnedders:config-class branch from 47c97a4 to ce02f62 Apr 9, 2018

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Apr 9, 2018

Review status: 3 of 9 files reviewed at latest revision, 8 unresolved discussions, some commit checks pending.


tools/wptserve/tests/test_config.py, line 26 at r3 (raw file):

    ({"a": {"b": 1}}, {"a": {"b": 2, "c": 3}}, {"a": {"b": 2}}),
    pytest.param({"a": {"b": 1}}, {"a": 2}, {"a": 1}, marks=pytest.mark.xfail),
    pytest.param({"a": 1}, {"a": {"b": 2}}, {"a": 1}, marks=pytest.mark.xfail),

FYI: these xfails already exist on master, we just don't have tests for it before.


Comments from Reviewable

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Apr 9, 2018

Now in wptserve.


Review status: 3 of 9 files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.


tools/serve/serve.py, line 380 at r3 (raw file):

        if not bad_port(port):
            break
    logger.debug("Going to use port %s" % port)

Note moving this to wptserve has lost this logging (because wptserve.utils doesn't have a global logger). We should probably move this elsewhere and give a better message in general (as to what we're going to use the port for!).


tools/serve/config.py, line 74 at r2 (raw file):

Previously, jgraham wrote…

Isn't this also going to return logger? It seems pretty fragile. One option would be to reverse the setup so the config is still stored in a dict and we override the attribute getters and setters to use that dict if nothing is present on the class.

If we want to view logger as a config item (given it's used by various things), then surely it should be present here.


tools/serve/config.py, line 79 at r2 (raw file):

Previously, jgraham wrote…

This could be called update

Done.


Comments from Reviewable

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Apr 9, 2018

Reviewed 4 of 10 files at r3, 2 of 3 files at r4.
Review status: all files reviewed at latest revision, 9 unresolved discussions, some commit checks pending.


tools/serve/serve.py, line 581 at r4 (raw file):

_not_subdomains = {u"nonexistent-origin"}

def set_serve_config(config):

Why don't we just pass these in through the constructor?


tools/serve/config.py, line 156 at r2 (raw file):

Previously, gsnedders (Geoffrey Sneddon) wrote…

I'm unconvinced by things that are essentially "get this computed config value", which get_subdomains is. My feeling is we have a subdomains and not_subdomains config option that serve sets for us (though nothing actually ever reads it; everything always just reads domains, but that's not unique—there's other places where the computed value is always the one used).

This is fine now subdomains are stored on this object.


Comments from Reviewable

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Apr 9, 2018

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.


tools/serve/serve.py, line 581 at r4 (raw file):

Previously, jgraham wrote…

Why don't we just pass these in through the constructor?

Because we also construct it in wptrunner.environment.TestEnvironment.load_config; we don't necessarily get the config from serve.load_config.


tools/serve/config.py, line 65 at r2 (raw file):

Previously, jgraham wrote…

I would provide the full list of unexpected arguments (i.e. kwargs.keys() even though it's less like the built in message.

Done.


tools/serve/config.py, line 116 at r2 (raw file):

Previously, gsnedders (Geoffrey Sneddon) wrote…

Also moving the class to wptserve means we add a dependency to serve for get_port, or perhaps more sensibly move all of the get_port stuff to somewhere in wptserve (where, though?).

Done.


Comments from Reviewable

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Apr 9, 2018

Review status: 7 of 9 files reviewed at latest revision, 10 unresolved discussions, some commit checks pending.


tools/serve/config.py, line 14 at r2 (raw file):

Previously, jgraham wrote…

I'm not thrilled by having this IO at the top level. Put it on the class and cache the result.

Done.


Comments from Reviewable

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Apr 9, 2018

Review status: 7 of 9 files reviewed at latest revision, 10 unresolved discussions, some commit checks pending.


tools/serve/config.py, line 173 at r2 (raw file):

Previously, gsnedders (Geoffrey Sneddon) wrote…

To be fair, they originally made more sense on the alternate-hosts branch, but I think now don't? So yeah, should probably go.

Done.


Comments from Reviewable

@gsnedders

This comment has been minimized.

Copy link
Contributor Author

gsnedders commented Apr 9, 2018

Review status: 4 of 9 files reviewed at latest revision, 2 unresolved discussions, some commit checks pending.


tools/serve/serve.py, line 581 at r4 (raw file):

Previously, gsnedders (Geoffrey Sneddon) wrote…

Because we also construct it in wptrunner.environment.TestEnvironment.load_config; we don't necessarily get the config from serve.load_config.

Done. Moved everything to use serve.Config which is a subclass of wptserve.config.Config which sets all the serve config correctly.


Comments from Reviewable

@gsnedders gsnedders force-pushed the gsnedders:config-class branch from 53251bb to af67827 Apr 9, 2018

@jgraham

This comment has been minimized.

Copy link
Contributor

jgraham commented Apr 9, 2018

Reviewed 4 of 6 files at r5, 2 of 2 files at r6.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


Comments from Reviewable

@jgraham

jgraham approved these changes Apr 9, 2018

@gsnedders gsnedders merged commit bfef1f2 into web-platform-tests:master Apr 9, 2018

1 check passed

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

@gsnedders gsnedders deleted the gsnedders:config-class branch Apr 9, 2018

Hexcles added a commit that referenced this pull request Apr 27, 2018

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.