Bug 64922

Created attachment 101552 [details] WIP: Still lots to do.

Created attachment 102710 [details] Patch

Attachment 102710 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1 Tools/Scripts/webkitpy/layout_tests/models/test_expectations_unittest.py:603: whitespace before ']' [pep8/E202] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 102710 [details] Patch 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/queueengine.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.

Comment on attachment 102710 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=102710&action=review >> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:588 >> + 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/queueengine.py#L46 ). It's both an interface and a mock object now. I'll make it an interface and move mock to the test. >> 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. Whoa. I just learned about the new-style classes. Thanks! >> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:597 >> + 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. yep. >> Tools/Scripts/webkitpy/layout_tests/models/test_expectations.py:633 >> + 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. Yeah, this is incremental first step. I am not sure either.

Committed r92282: <http://trac.webkit.org/changeset/92282>