Bug 65794 – Wire up updating expectations in garden-o-matic.

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

[Dimitri Glazkov (Google)]

Wire up updating expectations in garden-o-matic.

[Dimitri Glazkov (Google)]

Created attachment 103123 [details]
Patch

[Adam Barth]

Comment on attachment 103123 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103123&action=review

> Tools/Scripts/webkitpy/layout_tests/port/factory.py:40
> +    DEBUG_CONFIGURATION_REGEX = r"[d|D](ebu|b)g"

What's the point of having this be a class-level static?  I'd just keep it as a private variable to __init__

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:35
> +# FIXME: This needs to be properly implemented.
> +class GardeningBugManager(object):

OMG, that's the worst name ever.  Does this class need to inherit from some sort of interface object?  Usually, we'd just have GardeningExpectationsController inherit from that interface and implement these methods.  Haven't a separate class doesn't seem to buy us that much.

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:43
> +class GardeningExpectationsController(object):

This class does not appear to be tested.  Also, I'd call it ExpectationsUpdater.  :)

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:44
> +    def __init__(self, filesystem):

This should probably take a whole tool b/c it's going to need tool.bugs at some point too.  If we were lower down in the dependency graph, we'd want to break out the various parts of the tool, but up here in the "tool" package, we usually just pass around the tool itself.

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:51
> +        self.port = factory.get("chromium-win-win7")
> +        self.bug_manager = GardeningBugManager()
> +        self.converter = TestConfigurationConverter(self.port.all_test_configurations(), self.port.configuration_specifier_macros())
> +        self.parser = TestExpectationParser(self.port, [], allow_rebaseline_modifier=False)

If these are all private, then should start with an _ character.

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:63
> +    def _builder_to_test_config(self, builder_name):
> +        return factory.get_from_builder_name(builder_name).test_configuration()

@memoize.  You're going to be calling this sucker a lot because you call it from the for failure_info in failure_info_list loop.

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:70
> +        self.expectations_controller = GardeningExpectationsController(self.tool.filesystem)

Rather than keeping this on the server, I'd use a memoized function on GardeningHTTPRequestHandler that creates the object.

[Adam Barth]

Comment on attachment 103123 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103123&action=review

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:59
> +            editor.update_expectations(failure_info.testName, self._builder_to_test_config(failure_info.builderName), set(failure_info.failureTypeList))

Doesn't this need to be failure_info['testName'] ?  This isn't JavaScript.  There are rules!

[Dimitri Glazkov (Google)]

Comment on attachment 103123 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=103123&action=review

>> Tools/Scripts/webkitpy/layout_tests/port/factory.py:40
>> +    DEBUG_CONFIGURATION_REGEX = r"[d|D](ebu|b)g"
> 
> What's the point of having this be a class-level static?  I'd just keep it as a private variable to __init__

Fixed.

>> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:35
>> +class GardeningBugManager(object):
> 
> OMG, that's the worst name ever.  Does this class need to inherit from some sort of interface object?  Usually, we'd just have GardeningExpectationsController inherit from that interface and implement these methods.  Haven't a separate class doesn't seem to buy us that much.

Moved to GardeningExpectationsUpdater.

>> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:43
>> +class GardeningExpectationsController(object):
> 
> This class does not appear to be tested.  Also, I'd call it ExpectationsUpdater.  :)

Renamed, adding unit tests now.

>> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:44
>> +    def __init__(self, filesystem):
> 
> This should probably take a whole tool b/c it's going to need tool.bugs at some point too.  If we were lower down in the dependency graph, we'd want to break out the various parts of the tool, but up here in the "tool" package, we usually just pass around the tool itself.

Fixed.

>> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:51
>> +        self.parser = TestExpectationParser(self.port, [], allow_rebaseline_modifier=False)
> 
> If these are all private, then should start with an _ character.

Fixed.

>> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:59
>> +            editor.update_expectations(failure_info.testName, self._builder_to_test_config(failure_info.builderName), set(failure_info.failureTypeList))
> 
> Doesn't this need to be failure_info['testName'] ?  This isn't JavaScript.  There are rules!

Rules are for the weak.

>> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:63
>> +        return factory.get_from_builder_name(builder_name).test_configuration()
> 
> @memoize.  You're going to be calling this sucker a lot because you call it from the for failure_info in failure_info_list loop.

Fixed.

>> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:70
>> +        self.expectations_controller = GardeningExpectationsController(self.tool.filesystem)
> 
> Rather than keeping this on the server, I'd use a memoized function on GardeningHTTPRequestHandler that creates the object.

Ooh, pretty. Done.

[Dimitri Glazkov (Google)]

Created attachment 103276 [details]
Better.

[Adam Barth]

Comment on attachment 103276 [details]
Better.

View in context: https://bugs.webkit.org/attachment.cgi?id=103276&action=review

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:59
> +            expectation_set = set([expectation for expectation in map(TestExpectations.expectation_from_string, failure_info['failureTypeList']) if expectation])

This looks like just map + filter.

> Tools/Scripts/webkitpy/tool/servers/gardeningserver.py:63
> +            editor.update_expectation(failure_info['testName'], set([self._builder_to_test_config(failure_info['builderName'])]), expectation_set)

failure_info['testName'] -> test_name

> Tools/Scripts/webkitpy/tool/servers/gardeningserver_unittest.py:55
> +    @classmethod
> +    def path_to_test_expectations_file(cls):
> +        if not cls._path_to_test_expectations_file:
> +            cls._path_to_test_expectations_file = cls.create().path_to_test_expectations_file()
> +        return cls._path_to_test_expectations_file

This looks like a manual memoize.  Does this really need to be cached?  How much does it slow down the test not to be cached?

[Dimitri Glazkov (Google)]

Committed r92624: <http://trac.webkit.org/changeset/92624>