Bug 38936 - sheriffbot is spammy.
Summary: sheriffbot is spammy.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-11 14:39 PDT by David Levin
Modified: 2010-05-11 21:37 PDT (History)
3 users (show)

See Also:


Attachments
Patch (8.61 KB, patch)
2010-05-11 18:20 PDT, Adam Barth
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2010-05-11 14:39:16 PDT
Today the tree went red.
Sherriffbot decided to blame everyone for the failure except for the right change which was 59171.

sheriffbot: dave_levin, ojan: http://trac.webkit.org/changeset/59168 might have broken Chromium Win Release
[2:25pm] sheriffbot: abarth, japhet: http://trac.webkit.org/changeset/59169 might have broken Chromium Win Release
[2:25pm] sheriffbot: ggaren: http://trac.webkit.org/changeset/59170 might have broken Chromium Win Release
[2:25pm] sheriffbot: tonikitoo: http://trac.webkit.org/changeset/59165 might have broken Chromium Win Release
[2:25pm] sheriffbot: jianli: http://trac.webkit.org/changeset/59166 might have broken Chromium Win Release

A few possible issues: 
1. Perhaps it would be better to do more of a summary.
2. It is odd that it omitted the right change.
3. Does it keep telling people that they may have broken the build if the build was already broken and didn't turn green yet?
Comment 1 Adam Barth 2010-05-11 16:41:34 PDT
The problem is that SheriffBot detected that 59171 broke Qt Linux Release minimal already.  When Chromium Win Release broke, the blamelist included a bunch of other patches.  In an effort not to spam, it didn't repeat the warning for 59171.

One proposal is to not warn about a blamelist that has some intersection with an already blamed list.  That will reduce false positives in cases like this at the cost of missing some warnings about concurrent failures.
Comment 2 Adam Barth 2010-05-11 18:20:10 PDT
Created attachment 55792 [details]
Patch
Comment 3 Eric Seidel (no email) 2010-05-11 20:54:28 PDT
Comment on attachment 55792 [details]
Patch

WebKitTools/Scripts/webkitpy/tool/commands/sheriffbot.py:64
 +          # been that skippign this check causes a lot of spam for builders that
skippign

Makes sense.

I still think our sheriffbot arch is less than optimal.  I would like him eventually to hold in memory and increasingly good picture of what went wrong.  So that one can ask him "what's broken" and he can tell you the exact revision and what it broke.  Right now he just warns as he gets information and doesn't keep around any bigger picture.

Ideally we should cross-reference builds to figure out what actually started when and hold multiple "break" objects or something in memory.  Then we would warn about them every 30m or something until they got fixed, and could explain all the information we know about them over IRC when asked.
Comment 4 Adam Barth 2010-05-11 21:20:20 PDT
Committed r59204: <http://trac.webkit.org/changeset/59204>
Comment 5 Eric Seidel (no email) 2010-05-11 21:37:01 PDT
Committed r59206: <http://trac.webkit.org/changeset/59206>