Bug 40891

Summary: Python Tests Fail after r61508
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, eric, joepeck, ojan, ossy, tonyg, wsiegrist
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
[PATCH] Fix Tests
none
[PATCH] Fix Test and Update Example Bots List
none
[DIFF] The Difference in the example_bots list
none
[PATCH] Fix Test + Order of Bots on Full Waterfall dglazkov: review+

Description Joseph Pecoraro 2010-06-20 13:04:57 PDT
http://trac.webkit.org/changeset/61508 causes a test failure:

        # This test should probably be updated if the default regexp list changes
        self.assertEquals(buildbot.core_builder_names_regexps, name_regexps)

The test's name_regexps needs to be updated.
Comment 1 Joseph Pecoraro 2010-06-20 13:07:32 PDT
Created attachment 59211 [details]
[PATCH] Fix Tests

Anyone know why this assert is there instead of just using the list directly?
Comment 2 Joseph Pecoraro 2010-06-20 13:11:52 PDT
Its likely that the test should also include new "Chromium" builders in example_builders. I'll see if I can find out the names of these and add them.
Comment 3 Joseph Pecoraro 2010-06-20 13:31:04 PDT
Created attachment 59213 [details]
[PATCH] Fix Test and Update Example Bots List

I couldn't find a simple list of all the bots on build.webkit.org, so
I extracted the names of bots from:
http://build.webkit.org/buildslaves

I ran them through `sort | uniq` and update the example_builders
list. I then sorted the expected results. This does include a few
new Chromium bots which would have made Will's change necessary.
Comment 4 Joseph Pecoraro 2010-06-20 13:35:19 PDT
Created attachment 59214 [details]
[DIFF] The Difference in the example_bots list

Here is the diff of the old list (sorted) an the new list (sorted).
It looks like just the Chromium bots were added.
Comment 5 Csaba Osztrogon√°c 2010-06-20 14:14:06 PDT
I think you shouldn't modify the order of 
the bots, because same order in python file 
as on the buildbot master is more readable.

I prefer a patch like this:

diff --git a/WebKitTools/Scripts/webkitpy/common/net/buildbot_unittest.py b/WebKitTools/Scripts/webkitpy/common/net/buildbot_uni
index 5384321..2742040 100644
--- a/WebKitTools/Scripts/webkitpy/common/net/buildbot_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/common/net/buildbot_unittest.py
@@ -275,6 +275,9 @@ class BuildBotTest(unittest.TestCase):
             {'name': u'Chromium Linux Release', },
             {'name': u'Chromium Mac Release', },
             {'name': u'Chromium Win Release', },
+            {'name': u'Chromium Linux Release (Tests)', },
+            {'name': u'Chromium Mac Release (Tests)', },
+            {'name': u'Chromium Win Release (Tests)', },
             {'name': u'New run-webkit-tests', },
         ]
         name_regexps = [
@@ -285,7 +288,7 @@ class BuildBotTest(unittest.TestCase):
             "Windows.*Build",
             "GTK",
             "Qt",
-            "Chromium",
+            "Chromium.*Release$",
         ]
         expected_builders = [
             {'name': u'Tiger Intel Release', },
Comment 6 Joseph Pecoraro 2010-06-20 14:20:58 PDT
Created attachment 59216 [details]
[PATCH] Fix Test + Order of Bots on Full Waterfall

Took Ossy's suggestion.
Comment 7 Dimitri Glazkov (Google) 2010-06-20 14:22:03 PDT
Comment on attachment 59216 [details]
[PATCH] Fix Test + Order of Bots on Full Waterfall

r=me, except:

WebKitTools/Scripts/webkitpy/common/net/buildbot_unittest.py:291
 +              "Chromium.*Release$",
I already landed that :)
Comment 8 Joseph Pecoraro 2010-06-20 14:31:47 PDT
> WebKitTools/Scripts/webkitpy/common/net/buildbot_unittest.py:291
>  +              "Chromium.*Release$",
> I already landed that :)

I see, in:
http://trac.webkit.org/changeset/61512

Committed r61513
	M	WebKitTools/ChangeLog
	M	WebKitTools/Scripts/webkitpy/common/net/buildbot_unittest.py
r61513 = c3ff3c574713fdc4ffdb4220115e06adfc5599c3 (refs/remotes/trunk)
http://trac.webkit.org/changeset/61513(In reply to comment #7)

Thanks!
Comment 9 William Siegrist 2010-06-20 23:10:11 PDT
Wouldn't a better fix be to only have the list(s) in one location? If you don't want the unit tests to require the buildbot master config, then you can make the buildbot config read the builder data from webkitpy (since the master has access to it already). 

In any case, sorry about the breakage.
Comment 10 Adam Barth 2010-06-21 09:44:17 PDT
Yeah, I think the current system is kind of redundant, but testing usually introduces redundancies into the system.  @eric, what do you think?
Comment 11 Eric Seidel (no email) 2010-06-21 10:10:45 PDT
We've had multiple people trip on these.  But they're also very useful for testing the regexps and making sure the core builder list is actually what you plan it to be. :)  So donno.