Bug 164979 – Remove webkitpy dependency on Eliza.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Dean Johnson]

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.

[Alexey Proskuryakov]

Created attachment 295901 [details]
proposed patch

[Daniel Bates]

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

[Alexey Proskuryakov]

Committed r209216.

[Dean Johnson]

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.

[Alexey Proskuryakov]

> 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.

[Dean Johnson]

(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.

[Alexey Proskuryakov]

> 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.