Bug 64922 - Add a way to edit test expectations.
Summary: Add a way to edit test expectations.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dimitri Glazkov (Google)
URL:
Keywords:
Depends on:
Blocks: 64184
  Show dependency treegraph
 
Reported: 2011-07-20 20:52 PDT by Dimitri Glazkov (Google)
Modified: 2011-08-03 08:33 PDT (History)
2 users (show)

See Also:


Attachments
WIP: Still lots to do. (11.82 KB, patch)
2011-07-20 21:22 PDT, Dimitri Glazkov (Google)
no flags Details | Formatted Diff | Diff
Patch (30.98 KB, patch)
2011-08-02 16:15 PDT, Dimitri Glazkov (Google)
abarth: review+
abarth: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dimitri Glazkov (Google) 2011-07-20 20:52:30 PDT
Add a way to edit test expectations.
Comment 1 Dimitri Glazkov (Google) 2011-07-20 21:22:46 PDT
Created attachment 101552 [details]
WIP: Still lots to do.
Comment 2 Dimitri Glazkov (Google) 2011-08-02 16:15:36 PDT
Created attachment 102710 [details]
Patch
Comment 3 WebKit Review Bot 2011-08-02 16:18:40 PDT
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 4 Adam Barth 2011-08-02 16:22:29 PDT
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 5 Dimitri Glazkov (Google) 2011-08-03 08:09:31 PDT
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.
Comment 6 Dimitri Glazkov (Google) 2011-08-03 08:33:46 PDT
Committed r92282: <http://trac.webkit.org/changeset/92282>