Eliza isn't used and the third-party module we rely on for it is hosted on a domain that we have no guarantees won't go down. If that domain was to go offline or the module stopped being hosted there, all webkitpy tests would cease to work on any machines that had not ran them before.
Created attachment 295901 [details] proposed patch
Comment on attachment 295901 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=295901&action=review OK. > Tools/ChangeLog:8 > + This module is not on pypi, so installing it is a challenge. The jokes feel pretty old too! :D
Committed r209216.
Comment on attachment 295901 [details] proposed patch View in context: https://bugs.webkit.org/attachment.cgi?id=295901&action=review I'm confused as to why Eliza was replaced with UnknownCommand... If it's a hotfix, we definitely should have a FIXME. (Adding r+ so my comment doesn't push it back to r?) > Tools/Scripts/webkitpy/thirdparty/__init___unittest.py:44 > + self.buildbot_installed = True Why was this changed from eliza to buildbot? I think all Eliza references should just be removed. > Tools/Scripts/webkitpy/thirdparty/__init___unittest.py:53 > + self.assertTrue(mock_import_hook.buildbot_installed) Ditto. > Tools/Scripts/webkitpy/tool/bot/ircbot.py:57 > + return "%s: %s" % (nick, "...") This doesn't seem like the right approach. Why is Eliza being replaced with UnknownCommand, and why does it return '...'? That doesn't seem user friendly. If we want to do this as simply a "hotfix", there should definitely be a FIXME. > Tools/Scripts/webkitpy/tool/bot/ircbot.py:78 > + command = UnknownCommand Ditto to above. > Tools/Scripts/webkitpy/tool/bot/ircbot_unittest.py:37 > +from webkitpy.tool.bot.ircbot import UnknownCommand Ditto. > Tools/Scripts/webkitpy/tool/bot/ircbot_unittest.py:55 > + self.assertEqual(bot._parse_command_and_args(" "), (UnknownCommand, [""])) Ditto.
> I'm confused as to why Eliza was replaced with UnknownCommand... If it's a hotfix, we definitely should have a FIXME. I don't know what I could write in such a FIXME. What other fix do you have in mind? > Why was this changed from eliza to buildbot? I think all Eliza references should just be removed. This code isn't testing eliza, it is testing that autoinstall hooks work. So, removing it would have removed important test coverage. It doesn't matter which package to test against, eliza must have been picked randomly.
(In reply to comment #5) > > I'm confused as to why Eliza was replaced with UnknownCommand... If it's a hotfix, we definitely should have a FIXME. > > I don't know what I could write in such a FIXME. What other fix do you have > in mind? Maybe I first need to understand why you replaced it with UnknownCommand? It looks like an unnecessary stub that was put in place so minimal changes had to be done to remove Eliza. I think the right approach here would have been to remove Eliza and it's references, as well as the code built around Eliza to employ its use. > > > Why was this changed from eliza to buildbot? I think all Eliza references should just be removed. > > This code isn't testing eliza, it is testing that autoinstall hooks work. > So, removing it would have removed important test coverage. > > It doesn't matter which package to test against, eliza must have been picked > randomly. I see that now. I was wrong, everything seems correct there.
> Maybe I first need to understand why you replaced it with UnknownCommand? I think that the bot should respond with _something_ when it doesn't recognize the command, to avoid giving the impression that it's frozen. Which exact thing to say is debatable. I went with "...", but something like "unknown command" would have worked too.