Unittests/Python 3: update syntax, catch unbound method registration test in extension points as expected failure by josephsl · Pull Request #9726 · nvaccess/nvda

josephsl · 2019-06-12T15:54:03Z

Link to issue number:

Fixes #9720

Summary of the issue:

Updates unitt4est test case syntax to Python 3.

Description of how this pull request fixes the issue:

Unittest test cases uses Python 2 syntax, which will fail if run with Python 3. Thus update syntax.

Procedure:

With latest py3 staging branch, run unit tests with scons tests.
For every error, look for tests that are causing the error. Errors may include relative import problem (unable to import "test.unit" caused by relative import issue).

Testing performed:

Tested with Python 2 and 3 from source.

Known issues with pull request:

In extension points tests, unbound method registration test fails because TypeError is not raised. This is marked as expected failure.
Due to repeated insertion of msvcr90.dll by brltty/brlapi, braille display gestures integrity test causes VC++ runtime error to be thrown (see Brltty/Python 3: R6034 error when running unittests #9721).

Change log entry:

None

…ive import. Re nvaccess#9720.

…nverting it into Unicode first. Re nvaccess#9720.

…l. Re nvaccess#9720. Python 2's unittest documentation states self.assertItemsEqual is replaced by self.assertCountEqual in Python 3, thus follow this directive.

…test as expected failure. Re nvaccess#9720. For some reason unbound method registration test keeps failing in Python 3. Until this is resolved, mark this test as expected failure.

…RaisesRegex due to deprecation warning. Re nvaccess#9720.

josephsl · 2019-06-12T15:55:23Z

Hi,

Note that some tests will throw errors because itertools.izip is missing in Python 3. I'll correct this issue in a follow-up pull request later today.

Thanks.

LeonarddeR · 2019-06-12T19:06:01Z


 	def test_abstractProperty(self):
-		self.assertRaisesRegexp(TypeError,
+		# #9720 (Py3 review required): self.assertRaisesRegexp is deprecated.


I think this is just a rename, so this comment isn't necessary to go in eventually.

LeonarddeR · 2019-06-12T19:06:25Z


 	def test_subclassedAbstractProperty(self):
-		self.assertRaisesRegexp(TypeError,
+		# #9720 (Py3 review required): self.assertRaisesRegexp is deprecated.


LeonarddeR · 2019-06-12T19:07:40Z

 	def test_roleLabels(self):
 		"""Test to check whether every role has its own label in controlTypes.roleLabels"""
-		for name, const in controlTypes.__dict__.iteritems():
+		for name, const in controlTypes.__dict__.items():


I think I prefer this to be changed as follows while at it

Suggested change

for name, const in controlTypes.__dict__.items():

for name, const in vars(controlTypes).items():

LeonarddeR · 2019-06-12T19:07:57Z

 	def test_positiveStateLabels(self):
 		"""Test to check whether every state has its own label in controlTypes.stateLabels"""
-		for name, const in controlTypes.__dict__.iteritems():
+		for name, const in controlTypes.__dict__.items():


LeonarddeR · 2019-06-12T19:11:33Z

 		actual = list(self.reg.handlers)
 		self.assertEqual(actual, [inst.method])

+	# #9720 (Py3 review required): for some reason, this test keeps failing, so mark this as expected failure for now.


Please add a comment to the register method in extensionPoints. That code really needs a revisit.

In Python3, unbound methods no longer exist. Rather, fetching a method from a class directly (I.e. not from an instance) gives you back a normal function.
In the case of register: it will handle these as normal functions, and succeed.
I don't believe this would cause any problems for us: the handler will be called later like any normal function, as far as it makes sense to call an unbound method function. I.e. the code registering this function would have to pass in self as one of its arguments to make the function work in any meaningful way.
We could argue that we should disallow registering unbound method functions still, but there is no way of detecting these -- they are just functions.
I am happy to leave this in the code as a disabled test and approve/merge this as is. Though I would be interested in thoughts from @jcsteh on this. Should we just remove the test?

LeonarddeR · 2019-06-12T19:12:28Z

 		self.assertEqual(script_test.__doc__, "description")
 		self.assertEqual(script_test.category, SCRCAT_MISC)
-		self.assertItemsEqual(script_test.gestures, ["kb:a", "kb:b", "kb:c"])
+		# #9720 (Py3 review required): self.assertItemsEqual -> self.assertCountEqual.


Again as above, I don't think this should be in the eventual code of this pr if we just take the rename for granted.

… Re nvaccess#9720.

…ess#9720.

josephsl · 2019-06-12T19:48:32Z

Hi, done. Thanks. From: Leonard de Ruijter <notifications@github.com> Sent: Wednesday, June 12, 2019 12:13 PM To: nvaccess/nvda <nvda@noreply.github.com> Cc: Joseph Lee <joseph.lee22590@gmail.com>; Author <author@noreply.github.com> Subject: Re: [nvaccess/nvda] Unittests/Python 3: update syntax, catch unbound method registration test in extension points as expected failure (#9726) @LeonarddeR requested changes on this pull request.

_____ In tests/unit/test_baseObject.py <#9726 (comment)> :

@@ -137,14 +137,16 @@ class TestAbstractAutoPropertyObjects(unittest.TestCase):

""" def test_abstractProperty(self): - self.assertRaisesRegexp(TypeError, + # #9720 (Py3 review required): self.assertRaisesRegexp is deprecated. I think this is just a rename, so this comment isn't necessary to go in eventually.

_____ In tests/unit/test_baseObject.py <#9726 (comment)> :

"^Can't instantiate abstract class AutoPropertyObjectWithAbstractProperty "

"with abstract methods x", AutoPropertyObjectWithAbstractProperty ) def test_subclassedAbstractProperty(self): - self.assertRaisesRegexp(TypeError, + # #9720 (Py3 review required): self.assertRaisesRegexp is deprecated. See above

_____ In tests/unit/test_controlTypes.py <#9726 (comment)> :

@@ -15,13 +15,13 @@ class TestLabels(unittest.TestCase):

def test_roleLabels(self): """Test to check whether every role has its own label in controlTypes.roleLabels""" - for name, const in controlTypes.__dict__.iteritems(): + for name, const in controlTypes.__dict__.items(): I think I prefer this to be changed as follows while at it ⬇️ Suggested change - for name, const in controlTypes.__dict__.items(): + for name, const in vars(controlTypes).items():

_____ In tests/unit/test_controlTypes.py <#9726 (comment)> :

if name.startswith("ROLE_"):

self.assertIsNotNone(controlTypes.roleLabels.get(const),msg="{name} has no label".format(name=name)) def test_positiveStateLabels(self): """Test to check whether every state has its own label in controlTypes.stateLabels""" - for name, const in controlTypes.__dict__.iteritems(): + for name, const in controlTypes.__dict__.items(): See above

_____ In tests/unit/test_extensionPoints.py <#9726 (comment)> :

@@ -335,6 +335,8 @@ def test_registerInstanceMethod(self):

actual = list(self.reg.handlers) self.assertEqual(actual, [inst.method]) + # #9720 (Py3 review required): for some reason, this test keeps failing, so mark this as expected failure for now. Please add a comment to the register method in extensionPoints. That code really needs a revisit.

_____ In tests/unit/test_scriptHandler.py <#9726 (comment)> :

@@ -29,7 +29,8 @@ def script_test(self, gesture):

self.assertEqual(script_test.__doc__, "description") self.assertEqual(script_test.category, SCRCAT_MISC) - self.assertItemsEqual(script_test.gestures, ["kb:a", "kb:b", "kb:c"]) + # #9720 (Py3 review required): self.assertItemsEqual -> self.assertCountEqual. Again as above, I don't think this should be in the eventual code of this pr if we just take the rename for granted. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#9726?email_source=notifications&email_token=AB4AXEGT4VEAGBJ57KRROWTP2FDDBA5CNFSM4HXKNMDKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3LNNPA#pullrequestreview-248960700> , or mute the thread <https://github.com/notifications/unsubscribe-auth/AB4AXEDEZG7BMI7ZQFZ2EZTP2FDDBANCNFSM4HXKNMDA> .

michaelDCurran · 2019-06-13T23:44:33Z

 		actual = list(self.reg.handlers)
 		self.assertEqual(actual, [inst.method])

+	# #9720 (Py3 review required): for some reason, this test keeps failing, so mark this as expected failure for now.


In Python3, unbound methods no longer exist. Rather, fetching a method from a class directly (I.e. not from an instance) gives you back a normal function.
In the case of register: it will handle these as normal functions, and succeed.
I don't believe this would cause any problems for us: the handler will be called later like any normal function, as far as it makes sense to call an unbound method function. I.e. the code registering this function would have to pass in self as one of its arguments to make the function work in any meaningful way.
We could argue that we should disallow registering unbound method functions still, but there is no way of detecting these -- they are just functions.
I am happy to leave this in the code as a disabled test and approve/merge this as is. Though I would be interested in thoughts from @jcsteh on this. Should we just remove the test?

josephsl added 10 commits June 12, 2019 08:43

Unittest/Python 3: update copyright years. Re nvaccess#9720.

3534581

Unittest/test driver/Python 3: no more unicode/decoding, proper relat…

2d9ea54

…ive import. Re nvaccess#9720.

Unittest/base object tests/Python 3: proper relative import. Re nvacc…

d63cc4a

…ess#9720.

Unittest/braille tests/Python 3: proper relative import. Re nvaccess#…

0d833ea

…9720.

Unittest/braille tables/Python 3: dict.itervalues -> dict.values. Re n…

641d355

…vaccess#9720.

Unittest/control types/Python 3: dict.iteritems -> dict.items. Re nva…

e60fb82

…ccess#9720.

Unittest/text provider/Python 3: just use text as given instead of co…

d55aaa9

…nverting it into Unicode first. Re nvaccess#9720.

Unittest/script handler/Python 3: assertItemsEqual -> assertCountEqua…

87fffcc

…l. Re nvaccess#9720. Python 2's unittest documentation states self.assertItemsEqual is replaced by self.assertCountEqual in Python 3, thus follow this directive.

Unittest/extension points/Python 3: mark unbound method registration …

f85ffbe

…test as expected failure. Re nvaccess#9720. For some reason unbound method registration test keeps failing in Python 3. Until this is resolved, mark this test as expected failure.

Unittest/base object/Python 3: self.assertRaisesRegexp -> self.assert…

741d130

…RaisesRegex due to deprecation warning. Re nvaccess#9720.

josephsl requested a review from michaelDCurran June 12, 2019 15:54

LeonarddeR suggested changes Jun 12, 2019

View reviewed changes

josephsl added 3 commits June 12, 2019 12:25

Unittests/review action: remove unnecessary comments. Re nvaccess#9720.

59f0b6f

Unittests/control types: contorlTypes.__dict__ -> vars(contorlTypes).…

46ed14e

… Re nvaccess#9720.

Extension points/util: add a comment about failing unittest. Re nvacc…

629653b

…ess#9720.

LeonarddeR approved these changes Jun 13, 2019

View reviewed changes

michaelDCurran approved these changes Jun 13, 2019

View reviewed changes

michaelDCurran merged commit e6fa058 into nvaccess:threshold_py3_staging Jun 13, 2019

nvaccessAuto added this to the 2019.3 milestone Jun 13, 2019

LeonarddeR pushed a commit to LeonarddeR/nvda that referenced this pull request Jun 21, 2019

Revert expected failure from nvaccess#9726

2c74ef4

LeonarddeR mentioned this pull request Jun 28, 2019

Tests/Python 3: update unittest syntax #9720

Closed

josephsl mentioned this pull request Jul 23, 2019

What's new and readme: we are moving to Python 3.7 #9942

Merged

josephsl deleted the py3unittest branch September 16, 2019 02:00

Navigation Menu

Uh oh!

Uh oh!

Unittests/Python 3: update syntax, catch unbound method registration test in extension points as expected failure - #9726

Unittests/Python 3: update syntax, catch unbound method registration test in extension points as expected failure#9726
michaelDCurran merged 13 commits into
nvaccess:threshold_py3_stagingfrom
josephsl:py3unittest

josephsl commented Jun 12, 2019

Uh oh!

josephsl commented Jun 12, 2019

Uh oh!

LeonarddeR Jun 12, 2019

Uh oh!

LeonarddeR Jun 12, 2019

Uh oh!

LeonarddeR Jun 12, 2019

Uh oh!

LeonarddeR Jun 12, 2019

Uh oh!

LeonarddeR Jun 12, 2019

Uh oh!

michaelDCurran Jun 13, 2019

Uh oh!

LeonarddeR Jun 12, 2019

Uh oh!

josephsl commented Jun 12, 2019 via email

Uh oh!

michaelDCurran Jun 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	for name, const in controlTypes.__dict__.items():
	for name, const in vars(controlTypes).items():

Uh oh!

Uh oh!

Conversation

josephsl commented Jun 12, 2019

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

josephsl commented Jun 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephsl commented Jun 12, 2019 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants