164979 2016-11-18 18:55:26 -0800 Remove webkitpy dependency on Eliza. 2016-12-01 16:02:04 -0800 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 dean_johnson ap ap commit-queue glenn lforschler oldest_to_newest 1253007 0 dean_johnson 2016-11-18 18:55:26 -0800 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. 1255493 1 295901 ap 2016-12-01 14:46:38 -0800 Created attachment 295901 proposed patch 1255495 2 295901 dbates 2016-12-01 14:49:19 -0800 Comment on attachment 295901 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 1255498 3 ap 2016-12-01 14:54:04 -0800 Committed r209216. 1255504 4 295901 dean_johnson 2016-12-01 15:00:02 -0800 Comment on attachment 295901 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. 1255519 5 ap 2016-12-01 15:31:09 -0800 > 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. 1255526 6 dean_johnson 2016-12-01 15:41:55 -0800 (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. 1255536 7 ap 2016-12-01 16:02:04 -0800 > 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. 295901 2016-12-01 14:46:38 -0800 2016-12-01 14:49:19 -0800 proposed patch eliza.txt text/plain 6683 ap SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDIwOTIxNCkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE4IEBACisyMDE2LTEyLTAxICBBbGV4ZXkgUHJvc2t1cnlha292ICA8YXBAYXBwbGUuY29t PgorCisgICAgICAgIFJlbW92ZSB3ZWJraXRweSBkZXBlbmRlbmN5IG9uIEVsaXphCisgICAgICAg IGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjQ5NzkKKworICAgICAg ICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUaGlzIG1vZHVsZSBpcyBu b3Qgb24gcHlwaSwgc28gaW5zdGFsbGluZyBpdCBpcyBhIGNoYWxsZW5nZS4gVGhlIGpva2VzIGZl ZWwgcHJldHR5IG9sZCB0b28hCisKKyAgICAgICAgKiBTY3JpcHRzL3dlYmtpdHB5L3RoaXJkcGFy dHkvX19pbml0X18ucHk6CisgICAgICAgICogU2NyaXB0cy93ZWJraXRweS90aGlyZHBhcnR5L19f aW5pdF9fX3VuaXR0ZXN0LnB5OiBDaGFuZ2VkIG1vY2sgaW1wb3J0IGhvb2sgdG8KKyAgICAgICAg dGVzdCBidWlsZGJvdCBpbnN0ZWFkIG9mIGVsaXphLgorICAgICAgICAqIFNjcmlwdHMvd2Via2l0 cHkvdG9vbC9ib3QvaXJjYm90LnB5OgorICAgICAgICAqIFNjcmlwdHMvd2Via2l0cHkvdG9vbC9i b3QvaXJjYm90X3VuaXR0ZXN0LnB5OgorCiAyMDE2LTEyLTAxICBCZXRoIERha2luICA8YmRha2lu QGFwcGxlLmNvbT4KIAogICAgICAgICBGb3JjZSBjbGlja2luZyB0ZXN0cyBmYWlsIG9uIHNvbWUg bWFjaGluZXMKSW5kZXg6IFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvdGhpcmRwYXJ0eS9fX2luaXRf Xy5weQo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09Ci0tLSBUb29scy9TY3JpcHRzL3dlYmtpdHB5L3RoaXJkcGFydHkvX19p bml0X18ucHkJKHJldmlzaW9uIDIwOTIwNikKKysrIFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvdGhp cmRwYXJ0eS9fX2luaXRfXy5weQkod29ya2luZyBjb3B5KQpAQCAtODUsOCArODUsNiBAQCBjbGFz cyBBdXRvaW5zdGFsbEltcG9ydEhvb2sob2JqZWN0KToKICAgICAgICAgICAgIHNlbGYuX2luc3Rh bGxfcHlsaW50KCkKICAgICAgICAgZWxpZiAnLmNvdmVyYWdlJyBpbiBmdWxsbmFtZToKICAgICAg ICAgICAgIHNlbGYuX2luc3RhbGxfY292ZXJhZ2UoKQotICAgICAgICBlbGlmICcuZWxpemEnIGlu IGZ1bGxuYW1lOgotICAgICAgICAgICAgc2VsZi5faW5zdGFsbF9lbGl6YSgpCiAgICAgICAgIGVs aWYgJy5idWlsZGJvdCcgaW4gZnVsbG5hbWU6CiAgICAgICAgICAgICBzZWxmLl9pbnN0YWxsX2J1 aWxkYm90KCkKICAgICAgICAgZWxpZiAnLmtleXJpbmcnIGluIGZ1bGxuYW1lOgpAQCAtMTQ3LDkg KzE0NSw2IEBAIGNsYXNzIEF1dG9pbnN0YWxsSW1wb3J0SG9vayhvYmplY3QpOgogICAgICAgICBz ZWxmLl9lbnN1cmVfYXV0b2luc3RhbGxlZF9kaXJfaXNfaW5fc3lzX3BhdGgoKQogICAgICAgICBz ZWxmLl9pbnN0YWxsKHVybD0iaHR0cDovL3B5cGkucHl0aG9uLm9yZy9wYWNrYWdlcy9zb3VyY2Uv Yy9jb3ZlcmFnZS9jb3ZlcmFnZS0zLjUuMS50YXIuZ3ojbWQ1PTQxMGQ0YzgxNTVhNGRhYjIyMmYy YmM1MTIxMmQ0YTI0IiwgdXJsX3N1YnBhdGg9ImNvdmVyYWdlLTMuNS4xL2NvdmVyYWdlIikKIAot ICAgIGRlZiBfaW5zdGFsbF9lbGl6YShzZWxmKToKLSAgICAgICAgc2VsZi5faW5zdGFsbCh1cmw9 Imh0dHA6Ly93d3cuYWRhbWJhcnRoLmNvbS93ZWJraXQvZWxpemEiLCB0YXJnZXRfbmFtZT0iZWxp emEucHkiKQotCiAgICAgZGVmIF9pbnN0YWxsX3R3aXN0ZWRfMTVfNV8wKHNlbGYpOgogICAgICAg ICB0d2lzdGVkX2RpciA9IHNlbGYuX2ZzLmpvaW4oX0FVVE9JTlNUQUxMRURfRElSLCAidHdpc3Rl ZF8xNV81XzAiKQogICAgICAgICBpbnN0YWxsZXIgPSBBdXRvSW5zdGFsbGVyKHByZXBlbmRfdG9f c2VhcmNoX3BhdGg9VHJ1ZSwgdGFyZ2V0X2Rpcj10d2lzdGVkX2RpcikKSW5kZXg6IFRvb2xzL1Nj cmlwdHMvd2Via2l0cHkvdGhpcmRwYXJ0eS9fX2luaXRfX191bml0dGVzdC5weQo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBUb29scy9TY3JpcHRzL3dlYmtpdHB5L3RoaXJkcGFydHkvX19pbml0X19fdW5pdHRlc3Qu cHkJKHJldmlzaW9uIDIwOTIwNikKKysrIFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvdGhpcmRwYXJ0 eS9fX2luaXRfX191bml0dGVzdC5weQkod29ya2luZyBjb3B5KQpAQCAtMzgsMTAgKzM4LDEwIEBA IGNsYXNzIFRoaXJkcGFydHlUZXN0KHVuaXR0ZXN0LlRlc3RDYXNlKToKICAgICAgICAgY2xhc3Mg TW9ja0ltcG9ydEhvb2soQXV0b2luc3RhbGxJbXBvcnRIb29rKToKICAgICAgICAgICAgIGRlZiBf X2luaXRfXyhzZWxmKToKICAgICAgICAgICAgICAgICBBdXRvaW5zdGFsbEltcG9ydEhvb2suX19p bml0X18oc2VsZikKLSAgICAgICAgICAgICAgICBzZWxmLmVsaXphX2luc3RhbGxlZCA9IEZhbHNl CisgICAgICAgICAgICAgICAgc2VsZi5idWlsZGJvdF9pbnN0YWxsZWQgPSBGYWxzZQogCi0gICAg ICAgICAgICBkZWYgX2luc3RhbGxfZWxpemEoc2VsZik6Ci0gICAgICAgICAgICAgICAgc2VsZi5l bGl6YV9pbnN0YWxsZWQgPSBUcnVlCisgICAgICAgICAgICBkZWYgX2luc3RhbGxfYnVpbGRib3Qo c2VsZik6CisgICAgICAgICAgICAgICAgc2VsZi5idWlsZGJvdF9pbnN0YWxsZWQgPSBUcnVlCiAK ICAgICAgICAgbW9ja19pbXBvcnRfaG9vayA9IE1vY2tJbXBvcnRIb29rKCkKICAgICAgICAgdHJ5 OgpAQCAtNDksOCArNDksOCBAQCBjbGFzcyBUaGlyZHBhcnR5VGVzdCh1bml0dGVzdC5UZXN0Q2Fz ZSk6CiAgICAgICAgICAgICAjIHNvIHRoZXNlIG1vZHVsZXMgd2lsbCBnZXQgaW5zdGFsbGVkIGJl Zm9yZSBNb2NrSW1wb3J0SG9vayBydW5zLgogICAgICAgICAgICAgc3lzLm1ldGFfcGF0aC5hcHBl bmQobW9ja19pbXBvcnRfaG9vaykKICAgICAgICAgICAgICMgdW51c2VkLXZhcmlhYmxlLCBpbXBv cnQgZmFpbHVyZXMgLSBweWxpbnQ6IGRpc2FibGUtbXNnPVcwNjEyLEUwNjExLEYwNDAxCi0gICAg ICAgICAgICBmcm9tIHdlYmtpdHB5LnRoaXJkcGFydHkuYXV0b2luc3RhbGxlZCBpbXBvcnQgZWxp emEKLSAgICAgICAgICAgIHNlbGYuYXNzZXJ0VHJ1ZShtb2NrX2ltcG9ydF9ob29rLmVsaXphX2lu c3RhbGxlZCkKKyAgICAgICAgICAgIGZyb20gd2Via2l0cHkudGhpcmRwYXJ0eS5hdXRvaW5zdGFs bGVkIGltcG9ydCBidWlsZGJvdAorICAgICAgICAgICAgc2VsZi5hc3NlcnRUcnVlKG1vY2tfaW1w b3J0X2hvb2suYnVpbGRib3RfaW5zdGFsbGVkKQogCiAgICAgICAgIGZpbmFsbHk6CiAgICAgICAg ICAgICBzeXMubWV0YV9wYXRoLnJlbW92ZShtb2NrX2ltcG9ydF9ob29rKQpAQCAtNjAsNyArNjAs NiBAQCBjbGFzcyBUaGlyZHBhcnR5VGVzdCh1bml0dGVzdC5UZXN0Q2FzZSk6CiAgICAgICAgICMg dW51c2VkLXZhcmlhYmxlLCBpbXBvcnQgZmFpbHVyZXMgLSBweWxpbnQ6IGRpc2FibGUtbXNnPVcw NjEyLEUwNjExLEYwNDAxCiAgICAgICAgIGltcG9ydCB3ZWJraXRweS50aGlyZHBhcnR5LmF1dG9p bnN0YWxsZWQuYnVpbGRib3QKICAgICAgICAgaW1wb3J0IHdlYmtpdHB5LnRoaXJkcGFydHkuYXV0 b2luc3RhbGxlZC5jb3ZlcmFnZQotICAgICAgICBpbXBvcnQgd2Via2l0cHkudGhpcmRwYXJ0eS5h dXRvaW5zdGFsbGVkLmVsaXphCiAgICAgICAgIGltcG9ydCB3ZWJraXRweS50aGlyZHBhcnR5LmF1 dG9pbnN0YWxsZWQubWVjaGFuaXplCiAgICAgICAgIGltcG9ydCB3ZWJraXRweS50aGlyZHBhcnR5 LmF1dG9pbnN0YWxsZWQucHlsaW50CiAgICAgICAgIGltcG9ydCB3ZWJraXRweS50aGlyZHBhcnR5 LmF1dG9pbnN0YWxsZWQucGVwOApJbmRleDogVG9vbHMvU2NyaXB0cy93ZWJraXRweS90b29sL2Jv dC9pcmNib3QucHkKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PQotLS0gVG9vbHMvU2NyaXB0cy93ZWJraXRweS90b29sL2Jv dC9pcmNib3QucHkJKHJldmlzaW9uIDIwOTIwNikKKysrIFRvb2xzL1NjcmlwdHMvd2Via2l0cHkv dG9vbC9ib3QvaXJjYm90LnB5CSh3b3JraW5nIGNvcHkpCkBAIC01MiwxNiArNTIsOSBAQCBjbGFz cyBfSVJDVGhyZWFkVGVhcm9mZihJUkNCb3REZWxlZ2F0ZSk6CiAgICAgICAgIHJldHVybiBzZWxm Ll9wYXNzd29yZAogCiAKLWNsYXNzIEVsaXphKElSQ0NvbW1hbmQpOgotICAgIHRoZXJhcGlzdCA9 IE5vbmUKLQotICAgIGRlZiBfX2luaXRfXyhzZWxmKToKLSAgICAgICAgaWYgbm90IHNlbGYudGhl cmFwaXN0OgotICAgICAgICAgICAgaW1wb3J0IHdlYmtpdHB5LnRoaXJkcGFydHkuYXV0b2luc3Rh bGxlZC5lbGl6YSBhcyBlbGl6YQotICAgICAgICAgICAgRWxpemEudGhlcmFwaXN0ID0gZWxpemEu ZWxpemEoKQotCitjbGFzcyBVbmtub3duQ29tbWFuZChJUkNDb21tYW5kKToKICAgICBkZWYgZXhl Y3V0ZShzZWxmLCBuaWNrLCBhcmdzLCB0b29sLCBzaGVyaWZmKToKLSAgICAgICAgcmV0dXJuICIl czogJXMiICUgKG5pY2ssIHNlbGYudGhlcmFwaXN0LnJlc3BvbmQoIiAiLmpvaW4oYXJncykpKQor ICAgICAgICByZXR1cm4gIiVzOiAlcyIgJSAobmljaywgIi4uLiIpCiAKIAogY2xhc3MgSVJDQm90 KG9iamVjdCk6CkBAIC04Miw3ICs3NSw3IEBAIGNsYXNzIElSQ0JvdChvYmplY3QpOgogICAgICAg ICBhcmdzID0gdG9rZW5pemVkX3JlcXVlc3RbMTpdCiAgICAgICAgIGlmIG5vdCBjb21tYW5kOgog ICAgICAgICAgICAgIyBHaXZlIHRoZSBwZW9wbGVzIHNvbWVvbmUgdG8gdGFsayB3aXRoLgotICAg ICAgICAgICAgY29tbWFuZCA9IEVsaXphCisgICAgICAgICAgICBjb21tYW5kID0gVW5rbm93bkNv bW1hbmQKICAgICAgICAgICAgIGFyZ3MgPSB0b2tlbml6ZWRfcmVxdWVzdAogICAgICAgICByZXR1 cm4gKGNvbW1hbmQsIGFyZ3MpCiAKSW5kZXg6IFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvdG9vbC9i b3QvaXJjYm90X3VuaXR0ZXN0LnB5Cj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFRvb2xzL1NjcmlwdHMvd2Via2l0 cHkvdG9vbC9ib3QvaXJjYm90X3VuaXR0ZXN0LnB5CShyZXZpc2lvbiAyMDkyMDYpCisrKyBUb29s cy9TY3JpcHRzL3dlYmtpdHB5L3Rvb2wvYm90L2lyY2JvdF91bml0dGVzdC5weQkod29ya2luZyBj b3B5KQpAQCAtMzQsNyArMzQsNyBAQCBmcm9tIHdlYmtpdHB5LnRvb2wuYm90IGltcG9ydCBpcmNf Y29tbWFuCiBmcm9tIHdlYmtpdHB5LnRvb2wuYm90LnF1ZXVlZW5naW5lIGltcG9ydCBUZXJtaW5h dGVRdWV1ZQogZnJvbSB3ZWJraXRweS50b29sLmJvdC5zaGVyaWZmIGltcG9ydCBTaGVyaWZmCiBm cm9tIHdlYmtpdHB5LnRvb2wuYm90LmlyY2JvdCBpbXBvcnQgSVJDQm90Ci1mcm9tIHdlYmtpdHB5 LnRvb2wuYm90LmlyY2JvdCBpbXBvcnQgRWxpemEKK2Zyb20gd2Via2l0cHkudG9vbC5ib3QuaXJj Ym90IGltcG9ydCBVbmtub3duQ29tbWFuZAogZnJvbSB3ZWJraXRweS50b29sLmJvdC5zaGVyaWZm X3VuaXR0ZXN0IGltcG9ydCBNb2NrU2hlcmlmZkJvdAogZnJvbSB3ZWJraXRweS50b29sLm1vY2t0 b29sIGltcG9ydCBNb2NrVG9vbAogCkBAIC00OCwxNiArNDgsMTEgQEAgZGVmIHJ1bihtZXNzYWdl KToKIAogCiBjbGFzcyBJUkNCb3RUZXN0KHVuaXR0ZXN0LlRlc3RDYXNlKToKLSAgICBkZWYgdGVz dF9lbGl6YShzZWxmKToKLSAgICAgICAgZWxpemEgPSBFbGl6YSgpCi0gICAgICAgIGVsaXphLmV4 ZWN1dGUoInRvbSIsICJoaSIsIE5vbmUsIE5vbmUpCi0gICAgICAgIGVsaXphLmV4ZWN1dGUoInRv bSIsICJieWUiLCBOb25lLCBOb25lKQotCiAgICAgZGVmIHRlc3RfcGFyc2VfY29tbWFuZF9hbmRf YXJncyhzZWxmKToKICAgICAgICAgdG9vbCA9IE1vY2tUb29sKCkKICAgICAgICAgYm90ID0gSVJD Qm90KCJzaGVyaWZmYm90IiwgdG9vbCwgU2hlcmlmZih0b29sLCBNb2NrU2hlcmlmZkJvdCgpKSwg aXJjX2NvbW1hbmQuY29tbWFuZHMpCi0gICAgICAgIHNlbGYuYXNzZXJ0RXF1YWwoYm90Ll9wYXJz ZV9jb21tYW5kX2FuZF9hcmdzKCIiKSwgKEVsaXphLCBbIiJdKSkKLSAgICAgICAgc2VsZi5hc3Nl cnRFcXVhbChib3QuX3BhcnNlX2NvbW1hbmRfYW5kX2FyZ3MoIiAgICIpLCAoRWxpemEsIFsiIl0p KQorICAgICAgICBzZWxmLmFzc2VydEVxdWFsKGJvdC5fcGFyc2VfY29tbWFuZF9hbmRfYXJncygi IiksIChVbmtub3duQ29tbWFuZCwgWyIiXSkpCisgICAgICAgIHNlbGYuYXNzZXJ0RXF1YWwoYm90 Ll9wYXJzZV9jb21tYW5kX2FuZF9hcmdzKCIgICAiKSwgKFVua25vd25Db21tYW5kLCBbIiJdKSkK ICAgICAgICAgc2VsZi5hc3NlcnRFcXVhbChib3QuX3BhcnNlX2NvbW1hbmRfYW5kX2FyZ3MoIiBo aSAiKSwgKGlyY19jb21tYW5kLkhpLCBbXSkpCiAgICAgICAgIHNlbGYuYXNzZXJ0RXF1YWwoYm90 Ll9wYXJzZV9jb21tYW5kX2FuZF9hcmdzKCIgaGkgdGhlcmUgIiksIChpcmNfY29tbWFuZC5IaSwg WyJ0aGVyZSJdKSkKIAo=