Bug 105607 - Use Python 2.7 unittest syntax in our unittests
Summary: Use Python 2.7 unittest syntax in our unittests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim 'mithro' Ansell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-12-20 20:25 PST by Eric Seidel (no email)
Modified: 2013-01-24 16:48 PST (History)
11 users (show)

See Also:


Attachments
Patch (173.58 KB, patch)
2013-01-21 21:44 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff
Patch (180.25 KB, patch)
2013-01-22 22:39 PST, Tim 'mithro' Ansell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-12-20 20:25:42 PST
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. :)
Comment 1 Dirk Pranke 2012-12-21 21:01:32 PST
unittest2 is a backport of the 2.7 unittest module to 2.3-2.6. We could just install that.
Comment 2 Zan Dobersek 2013-01-10 02:22:38 PST
(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?
Comment 3 Tim 'mithro' Ansell 2013-01-10 02:40:17 PST
We should go through and actually update any tests to use the proper 2.7 methods?
Comment 4 Dirk Pranke 2013-01-10 09:12:53 PST
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.
Comment 5 Zan Dobersek 2013-01-10 09:52:49 PST
(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?
Comment 6 Eric Seidel (no email) 2013-01-10 10:24:30 PST
(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.
Comment 7 Dirk Pranke 2013-01-10 11:02:35 PST
(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).
Comment 8 Eric Seidel (no email) 2013-01-10 11:06:47 PST
(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.
Comment 9 Dirk Pranke 2013-01-14 14:24:20 PST
(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?
Comment 10 Tim 'mithro' Ansell 2013-01-21 21:44:20 PST
Created attachment 183887 [details]
Patch
Comment 11 WebKit Review Bot 2013-01-21 21:48:14 PST
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.
Comment 12 Eric Seidel (no email) 2013-01-22 01:17:39 PST
Should I be concerned by the pycheck failures?
Comment 13 Dirk Pranke 2013-01-22 12:03:23 PST
(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.
Comment 14 Tim 'mithro' Ansell 2013-01-22 16:07:37 PST
I think these errors are caused by the pylint not understanding that we are importing unittest2 as unittest.
Comment 15 Eric Seidel (no email) 2013-01-22 16:24:52 PST
(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?
Comment 16 Eric Seidel (no email) 2013-01-22 16:26:25 PST
Maybe we need to modify the sys.path when running pylint?
Comment 17 Tim 'mithro' Ansell 2013-01-22 22:39:17 PST
Created attachment 184143 [details]
Patch
Comment 18 Eric Seidel (no email) 2013-01-22 22:51:59 PST
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 19 WebKit Review Bot 2013-01-22 23:14:14 PST
Comment on attachment 184143 [details]
Patch

Clearing flags on attachment: 184143

Committed r140510: <http://trac.webkit.org/changeset/140510>
Comment 20 WebKit Review Bot 2013-01-22 23:14:19 PST
All reviewed patches have been landed.  Closing bug.
Comment 21 Dirk Pranke 2013-01-23 11:44:29 PST
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' ?
Comment 22 Tim 'mithro' Ansell 2013-01-23 18:37:15 PST
We rely on this functionality for other modules, so I think we should be consistent?
Comment 23 Dirk Pranke 2013-01-24 15:57:28 PST
(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?
Comment 24 Tim 'mithro' Ansell 2013-01-24 16:19:21 PST
(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?
Comment 25 Dirk Pranke 2013-01-24 16:25:56 PST
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.
Comment 26 Eric Seidel (no email) 2013-01-24 16:48:59 PST
(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.