[webkit-reviews] review granted: [Bug 47510] new-run-webkit-tests: clean up port option-parsing code : [Attachment 71103] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Mon Oct 18 17:44:33 PDT 2010


Eric Seidel <eric at webkit.org> has granted Dirk Pranke <dpranke at chromium.org>'s
request for review:
Bug 47510: new-run-webkit-tests: clean up port option-parsing code
https://bugs.webkit.org/show_bug.cgi?id=47510

Attachment 71103: Patch
https://bugs.webkit.org/attachment.cgi?id=71103&action=review

------- Additional Comments from Eric Seidel <eric at webkit.org>
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


More information about the webkit-reviews mailing list