Bug 64922 – Add a way to edit test expectations.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dimitri Glazkov (Google)]

Add a way to edit test expectations.

[Dimitri Glazkov (Google)]

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

[Dimitri Glazkov (Google)]

Created attachment 102710 [details]
Patch

[WebKit Review Bot]

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.

[Adam Barth]

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.

[Dimitri Glazkov (Google)]

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.

[Dimitri Glazkov (Google)]

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