Bug 47510 – new-run-webkit-tests: clean up port option-parsing code

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

new-run-webkit-tests: clean up port option-parsing code

[Dirk Pranke]

Created attachment 70488 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 70488 [details]
Patch

I'm not sure I understand this patch.  Doesn't this just make typos easier?  Since now get_option("fooTYPO") will return None where as options.fooTYPO would have thrown?

[Dirk Pranke]

Created attachment 70825 [details]
Patch

[Dirk Pranke]

Okay, I've considerably revised this patch, and we now use the MockOptions() object in mocktool instead of a new type of dummy object.

This patch can't land before 47709 lands, as it requires that functionality (versions of MockOptions prior to that patch have the wrong default semantics and API).

[Dirk Pranke]

Regarding the "making typos easier" comment, the answer is yes and no.

Normally, optparse defines attributes for every argument that is passed in to make_option(), so if you called make_option('--foo'), options.foo will be defined; if you didn't, then accessing options.foo will raise an error.

So, in that sense, options.foo is slightly safer than get_option('foo').

*However*, the port classes are currently written to not assume any particular set of options were passed to make_option(), so they need to be able to deal with the attribute not being defined. This was not being done everywhere, and this patch at least makes everything consistent.

This is still probably the wrong behavior. Ideally we should be able to pull the list of options needed from the ports and merge them together, and then pass that to optparse (or a mock object). I didn't want to do that in this CL in order to keep too many things from changing at once, but I will make the change in a subsequent CL.

[Eric Seidel (no email)]

Comment on attachment 70825 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70825&action=review

Overall I really like what you're doing here.  I just have a couple concerns (listed below) and worry a bit about deploying get_option too widely (since it has the side effect of making typos easier).

> WebKitTools/ChangeLog:26
> +        Arguably this is the wrong semantics to have, and the Port
> +        classes should be able to assume a default set of
> +        attributes/arguments, but that change will need to wait for a
> +        different CL where we can modify new-run-webkit-tests to pull a
> +        list of arguments from the port factory routines.

Can you explain more what that would look like?  I'm not sure I understand yet.

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:76
> +        if self._options is None:
> +            self._options = mocktool.MockOptions()

Really?  This seems wrong to ever have a Mock implicit in the real code.  Seems the unit tests should have to explicitly pass a Mock, or?  Or if you want the MockOptions behavior here, maybe MockOptions should subclass from some other non-unittest class which this code would use here instead of MockOptions. Does that make sense?

As written this code could end up having unexpected behavior changes if someone changed MockOptions and expected only unit test behavior to change.

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:455
> +    def get_option(self, name, default_value=None):

If get_option is not the right long-term solution (as you seem to imply in your ChangeLog) seems we need a FIXME: here to explain why this (and all callers) should be replace with someething else and what that something else looks like.

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:457
> +            return self._options.__dict__[name]

I don't think this is the right call.  __dict__ seems like the wrong interface.  Maybe getattr(name)?

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:461
> +        self._options.__dict__.setdefault(name, default_value)

Seems strange we're grabbing at __dict__ here.  Are we sure this is the right method?

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:527
> +        if self.get_option('use_apache'):

So why would these sites need to change?  I figured only sites which use an existing hasattr() check would want to move to get_option?

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:30
> +from webkitpy.tool import mocktool

Eventually we probably want to split mocktool out into a module of mocks and have the tool-specific stuff live under tool, and all the other mocks be shared somewhere.

