Bug 47709

Summary: mocktool.MockOptions shouldn't default values to objects
Product: WebKit Reporter: Dirk Pranke <dpranke>
Component: New BugsAssignee: Dirk Pranke <dpranke>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, cjerdonek, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 47510    
Attachments:
Description Flags
Patch
none
Patch none

Description Dirk Pranke 2010-10-14 19:40:20 PDT
mocktool.MockOptions shouldn't default values to objects
Comment 1 Dirk Pranke 2010-10-14 19:51:30 PDT
Created attachment 70824 [details]
Patch
Comment 2 Dirk Pranke 2010-10-14 20:07:41 PDT
see ChangeLog for motivation ...
Comment 3 Eric Seidel (no email) 2010-10-15 14:09:12 PDT
Comment on attachment 70824 [details]
Patch

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

Fantastic!  Thanks!

> WebKitTools/Scripts/webkitpy/tool/commands/commandtest.py:46
> +        options.blocks = True
> +        options.cc = 'MOCK cc'
> +        options.component = 'MOCK component'
> +        options.confirm = True
> +        options.email = 'MOCK email'
> +        options.git_commit = 'MOCK git commit'
> +        options.obsolete_patches = True
> +        options.open_bug = True
> +        options.port = 'MOCK port'
> +        options.quiet = True
> +        options.reviewer = 'MOCK reviewer'

So this was the minimum list required to get the tests to pass I assume?

> WebKitTools/Scripts/webkitpy/tool/mocktool.py:587
> +class MockOptions(object):
> +    def __init__(self, **kwargs):
> +        for key, value in kwargs.items():
> +            self.__dict__[key] = value

Very slick!

You might want to add a comment here to explain why we don't have any default options here.  Maybe we should?  Or maybe we should have a WebKitPatchMockOptions which has a bunch of global defaults?  (like port = None)
Comment 4 Dirk Pranke 2010-10-15 14:51:39 PDT
(In reply to comment #3)
> (From update of attachment 70824 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=70824&action=review
> 
> Fantastic!  Thanks!
> 
> > WebKitTools/Scripts/webkitpy/tool/commands/commandtest.py:46
> > +        options.blocks = True
> > +        options.cc = 'MOCK cc'
> > +        options.component = 'MOCK component'
> > +        options.confirm = True
> > +        options.email = 'MOCK email'
> > +        options.git_commit = 'MOCK git commit'
> > +        options.obsolete_patches = True
> > +        options.open_bug = True
> > +        options.port = 'MOCK port'
> > +        options.quiet = True
> > +        options.reviewer = 'MOCK reviewer'
> 
> So this was the minimum list required to get the tests to pass I assume?
> 

Yes.

> > WebKitTools/Scripts/webkitpy/tool/mocktool.py:587
> > +class MockOptions(object):
> > +    def __init__(self, **kwargs):
> > +        for key, value in kwargs.items():
> > +            self.__dict__[key] = value
> 
> Very slick!
> 
> You might want to add a comment here to explain why we don't have any default options here.  Maybe we should?  Or maybe we should have a WebKitPatchMockOptions which has a bunch of global defaults?  (like port = None)

I can add a comment. If we're going to reuse MockOptions for different kinds of unit tests (for example, tool and port) then there isn't a single set of defaults that make sense. Various unit tests do have some sets of defaults and wrappers.
Comment 5 Dirk Pranke 2010-10-15 18:27:03 PDT
Created attachment 70940 [details]
Patch
Comment 6 Dirk Pranke 2010-10-15 18:31:28 PDT
Committed r69905: <http://trac.webkit.org/changeset/69905>