Bug 39282 - Slave lost shouldn't be recognized as build failed
Summary: Slave lost shouldn't be recognized as build failed
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on: 40055 40056
  Show dependency treegraph
Reported: 2010-05-18 03:57 PDT by Csaba Osztrogonác
Modified: 2010-06-02 13:55 PDT (History)
2 users (show)

See Also:

proposed patch (1.49 KB, patch)
2010-05-18 03:59 PDT, Csaba Osztrogonác
no flags Details | Formatted Diff | Diff
Patch (3.33 KB, patch)
2010-06-02 08:41 PDT, Csaba Osztrogonác
eric: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2010-05-18 03:57:58 PDT
Unfortunately slave lost is recognized as build failed now,
and sherrifbot bother innocent folks with false positive alarms
on IRC and add comments into bugzilla.

I propose sherrifbot should handle "slave lost" as "build successful".
Comment 1 Csaba Osztrogonác 2010-05-18 03:59:38 PDT
Created attachment 56355 [details]
proposed patch
Comment 2 Eric Seidel (no email) 2010-05-18 04:06:07 PDT
I don't think this is the right fix.  But it's an interesting hack.

For one, we would need to unit test this.

Secondly, I think the right fix would be to crawl back a revision and use that as the current red/green status instead of just assuming "slave-lost" means green.  Although maybe we should just treat lost slaves as green?  Not sure.
Comment 3 Eric Seidel (no email) 2010-06-01 11:30:27 PDT
Comment on attachment 56355 [details]
proposed patch

Please add a FIXME comment.  Something like:

# FIXME: We treat slave lost as green even though it is not to work
# around the Qts bot being on a broken internet connection.
# The real fix is https://bugs.webkit.org/show_bug.cgi?id=37099

But this solution is pragmatic and we should do it.
Comment 4 Csaba Osztrogonác 2010-06-02 05:13:31 PDT
Landed in http://trac.webkit.org/changeset/60559
Comment 5 Csaba Osztrogonác 2010-06-02 05:37:49 PDT
(In reply to comment #4)
> Landed in http://trac.webkit.org/changeset/60559
And rolled-out by http://trac.webkit.org/changeset/60560
because I forgot about unit test and broke the whole world.

Sorry. Fix is coming soon.
Comment 6 Csaba Osztrogonác 2010-06-02 08:41:55 PDT
Created attachment 57659 [details]
Comment 7 Csaba Osztrogonác 2010-06-02 08:46:28 PDT
(In reply to comment #6)
> Created an attachment (id=57659) [details]
> Patch

Here is the fixed patch with unit test for slave lost.
I have to add a "not not", because False or None == None.
Or is there any simple way to fix this problem?

Error message why I had to roll-out my earlier patch:

Traceback (most recent call last):
  File "/Volumes/Data/WebKit-BuildSlave/snowleopard-intel-leaks/build/WebKitTools/Scripts/webkitpy/common/net/buildbot_unittest.py", line 218, in test_status_parsing
    self.assertEquals(builder[key], expected_value, ("Builder %d parse failure for key: %s: Actual='%s' Expected='%s'" % (x, key, builder[key], expected_value)))
AssertionError: Builder 2 parse failure for key: is_green: Actual='None' Expected='False'
Comment 8 Eric Seidel (no email) 2010-06-02 11:44:54 PDT
Comment on attachment 57659 [details]

LGTM.  This seems like the right fix.
Comment 9 Csaba Osztrogonác 2010-06-02 13:55:46 PDT
(In reply to comment #8)
> (From update of attachment 57659 [details])
> LGTM.  This seems like the right fix.
Landed in http://trac.webkit.org/changeset/60575 .