Make it possible to use Python 2.7 unittest syntax in our unittests It's long annoyed me that we don't get to use the fancy new self.assertMultiLineStringEquals and other similar type-aware assert methods in our unittests. Even assertEquals in python 2.7 is type-aware and does much nicer printing. It would be nice if we could either add ourselves some sort of unittest shim for python 2.6 and below, or find some other way to make this possible. :) Please pardon the broad you're-good-at-python CC, and feel welcome to remove yourself if you're not interested. :)
unittest2 is a backport of the 2.7 unittest module to 2.3-2.6. We could just install that.
(In reply to comment #1) > unittest2 is a backport of the 2.7 unittest module to 2.3-2.6. We could just install that. Done in r139294. http://trac.webkit.org/changeset/139294 Is there anything else this bug needs doing?
We should go through and actually update any tests to use the proper 2.7 methods?
We should probably go through all the files and change unittest to unittest2 so that we're consistent in the functionality available in the tests.
(In reply to comment #4) > We should probably go through all the files and change unittest to unittest2 so that we're consistent in the functionality available in the tests. Just to clear it up, in a recent patch the unittest2 module was not imported from webkitpy.thirdparty.autoinstalled. What's the stance on this? Could a system-wide unittest2 module be used instead of the one that's installed by webkitpy? Could this cause problems?
(In reply to comment #5) > (In reply to comment #4) > > We should probably go through all the files and change unittest to unittest2 so that we're consistent in the functionality available in the tests. > > Just to clear it up, in a recent patch the unittest2 module was not imported from webkitpy.thirdparty.autoinstalled. What's the stance on this? Could a system-wide unittest2 module be used instead of the one that's installed by webkitpy? Could this cause problems? It appears thirdparty.autoinstalled is added to sys.path these days, for better or worse.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > We should probably go through all the files and change unittest to unittest2 so that we're consistent in the functionality available in the tests. > > > > Just to clear it up, in a recent patch the unittest2 module was not imported from webkitpy.thirdparty.autoinstalled. What's the stance on this? Could a system-wide unittest2 module be used instead of the one that's installed by webkitpy? Could this cause problems? > I suppose it's possible that it could, but I wouldn't normally expect it to. > It appears thirdparty.autoinstalled is added to sys.path these days, for better or worse. Yes, it needs to, for some of the packages to work (pylint in particular, though I think there's another as well).
(In reply to comment #7) > > It appears thirdparty.autoinstalled is added to sys.path these days, for better or worse. > > Yes, it needs to, for some of the packages to work (pylint in particular, though I think there's another as well). I feel like that didn't used to be the case. Mechanize has dependent packages, but I thought we worked aroudn that by manually adding the bits it was expecting to sys.path? Or maybe I'm mistaken.
(In reply to comment #8) > (In reply to comment #7) > > > It appears thirdparty.autoinstalled is added to sys.path these days, for better or worse. > > > > Yes, it needs to, for some of the packages to work (pylint in particular, though I think there's another as well). > > I feel like that didn't used to be the case. Mechanize has dependent packages, but I thought we worked aroudn that by manually adding the bits it was expecting to sys.path? Or maybe I'm mistaken. It doesn't look like mechanize still has any dependent packages now. I'm not actually sure what you mean by "manually adding the bits it was expecting to sys.path"; how is that different from putting webkit.thirdparty.autoinstalled into sys.path?
Created attachment 183887 [details] Patch
Attachment 183887 [details] did not pass style-queue: Tools/Scripts/webkitpy/tool/commands/queues_unittest.py:159: [AbstractPatchQueueTest.test_next_patch] Instance of 'AbstractPatchQueueTest' has no 'assertIsNone' member [pylint/E1101] [5] Tools/Scripts/webkitpy/style/optparser_unittest.py:148: [ArgumentParserTest.test_parse_default_arguments] Instance of 'ArgumentParserTest' has no 'assertIsNone' member [pylint/E1101] [5] Tools/Scripts/webkitpy/layout_tests/port/mac_unittest.py:90: [MacTest.test_tests_for_other_platforms] Instance of 'MacTest' has no 'assertIn' member [pylint/E1101] [5] Tools/Scripts/webkitpy/layout_tests/port/mac_unittest.py:91: [MacTest.test_tests_for_other_platforms] Instance of 'MacTest' has no 'assertNotIn' member [pylint/E1101] [5] Tools/Scripts/webkitpy/layout_tests/port/mac_unittest.py:92: [MacTest.test_tests_for_other_platforms] Instance of 'MacTest' has no 'assertNotIn' member [pylint/E1101] [5] Tools/Scripts/webkitpy/common/system/crashlogs_unittest.py:91: [CrashLogsTest.test_find_log_darwin] Instance of 'CrashLogsTest' has no 'assertMutliLineEqual' member [pylint/E1101] [5] Tools/Scripts/webkitpy/common/system/executive_unittest.py:175: [ExecutiveTest.serial_test_kill_process] Instance of 'ExecutiveTest' has no 'assertIn' member [pylint/E1101] [5] Tools/Scripts/webkitpy/common/system/executive_unittest.py:179: [ExecutiveTest.serial_test_kill_process] Instance of 'ExecutiveTest' has no 'assertIn' member [pylint/E1101] [5] Tools/Scripts/webkitpy/common/system/executive_unittest.py:190: [ExecutiveTest.serial_test_kill_all] Instance of 'ExecutiveTest' has no 'assertIsNone' member [pylint/E1101] [5] Tools/Scripts/webkitpy/common/system/executive_unittest.py:199: [ExecutiveTest.serial_test_kill_all] Instance of 'ExecutiveTest' has no 'assertIn' member [pylint/E1101] [5] Tools/Scripts/webkitpy/common/system/executive_unittest.py:232: [ExecutiveTest.serial_test_running_pids] Instance of 'ExecutiveTest' has no 'assertIn' member [pylint/E1101] [5] /mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/pylint/lint.py:393: DeprecationWarning: disable-msg is deprecated, replace it by disable (/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py, line self.command = self.command_constructor() # lint warns that command_constructor might not be set, but this is intentional; pylint: disable-msg=E1102 ) DeprecationWarning) /mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/pylint/lint.py:393: DeprecationWarning: disable-msg is deprecated, replace it by disable (/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/tool/commands/rebaseline_unittest.py, line self.command._write = (lambda msg: self.lines.append(msg)) # pylint bug warning about unnecessary lambda? pylint: disable-msg=W0108 ) DeprecationWarning) Tools/Scripts/webkitpy/layout_tests/port/gtk_unittest.py:92: [GtkPortTest.test_get_crash_log] Instance of 'GtkPortTest' has no 'assertMultiLineEqual' member [pylint/E1101] [5] Tools/Scripts/webkitpy/layout_tests/port/gtk_unittest.py:96: [GtkPortTest.test_get_crash_log] Instance of 'GtkPortTest' has no 'assertMultiLineEqual' member [pylint/E1101] [5] Tools/Scripts/webkitpy/layout_tests/port/chromium_win_unittest.py:77: [ChromiumWinTest.test_versions] Instance of 'ChromiumWinTest' has no 'assertIn' member [pylint/E1101] [5] Tools/Scripts/webkitpy/tool/commands/earlywarningsystem_unittest.py:50: [AbstractEarlyWarningSystemTest.test_failing_tests_message] Instance of 'AbstractEarlyWarningSystemTest' has no 'assertMultiLineEqual' member [pylint/E1101] [5] /mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/pylint/lint.py:393: DeprecationWarning: disable-msg is deprecated, replace it by disable (/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/thirdparty/__init___unittest.py, line # unused-variable, import failures - pylint: disable-msg=W0612,E0611,F0401 ) DeprecationWarning) /mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/thirdparty/autoinstalled/pylint/lint.py:393: DeprecationWarning: disableFailed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/Scripts/webkitpy..." exit_code: 1 -msg is deprecated, replace it by disable (/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/thirdparty/__init___unittest.py, line # unused-variable, import failures - pylint: disable-msg=W0612,E0611,F0401 ) DeprecationWarning) Tools/Scripts/webkitpy/tool/commands/roll_unittest.py:60: [PostRollCommandsTest.test_prepare_state] Instance of 'PostRollCommandsTest' has no 'assertIsNone' member [pylint/E1101] [5] Tools/Scripts/webkitpy/common/checkout/checkout_unittest.py:163: [CheckoutTest.test_latest_entry_for_changelog_at_revision] Instance of 'str' has no 'contents' member [pylint/E1101] [5] Total errors found: 17 in 154 files If any of these errors are false positives, please file a bug against check-webkit-style.
Should I be concerned by the pycheck failures?
(In reply to comment #12) > Should I be concerned by the pycheck failures? I would be. I'm guessing that pylint is getting confused by the "import ... as", but we need to get to the bottom of that.
I think these errors are caused by the pylint not understanding that we are importing unittest2 as unittest.
(In reply to comment #14) > I think these errors are caused by the pylint not understanding that we are importing unittest2 as unittest. I can imagine that being true. Is it possible to pass an option to pylint to make it smarter?
Maybe we need to modify the sys.path when running pylint?
Created attachment 184143 [details] Patch
Comment on attachment 184143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184143&action=review Fantastic! Thank you very much! > Tools/Scripts/webkitpy/common/system/executive_unittest.py:38 > # Since we execute this script directly as part of the unit tests, we need to ensure > # that Tools/Scripts is in sys.path for the next imports to work correctly. Huh? Really? Oh, I guess it execs itself. We could totally have a separate simpler script. :p
Comment on attachment 184143 [details] Patch Clearing flags on attachment: 184143 Committed r140510: <http://trac.webkit.org/changeset/140510>
All reviewed patches have been landed. Closing bug.
I'm a little bit reluctant to rely on the fact that something has put webkitpy.thirdparty.autoinstalled into sys.path already; perhaps it would've been better to change things to 'from webkitpy.thirdparty.autoinstalled import unittest2 as unittest' ?
We rely on this functionality for other modules, so I think we should be consistent?
(In reply to comment #22) > We rely on this functionality for other modules, so I think we should be consistent? I'm sorry, I don't understand your comment. What are you referring to with "this functionality" and "be consistent"? AFAIK, we refer to stuff under webkitpy.thirdparty.autoinstalled explicitly using that path; we should not be assuming (in our code) that that is in sys.path. But perhaps we're not consistent here and some code does assume you can import those packages directly?
(In reply to comment #23) > (In reply to comment #22) > > We rely on this functionality for other modules, so I think we should be consistent? > > I'm sorry, I don't understand your comment. What are you referring to with "this functionality" and "be consistent"? > > AFAIK, we refer to stuff under webkitpy.thirdparty.autoinstalled explicitly using that path; we should not be assuming (in our code) that that is in sys.path. But perhaps we're not consistent here and some code does assume you can import those packages directly? From what I can see, most code is just importing via the sys.path hack rather than explicitly. eseidel had the same impression you did but this seems to have changed some time ago?
I see. Well, I vote that we should stop relying on webkitpy.thirdparty.autoinstalled being in sys.path, so we should be consistent in the other direction.
(In reply to comment #25) > I see. Well, I vote that we should stop relying on webkitpy.thirdparty.autoinstalled being in sys.path, so we should be consistent in the other direction. Sounds like someone should file a bug with information as to how good or bad we are at this, and we can post patches to switch us fully to one way or the other. Depending on what order of sys.path, it's not immediately clear to me if it's desired behavior that the non-specific paths can fall-back to locally installed modules. I guess that feels odd, and seems like a reason why we might want to be specific in these paths.