I think (but am not 100% certain) the design with tool/ and layout_tests/ and style/  is to have all the code specific to those commands tucked under those directories, and slowly move more and more code to common.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:46
> +        mock_options = mocktool.MockOptions(accelerated_compositing=None,

This is really slick, btw.  I love the use of kwargs here.

> WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:108
> +        port = ChromiumPortTest.TestLinuxPort(options=mock_options)

Remind me why the Port needs options in its constructor?

[Dirk Pranke]

(In reply to comment #6)
> (From update of attachment 70825 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70825&action=review
> 
> Overall I really like what you're doing here.  I just have a couple concerns (listed below) and worry a bit about deploying get_option too widely (since it has the side effect of making typos easier).
> 
> > WebKitTools/ChangeLog:26
> > +        Arguably this is the wrong semantics to have, and the Port
> > +        classes should be able to assume a default set of
> > +        attributes/arguments, but that change will need to wait for a
> > +        different CL where we can modify new-run-webkit-tests to pull a
> > +        list of arguments from the port factory routines.
> 
> Can you explain more what that would look like?  I'm not sure I understand yet.
> 

Basically the idea is that the port/* package would have a way to fetch all of the supported arguments, much like the tool/step commands do. They'd get merged together, and then instead of a MockOptions object there would some variant of a real optparse options argument that had all and only the arguments defined that should be defined.

> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:76
> > +        if self._options is None:
> > +            self._options = mocktool.MockOptions()
> 
> Really?  This seems wrong to ever have a Mock implicit in the real code.  Seems the unit tests should have to explicitly pass a Mock, or?  Or if you want the MockOptions behavior here, maybe MockOptions should subclass from some other non-unittest class which this code would use here instead of MockOptions. Does that make sense?
> 
> As written this code could end up having unexpected behavior changes if someone changed MockOptions and expected only unit test behavior to change.
>

Yeah, it is a bit weird, which is why I originally had a PortOptions() class. Eventually I decided that since the only current application of this is for unittesting, I was okay with a Mock class.

On the other hand, MockOptions itself should have unittests that document the dependencies, so that the semantics don't change. I think my other patch might have these tests already, but if it doesn't, I'll add them.

> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:455
> > +    def get_option(self, name, default_value=None):
> 
> If get_option is not the right long-term solution (as you seem to imply in your ChangeLog) seems we need a FIXME: here to explain why this (and all callers) should be replace with someething else and what that something else looks like.
>

Will add a FIXME
 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:457
> > +            return self._options.__dict__[name]
> 
> I don't think this is the right call.  __dict__ seems like the wrong interface.  Maybe getattr(name)?
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:461
> > +        self._options.__dict__.setdefault(name, default_value)
> 
> Seems strange we're grabbing at __dict__ here.  Are we sure this is the right method?
>

Good question. No, I"m not sure, and I will double-check.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:527
> > +        if self.get_option('use_apache'):
> 
> So why would these sites need to change?  I figured only sites which use an existing hasattr() check would want to move to get_option?
>

The sites that don't have an existing hasattr() check are broken. This change makes everything consistent.
 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:30
> > +from webkitpy.tool import mocktool
> 
> Eventually we probably want to split mocktool out into a module of mocks and have the tool-specific stuff live under tool, and all the other mocks be shared somewhere.
>
> I think (but am not 100% certain) the design with tool/ and layout_tests/ and style/  is to have all the code specific to those commands tucked under those directories, and slowly move more and more code to common.
> 

Sounds good.

> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_gpu_unittest.py:46
> > +        mock_options = mocktool.MockOptions(accelerated_compositing=None,
> 
> This is really slick, btw.  I love the use of kwargs here.
>

Thanks :)
 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/chromium_unittest.py:108
> > +        port = ChromiumPortTest.TestLinuxPort(options=mock_options)
> 
> Remind me why the Port needs options in its constructor?

I'm not sure I understand what (or why) you're asking this, so I'll give the basic answer - the port implementations need to be know what options were passed on the command line, because they change their behavior based on it. Where else would they get the options from? A set_options() call? (that's certainly possible)

[Dirk Pranke]

Created attachment 70936 [details]
Patch

[Dirk Pranke]

(In reply to comment #7)
> > > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:457
> > > +            return self._options.__dict__[name]
> > 
> > I don't think this is the right call.  __dict__ seems like the wrong interface.  Maybe getattr(name)?
> > 
> > > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:461
> > > +        self._options.__dict__.setdefault(name, default_value)
> > 
> > Seems strange we're grabbing at __dict__ here.  Are we sure this is the right method?
> >
> 
> Good question. No, I"m not sure, and I will double-check.
> 

So, normally, one would use __setattr__ and __getattribute__ instead of accessing __dict__ directly. Unfortunately, optparse.Values doesn't inherit from object (at least not in 2.6), and so those methods aren't defined.

[Dirk Pranke]

Created attachment 70943 [details]
Patch

[Dirk Pranke]

Created attachment 70944 [details]
Patch

[Dirk Pranke]

added unittests for mocktool.MockOptions.

[Eric Seidel (no email)]

Comment on attachment 70944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=70944&action=review

In general looks good.  r- for the mock inclusion and the __dict__ access.  I'd like to know about if all the get_option deploys are necessary or if the goal was consistency, or which. :)

Thanks!

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:50
> +from webkitpy.tool import mocktool

I would rather just copy MockOptions into this file (under a new name) than introduce this dependency.

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:464
> +            return self._options.__dict__[name]

Don't you mean http://docs.python.org/library/functions.html#getattr ?

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:468
> +        self._options.__dict__.setdefault(name, default_value)

This is the same as doing if not hasattr(): setattr().  I don't think we want to grab at __dict__.

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:534
> -        if self._options.use_apache:
> +        if self.get_option('use_apache'):

If get_option is "deprecated" as soon as we add the Port option stuff you mentioned above, then why change these?  Do we hit them in a unit test?

[Dirk Pranke]

(In reply to comment #13)
> (From update of attachment 70944 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70944&action=review
> 
> In general looks good.  r- for the mock inclusion and the __dict__ access.  I'd like to know about if all the get_option deploys are necessary or if the goal was consistency, or which. :)
> 

I answered the get_option() question in comment #7 - it's for consistency and to fix bugs where we wouldn't otherwise do the right thing.

> Thanks!
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:50
> > +from webkitpy.tool import mocktool
> 
> I would rather just copy MockOptions into this file (under a new name) than introduce this dependency.
> 

I'm not sure I understand why you think this. I think it makes the intent clearer (we're providing a mock implementation of a class since we didn't get a real argument), and it reduces code duplication and the chance of bugs. I could add  a comment to this effect. Why do you think duplicating the code is better?

> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:464
> > +            return self._options.__dict__[name]
> 
> Don't you mean http://docs.python.org/library/functions.html#getattr ?
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:468
> > +        self._options.__dict__.setdefault(name, default_value)
> 
> This is the same as doing if not hasattr(): setattr().  I don't think we want to grab at __dict__.
> 

Ah. I had tried _options.__getattr__ , which didn't work. I didn't know about the standalone functions.

> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:534
> > -        if self._options.use_apache:
> > +        if self.get_option('use_apache'):
> 
> If get_option is "deprecated" as soon as we add the Port option stuff you mentioned above, then why change these?  Do we hit them in a unit test?

As answered above, for consistency and to fix what would otherwise be bugs in the current code. I don't want code accessing option values in two different ways without being clear which behavior is intended. The code as written will raise a confusing exception, rather than erroring out cleanly, and it's unlikely that that's the intended behavior (at least, I don't want that behavior).

[Dirk Pranke]

Created attachment 71103 [details]
Patch

[Dirk Pranke]

(In reply to comment #15)
> Created an attachment (id=71103) [details]
> Patch

Note that this does include the cloned MockOptions (renamed, with a comment).

[Eric Seidel (no email)]

Comment on attachment 71103 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71103&action=review

Works for me.  Thanks for the patch.

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:61
> +    """Fake implementation of optparse.Values. Cloned from
> +    webkitpy.tool.mocktool.MockOptions.
> +

It makes me feel warmer and fuzzier inside to know that the main stuff is not depending on the unit tests or mocks. :)  Thank you.

We could make MockOptions inherit from DummyOptions if you liked.  But I think this is also just fine as-is.

To restate my previous concerns with new-words:
- My trouble with the previous design was that non-unittest code was using a class with "Mock" in its name (which we haven't done before).
- It was also bad that non-unittest code was importing from a module named mock, which could end up getting changed, or causing other mock-classes to bleed into the real code.

It would have also been possible to rename MockOptions to some other name, rip it out of the mocks, and use it from both places.

I hope that my added words have made my "design inclinations" clearer for the future. :)

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:91
> +            # FIXME: ideally we'd have a package-wide way to get a

Ideally

> WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:474
> +        # FIXME: eventually we should not have to do a test for

Eventually

[Dirk Pranke]

(In reply to comment #17)
> (From update of attachment 71103 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71103&action=review
> 
> Works for me.  Thanks for the patch.
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/base.py:61
> > +    """Fake implementation of optparse.Values. Cloned from
> > +    webkitpy.tool.mocktool.MockOptions.
> > +
> 
> It makes me feel warmer and fuzzier inside to know that the main stuff is not depending on the unit tests or mocks. :)  Thank you.
> 
> We could make MockOptions inherit from DummyOptions if you liked.  But I think this is also just fine as-is.
> 
> To restate my previous concerns with new-words:
> - My trouble with the previous design was that non-unittest code was using a class with "Mock" in its name (which we haven't done before).
> - It was also bad that non-unittest code was importing from a module named mock, which could end up getting changed, or causing other mock-classes to bleed into the real code.
> 
> It would have also been possible to rename MockOptions to some other name, rip it out of the mocks, and use it from both places.
> 

Yes, I understand your concerns about having real code depend on mock code, but I'm not sure that I think the options are better. I think we would both agree that having a way to get a real options argument would be better yet, so hopefully I'll have a patch for that soon.

Thanks for the feedback and the review!

-- Dirk

[Dirk Pranke]

Created attachment 71108 [details]
Patch

[Dirk Pranke]

(In reply to comment #19)
> Created an attachment (id=71108) [details]
> Patch

This patch is just a merge to current tip-of-tree.

[Dirk Pranke]

Committed r70013: <http://trac.webkit.org/changeset/70013>