Bug 47709 - mocktool.MockOptions shouldn't default values to objects
Summary: mocktool.MockOptions shouldn't default values to objects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Dirk Pranke
URL:
Keywords:
Depends on:
Blocks: 47510
  Show dependency treegraph
 
Reported: 2010-10-14 19:40 PDT by Dirk Pranke
Modified: 2010-10-15 18:31 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.84 KB, patch)
2010-10-14 19:51 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff
Patch (9.26 KB, patch)
2010-10-15 18:27 PDT, Dirk Pranke
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>