Bug 105698 – sheriff-bot should know who the gardeners/sheriffs are

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

sheriff-bot should know who the gardeners/sheriffs are

(and expose that knowledge through some command.)

I believe this information is accessble somewhere (in json?) from one of Chromium's servers.

I'm told Qt/Gtk, etc. have sheriffs, too so it's possible we eventually want to make this generic enough to report those too.  (That seems like a v2 feature though.)

[Alan Cutter]

Found URL for webkit sheriff listing: http://build.chromium.org/p/chromium.webkit/sheriff_webkit.js

[Eric Seidel (no email)]

No wonder the main buildbot pages inline this stuff server-side.  waiting on a synchronous script to document.write is a very slow way to write a webpage. :)

[Alan Cutter]

Created attachment 180998 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 180998 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180998&action=review

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:246
> +        if len(sheriffs.split()) == 1:
> +            return "%s: The current webkit sheriff is: %s" % (nick, sheriffs)
> +        return "%s: The current webkit sheriffs are: %s" % (nick, sheriffs)

I believe this will print using array syntax, e.g.  ['eric', 'alan']

[Eric Seidel (no email)]

Comment on attachment 180998 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180998&action=review

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:41
> +webkit_sheriff_url = "http://build.chromium.org/p/chromium.webkit/sheriff_webkit.js"

I think these normally go in config.urls.

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:216
> +class Current(IRCCommand):

I don't think current is going to be a good name for this.

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:217
> +    def _retrieve_webkit_sheriffs(self, url):

I suspect this function will eventually move to some separate object which knows how to get all the sherriffs from all the various places.  But for now, I think it makes sense as part of this command as you've done. :)

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:223
> +        try:
> +            sheriff_js = Web().get_binary(url, True)
> +        except:
> +            return None
> +        if sheriff_js == None:
> +            return None

I think the Host provides a "browser" object, which is a Mechanize Browser(), and probably strictly superior to "Web", which we designed before we knew about Mechanize. :)

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:232
> +        if match == None:
> +            return None
> +
> +        try:
> +            return match.group(1)
> +        except:
> +            return None

This none check could be skipped and the try/except would catch that case.

>> Tools/Scripts/webkitpy/tool/bot/irc_command.py:246
>> +        return "%s: The current webkit sheriffs are: %s" % (nick, sheriffs)
> 
> I believe this will print using array syntax, e.g.  ['eric', 'alan']

I also think you should note that these are "chromium webkit sheriffs", leaving room for gtk sheriffs, etc in the future.

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:277
> +    "current": Current,

I might name this "sheriffs" instead of "current".

> Tools/Scripts/webkitpy/tool/bot/irc_command_unittest.py:64
> +        url = "file://" + os.path.abspath("./webkitpy/tool/bot/testdata/webkit_sheriff_0.js")

I would have written a helper function here.  _sherrif_test_data_url('0') or some such.

> Tools/Scripts/webkitpy/tool/bot/irc_command_unittest.py:66
> +                         current.execute("tom", [url], None, None))

Having to pass a url to the execute function is a symptom of the get-the-sherrifs function wanting to be a separate object, which would be mocked for testing of the printing, and tested separately for behavior.  Again, something which will be done later when someone needs/wants to get the sherriffs in some other part of webkitpy (as I suspect we may want for when we create rollout bugs for sherrifbot? or when we start auto-filing flaky tests again).

> Tools/Scripts/webkitpy/tool/bot/irc_command_unittest.py:71
> +        self.assertEqual("tom: The current webkit sheriffs are: test_user_1, test_user_2",

I guess I was wrong above.  FYI, there is also a http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/tool/grammar.py, which used to be part of "common" until dpranke pointed out that it was impossible to implement such an api correctly. :)

[Eric Seidel (no email)]

I think the only change you really shoudl make is movign the url into the urls module.  The rest I mentioned above are really just nits.

