Bug 105607 – Use Python 2.7 unittest syntax in our unittests

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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

[Dirk Pranke]

unittest2 is a backport of the 2.7 unittest module to 2.3-2.6. We could just install that.

[Zan Dobersek]

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

We should go through and actually update any tests to use the proper 2.7 methods?

[Dirk Pranke]

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]

(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)]

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

(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)]

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

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

Created attachment 183887 [details]
Patch

[WebKit Review Bot]

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)]

Should I be concerned by the pycheck failures?

[Dirk Pranke]

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

I think these errors are caused by the pylint not understanding that we are importing unittest2 as unittest.

[Eric Seidel (no email)]

(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)]

Maybe we need to modify the sys.path when running pylint?

[Tim 'mithro' Ansell]

Created attachment 184143 [details]
Patch

[Eric Seidel (no email)]

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 on attachment 184143 [details]
Patch

Clearing flags on attachment: 184143

Committed r140510: <http://trac.webkit.org/changeset/140510>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Dirk Pranke]

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]

We rely on this functionality for other modules, so I think we should be consistent?

[Dirk Pranke]

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

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

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)]

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