[webkit-reviews] review granted: [Bug 46703] Move more SheriffBot smarts into FailureMap : [Attachment 69297] Patch

[bugzilla-daemon at webkit.org]

Eric Seidel <eric at webkit.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 46703: Move more SheriffBot smarts into FailureMap
https://bugs.webkit.org/show_bug.cgi?id=46703

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=69297&action=review

In general looks OK.

> WebKitTools/Scripts/webkitpy/common/net/failuremap.py:41
> +	   return self._failures == []

return not self._failures maybe?

That will make None empty too.

> WebKitTools/Scripts/webkitpy/common/net/failuremap.py:45
> +	   return sorted(set(sum([failure_info['regression_window'].revisions()

> +				  for failure_info in self._failures], [])))

Local variables seems useful here.

> WebKitTools/Scripts/webkitpy/common/net/regressionwindow.py:52
> +	   if self._revisions:
> +	       return self._revisions
> +	   self._revisions = range(self._failing_build.revision(),
self._build_before_failure.revision(), -1)
> +	   self._revisions.reverse()
> +	   return self._revisions

I would have reversed this if.

> WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py:61
> +    def _is_old_failure(self):
> +	   return self._tool.status_server.svn_revision(svn_revision)

This can't work.  Seems we're missing a unit test?

> WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py:67
> +	   # FIXME: We need to figure out how to provoke_flaky_builders.

Please file a bug.  If I had known this was broken, I could have fixed it long
ago. :(