Bug 46776 – new-run-webkit-tests - clean up port/factory, add abstraction for the source tree

• Component: New Bugs
• Status: RESOLVED WONTFIX

[Dirk Pranke]

new-run-webkit-tests - clean up port/factory, add abstraction for the sourc e tree

[Dirk Pranke]

Created attachment 69154 [details]
Patch

[Dirk Pranke]

Created attachment 69156 [details]
Patch

[Dirk Pranke]

Note that this patch contains the "better" fixes for the default configuration bug listed in bug 46278 (that had patches landed in r68268 and r68365.

[Tony Chang]

Comment on attachment 69156 [details]
Patch

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

> WebKitTools/Scripts/webkitpy/layout_tests/port/options.py:38
> +    def __init__(self, **kwargs):

Is it possible to subclass the options class in optparse?  What's wrong with just keeping a reference to the original optparse result?

[Tony Chang]

Hit the wrong button:

I was going to add:
This patch is large and hard to review.  Can you put the PortOptions changes into a stand alone patch first?

[Dirk Pranke]

(In reply to comment #4)
> (From update of attachment 69156 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69156&action=review
> 
> > WebKitTools/Scripts/webkitpy/layout_tests/port/options.py:38
> > +    def __init__(self, **kwargs):
> 
> Is it possible to subclass the options class in optparse?  What's wrong with just keeping a reference to the original optparse result?

1) I wanted a generic "options" class that didn't depend on the optparse.Values definition specifically, but provided similar functionality for readers. It's almost like a mock fixture, while was still easy to construct. There also didn't seem to be any real value to subclassing that class. 

That said, I'm not all that familiar with the optparse.Values class, so maybe it's a good idea. I'll take another look.

2) I'm not sure I fully understand your second question (just keeping a reference to the original result), but if I do understand it, in the normal case (run_webkit_tests), we do keep the reference. All of this other code is for cases where we didn't actually call optparse to get a Values object.

[Dirk Pranke]

(In reply to comment #5)
> Hit the wrong button:
> 
> I was going to add:
> This patch is large and hard to review.  Can you put the PortOptions changes into a stand alone patch first?

I was afraid this might be the case. I will see if I can break it up into some more manageable chunks. Thanks for giving it a shot first!

[Ojan Vafai]

Comment on attachment 69156 [details]
Patch

r- per comment #7

[Tony Chang]

(In reply to comment #6)
> 2) I'm not sure I fully understand your second question (just keeping a reference to the original result), but if I do understand it, in the normal case (run_webkit_tests), we do keep the reference. All of this other code is for cases where we didn't actually call optparse to get a Values object.

Does that mean PortOptions is only used in unittests?  Should it be called MockOptions?

[Dirk Pranke]

(In reply to comment #9)
> (In reply to comment #6)
> > 2) I'm not sure I fully understand your second question (just keeping a reference to the original result), but if I do understand it, in the normal case (run_webkit_tests), we do keep the reference. All of this other code is for cases where we didn't actually call optparse to get a Values object.
> 
> Does that mean PortOptions is only used in unittests?  

Well, sort of. The "options" argument is part of the Port public interface. Python doesn't have a good way AFAIK to express the concept of  "some object implementing interface X" for me to indicate this.

> Should it be called MockOptions?

As per above, I'm not 100% comfortable with this. I'd be happy for the implementation to be called MockOptions, but I need some name for the actual interface object. Maybe optparse.Values and MockValues would be the way to go.

[Dirk Pranke]

yeesh, getting back to this after two weeks, even I can't follow this patch. Gives me a whole new level of sympathy for trying to follow large patches. 

Closing this; I'll break it up into multiple separate changes.