[webkit-reviews] review granted: [Bug 64922] Add a way to edit test expectations. : [Attachment 102710] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has granted Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 64922: Add a way to edit test expectations.
https://bugs.webkit.org/show_bug.cgi?id=64922

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=102710&action=review


> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:588
> +class BugManager:
> +    """A simple interface for managing bugs from TestExpectationsEditor."""
> +    def close_bug(self, bug_id, reference_bug_id=None):
> +	   pass
> +
> +    def create_bug(self):
> +	   return 'BUG_NEWLY_CREATED'

Is this meant to be an interface?  If so, you should consider raising
NotImplemented exceptions (e.g., like
http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/bot/queueengin
e.py#L46 ).

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:591
> +class TestExpectationsEditor:

We use new-style classes that derive from object:

class TestExpectationsEditor(object):

I bet this is a common mistake in this code.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:597
> +    def __init__(self, expectation_lines, bug_manager=None):
> +	   self._bug_manager = bug_manager or BugManager()

I'd skip this line and either require the argument or handle the case when it's
None.

> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:633
> +		   if not expectation_line.matching_configurations:
> +		      
self._bug_manager.close_bug(expectation_line.parsed_bug_modifier)

I'm not 100% sure this class should be effecting the bug database.  Maybe it
should return a list of bugs to close?	Although, maybe this is a nice design. 
I'm not sure.