When abarth is back from vacation tomorrow, he may have more thoughts (he's worked on sheriffbot more than I have as of late), but I suspect he will be burried in a mountain of email and not care too much one way or another. :)

I'm ready to r+ an updated patch.

[Eric Seidel (no email)]

I guess the other change I feel somewhat strongly about is having a name other than "current", and indicating that these are chromium webkit sheriffs.  (Which invites other ports to list their sheriffs here too.)

[Marc-Antoine Ruel]

I don't have opinion on the patch itself but Eric raises a valid concern, the file should be eventually converted to a proper json file.

[Eric Seidel (no email)]

(In reply to comment #8)
> I don't have opinion on the patch itself but Eric raises a valid concern, the file should be eventually converted to a proper json file.

I believe he's replying to comment 2. :)

[Marc-Antoine Ruel]

Exact, sorry for the lack of reference.

[Alan Cutter]

Created attachment 181006 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 181006 [details]
Patch

LGTM.  You can restart sheriffbot by telling it "restart" in #webkit.

[Alan Cutter]

(In reply to comment #5)
> (From update of attachment 180998 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=180998&action=review
> 
> > Tools/Scripts/webkitpy/tool/bot/irc_command.py:41
> > +webkit_sheriff_url = "http://build.chromium.org/p/chromium.webkit/sheriff_webkit.js"
> 
> I think these normally go in config.urls.
> 

Moved URL into config.urls.

> > Tools/Scripts/webkitpy/tool/bot/irc_command.py:216
> > +class Current(IRCCommand):
> 
> I don't think current is going to be a good name for this.
> 

Agreed, renamed as "sheriffs" as suggested.

> > Tools/Scripts/webkitpy/tool/bot/irc_command.py:232
> > +        if match == None:
> > +            return None
> > +
> > +        try:
> > +            return match.group(1)
> > +        except:
> > +            return None
> 
> This none check could be skipped and the try/except would catch that case.
> 

Updated as suggested.

> >> Tools/Scripts/webkitpy/tool/bot/irc_command.py:246
> >> +        return "%s: The current webkit sheriffs are: %s" % (nick, sheriffs)
> > 
> > I believe this will print using array syntax, e.g.  ['eric', 'alan']
> 
> I also think you should note that these are "chromium webkit sheriffs", leaving room for gtk sheriffs, etc in the future.
> 

Renamed to "Chromium Webkit sheriff".

> > Tools/Scripts/webkitpy/tool/bot/irc_command_unittest.py:64
> > +        url = "file://" + os.path.abspath("./webkitpy/tool/bot/testdata/webkit_sheriff_0.js")
> 
> I would have written a helper function here.  _sherrif_test_data_url('0') or some such.
> 
> > Tools/Scripts/webkitpy/tool/bot/irc_command_unittest.py:66
> > +                         current.execute("tom", [url], None, None))
> 
> Having to pass a url to the execute function is a symptom of the get-the-sherrifs function wanting to be a separate object, which would be mocked for testing of the printing, and tested separately for behavior.  Again, something which will be done later when someone needs/wants to get the sherriffs in some other part of webkitpy (as I suspect we may want for when we create rollout bugs for sherrifbot? or when we start auto-filing flaky tests again).
> 

Added helper function.

[WebKit Review Bot]

Comment on attachment 181006 [details]
Patch

Clearing flags on attachment: 181006

Committed r138603: <http://trac.webkit.org/changeset/138603>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

FYI: It made 2 webkitpy tests fail:

Checking autoinstalled packages ...                                   Checking imports ...                    Finding the individual test methods ...                                       Running the tests ...                     [15/1617] webkitpy.common.checkout.changelog_unittest.ChangeLogTest.test_fuzzy_reviewer_match_george_staikos passed                                                                                                                   [41/1617] webkitpy.common.checkout.changelog_unittest.ChangeLogTest.test_fuzzy_reviewer_match_adam_barth passed                                                                                                               [98/1617] webkitpy.common.net.buildbot.buildbot_unittest.BuildBotTest.test_builder_with_name passed1                                                                                                   [166/1617] webkitpy.common.system.executive_unittest.ExecutiveTest.test_popen_args passed1                                                                                         [269/1617] webkitpy.common.system.executive_unittest.ExecutiveTest.test_run_command_args_type passed                                                                                                    [347/1617] webkitpy.layout_tests.models.test_expectations_unittest.MiscTests.test_get_test_set passed                                                                                                     [387/1617] webkitpy.layout_tests.models.test_expectations_unittest.TestExpectationSerializationTests.test_parsed_to_string passed                                                                                                                                 [637/1617] webkitpy.layout_tests.port.chromium_win_unittest.ChromiumWinTest.test_check_build (+2)                                                                                                 [692/1617] webkitpy.layout_tests.port.chromium_win_unittest.ChromiumWinTest.test_path_to_test_expectations_file (+4)                                                                                                                    [806/1617] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_dryrun (+5)                                                                                          [907/1617] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_batch_size (+5)                                                                                              [927/1617] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_natural_order passed                                                                                                   [939/1617] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_output_diffs passed                                                                                                  [952/1617] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_repeat_each_iterations_num_tests passed                                                                                                                      [956/1617] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_run_part passed                                                                                              [964/1617] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_run_chunk passed                                                                                               [970/1617] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_threaded passed                                                                                              [1000/1617] webkitpy.performance_tests.perftest_unittest.MainTest.test_parse_output_with_subtests passed                                                                                                        [1062/1617] webkitpy.performance_tests.perftestsrunner_unittest.MainTest.test_run_with_bad_slave_config_json passed                                                                                                                   [1105/1617] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_hung_thread (+7)                                                                                                [1120/1617] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_hung_thread (+7)                                                                                                [1194/1617] webkitpy.style.checkers.cpp_unittest.CppStyleTest.test_runtime_rtti passed                                                                                      [1268/1617] webkitpy.style.checkers.cpp_unittest.WebKitStyleTest.test_spacing passed                                                                                    [1354/1617] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_hung_thread (+7)                                                                                                [1420/1617] webkitpy.tool.bot.irc_command_unittest.IRCCommandTest.test_sheriffs failed:
  Traceback (most recent call last):
    File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/tool/bot/irc_command_unittest.py", line 69, in test_sheriffs
      sheriffs.execute("tom", [self._sheriff_test_data_url("0")], None, None))
  AssertionError: 'tom: There are no Chromium Webkit sheriffs currently assigned.' != 'tom: Failed to parse URL: file:///ramdisk/qt-linux-release/build/webkitpy/tool/bot/testdata/webkit_sheriff_0.js'
  
[1426/1617] webkitpy.tool.bot.ircbot_unittest.IRCBotTest.test_help failed:
  Traceback (most recent call last):
    File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/tool/bot/ircbot_unittest.py", line 92, in test_help
      OutputCapture().assert_outputs(self, run, args=["help"], expected_logs=expected_logs)
    File "/ramdisk/qt-linux-release/build/Tools/Scripts/webkitpy/common/system/outputcapture.py", line 100, in assert_outputs
      testcase.assertEqual(logs_string, expected_logs)
  AssertionError: 'MOCK: irc.post: mock_nick: Available commands: create-bug, help, hi, restart, roll-chromium-deps, rollout, sheriffs, whois\n' != 'MOCK: irc.post: mock_nick: Available commands: create-bug, help, hi, restart, roll-chromium-deps, rollout, whois\n'
  
[1479/1617] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_hung_thread (+6)                                                                                                [1511/1617] webkitpy.tool.commands.rebaseline_unittest.TestRebaselineTest.test_rebaseline_and_copy_no_overwrite_test passed                                                                                                                           [1516/1617] webkitpy.tool.commands.rebaseline_unittest.TestRebaselineTest.test_rebaseline_test passed                                                                                                     [1555/1617] webkitpy.tool.commands.download_unittest.DownloadCommandsTest.test_land_red_builders passed                                                                                                       [1561/1617] webkitpy.layout_tests.run_webkit_tests_integrationtest.RunTest.test_hung_thread passed                                                                                                  [1562/1617] webkitpy.tool.servers.rebaselineserver_unittest.RebaselineTestTest.test_text_rebaseline_move_no_op_1 passed                                                                                                                       [1578/1617] webkitpy.tool.steps.preparechangelog_unittest.PrepareChangeLogTest.test_fuzzy_reviewer_match_george_staikos passed                                                                                                                              [1592/1617] webkitpy.tool.steps.steps_unittest.StepsTest.test_post_diff passed                                                                              [1609/1617] webkitpy.tool.steps.preparechangelog_unittest.PrepareChangeLogTest.test_fuzzy_reviewer_match_initial passed                                                                                                                       [1611/1617] webkitpy.tool.servers.rebaselineserver_unittest.RebaselineTestTest.test_text_rebaseline_update passed                                                                                                                 [1615/1617] webkitpy.common.system.executive_unittest.ExecutiveTest.serial_test_run_in_parallel passed                                                                                                      Ran 1617 tests in 4.592s
FAILED (failures=2, errors=0)

[Csaba Osztrogonác]

ouch, sorry for the unreadable log, crazy test-webkitpy output with long long lines :-/

[Chris Dumez]

I filed Bug 105913 to track the regression and attached a patch there.

[Dirk Pranke]

Comment on attachment 181006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181006&action=review

> Tools/Scripts/webkitpy/tool/bot/irc_command.py:147
> +            sheriff_js = Web().get_binary(url, True)

rather than creating a Web() object, you should've used the web member of the tool object passed to you in execute(). Then you could've used tests that had a mocked out web object, rather than fetching test files. Generally speaking, we prefer tests that are self-contained with inline data; using the test files means hitting the filesystem which is slow and a bit more cumbersome.

> Tools/Scripts/webkitpy/tool/bot/testdata/webkit_sheriff_zero.js:1
> +document.write('');

I don't this file is actually being used, is it?

[Alan Cutter]

(In reply to comment #19)
> (From update of attachment 181006 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=181006&action=review
> 
> > Tools/Scripts/webkitpy/tool/bot/irc_command.py:147
> > +            sheriff_js = Web().get_binary(url, True)
> 
> rather than creating a Web() object, you should've used the web member of the tool object passed to you in execute(). Then you could've used tests that had a mocked out web object, rather than fetching test files. Generally speaking, we prefer tests that are self-contained with inline data; using the test files means hitting the filesystem which is slow and a bit more cumbersome.
> 
> > Tools/Scripts/webkitpy/tool/bot/testdata/webkit_sheriff_zero.js:1
> > +document.write('');
> 
> I don't this file is actually being used, is it?

Created bug 108262 to address these issues.