[webkit-reviews] review granted: [Bug 47872] CC authors of flaky tests when the commit-queue hits a flaky test : [Attachment 71118] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Oct 18 19:35:33 PDT 2010
Eric Seidel <eric at webkit.org> has granted Adam Barth <abarth at webkit.org>'s
request for review:
Bug 47872: CC authors of flaky tests when the commit-queue hits a flaky test
https://bugs.webkit.org/show_bug.cgi?id=47872
Attachment 71118: Patch
https://bugs.webkit.org/attachment.cgi?id=71118&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71118&action=review
Looks OK. Please consider my comments below.
> WebKitTools/Scripts/webkitpy/common/checkout/api.py:118
> + def recent_commit_infos_for_files(self, paths):
Eventually we'll want some sort of lookback_limit=5 argument here.
> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:295
> + test_paths = map(path_for_layout_test, flaky_tests)
> + commit_infos =
self._tool.checkout().recent_commit_infos_for_files(test_paths)
> + authors = [commit_info.author().bugzilla_email() for commit_info in
commit_infos if commit_info.author()]
I would have made this a helper function. authors =
self._author_emails_for_tests(flaky_tests)
That would have cleaned up this function a little.
> WebKitTools/Scripts/webkitpy/tool/commands/queues.py:298
> + cc_explaination = " The author(s) of the test(s) have been CCed
on this bug."
I'm not sure this really needs to be conditional, but I guess it's nice that it
is. Is this condition tested? Why not use a ternary to set this?
More information about the webkit-reviews
mailing list