WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
105607
Use Python 2.7 unittest syntax in our unittests
https://bugs.webkit.org/show_bug.cgi?id=105607
Summary
Use Python 2.7 unittest syntax in our unittests
Eric Seidel (no email)
Reported
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. :)
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Dirk Pranke
Comment 1
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.
Zan Dobersek
Comment 2
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?
Tim 'mithro' Ansell
Comment 3
2013-01-10 02:40:17 PST
We should go through and actually update any tests to use the proper 2.7 methods?
Dirk Pranke
Comment 4
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.
Zan Dobersek
Comment 5
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?
Eric Seidel (no email)
Comment 6
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.
Dirk Pranke
Comment 7
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).
Eric Seidel (no email)
Comment 8
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.
Dirk Pranke
Comment 9
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?
Tim 'mithro' Ansell
Comment 10
2013-01-21 21:44:20 PST
Created
attachment 183887
[details]
Patch
WebKit Review Bot
Comment 11
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.
Eric Seidel (no email)
Comment 12
2013-01-22 01:17:39 PST
Should I be concerned by the pycheck failures?
Dirk Pranke
Comment 13
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.
Tim 'mithro' Ansell
Comment 14
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.
Eric Seidel (no email)
Comment 15
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?
Eric Seidel (no email)
Comment 16
2013-01-22 16:26:25 PST
Maybe we need to modify the sys.path when running pylint?
Tim 'mithro' Ansell
Comment 17
2013-01-22 22:39:17 PST
Created
attachment 184143
[details]
Patch
Eric Seidel (no email)
Comment 18
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
WebKit Review Bot
Comment 19
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
>
WebKit Review Bot
Comment 20
2013-01-22 23:14:19 PST
All reviewed patches have been landed. Closing bug.
Dirk Pranke
Comment 21
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' ?
Tim 'mithro' Ansell
Comment 22
2013-01-23 18:37:15 PST
We rely on this functionality for other modules, so I think we should be consistent?
Dirk Pranke
Comment 23
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?
Tim 'mithro' Ansell
Comment 24
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?
Dirk Pranke
Comment 25
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.
Eric Seidel (no email)
Comment 26
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug