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.)
Found URL for webkit sheriff listing: http://build.chromium.org/p/chromium.webkit/sheriff_webkit.js
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. :)
Created attachment 180998 [details] Patch
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']
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. :)
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.
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.)
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.
(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. :)
Exact, sorry for the lack of reference.
Created attachment 181006 [details] Patch
Comment on attachment 181006 [details] Patch LGTM. You can restart sheriffbot by telling it "restart" in #webkit.
(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.
Comment on attachment 181006 [details] Patch Clearing flags on attachment: 181006 Committed r138603: <http://trac.webkit.org/changeset/138603>
All reviewed patches have been landed. Closing bug.
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)
ouch, sorry for the unreadable log, crazy test-webkitpy output with long long lines :-/
I filed Bug 105913 to track the regression and attached a patch there.
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?
(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.