unittest.mock._Call ignores name parameter

[jiajunhuang]

BPO	28961
Nosy	@vstinner, @voidspace, @berkerpeksag, @Vgr255, @jiajunhuang
PRs	bpo-28961: Address my comments from earlier code review #305 [3.6] bpo-28961: Address my comments from earlier code review #306 [Do Not Merge] Convert Misc/NEWS so that it is managed by towncrier #552
Files	mock_class_call.patch mock.patch mock.patch mock.patch mock.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2017-02-26.13:06:44.013>
created_at = <Date 2016-12-13.13:47:48.153>
labels = ['3.7', 'type-bug', 'library']
title = 'unittest.mock._Call ignores `name` parameter'
updated_at = <Date 2017-03-31.16:36:27.514>
user = 'https://github.com/jiajunhuang'

bugs.python.org fields:

activity = <Date 2017-03-31.16:36:27.514>
actor = 'dstufft'
assignee = 'none'
closed = True
closed_date = <Date 2017-02-26.13:06:44.013>
closer = 'berker.peksag'
components = ['Library (Lib)']
creation = <Date 2016-12-13.13:47:48.153>
creator = 'Jiajun Huang'
dependencies = []
files = ['45883', '45884', '45885', '45922', '46126']
hgrepos = []
issue_num = 28961
keywords = ['patch']
message_count = 19.0
messages = ['283104', '283120', '283157', '283159', '283164', '283231', '283235', '283369', '284246', '284564', '284572', '284589', '284595', '284823', '284839', '284840', '284885', '290393', '290395']
nosy_count = 6.0
nosy_names = ['vstinner', 'michael.foord', 'python-dev', 'berker.peksag', 'abarry', 'Jiajun Huang']
pr_nums = ['305', '306', '552']
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue28961'
versions = ['Python 3.5', 'Python 3.6', 'Python 3.7']

[jiajunhuang]

code in _Call.__new__:

      def __new__(cls, value=(), name=None, parent=None, two=False,                                                                    
      ¦   ¦   ¦   from_kall=True):                                                                                                     
      ¦   name = ''                                                                                                                    
      ¦   args = ()                                                                                                                    
      ¦   kwargs = {}                                                                                                                  
      ¦   _len = len(value)                                                                                                            
      ¦   if _len == 3:                                                                                                                
              ...

the parameter name had been override since the function starts. so whatever name is, it's been ignored. Is it a bug? or something else?

[Vgr255]

That indeed looks like a bug. Well spotted :)

That code has been there since unittest.mock was added back in March 2012. If I were to guess, I'd say that it should be if name is None: name = ''. Care to submit a patch?

[jiajunhuang]

Thanks for reply :) the patch has been uploaded.

[jiajunhuang]

update the patch file follow the doc(https://docs.python.org/devguide/gitdevs.html)

[jiajunhuang]

The new patch has been updated. :)

[berkerpeksag]

Thanks for the patch, Jiajun. The _Call class is tested in CallTest (located in Lib/unittest/test/testmock/testhelpers.py) Would it be possible to add a test case to make sure that we actually fixed the bug?

[jiajunhuang]

I think we can write _Call.__new__ as:

def __new__(cls, value=(), name='',...)

it's much simpler and readable.

[jiajunhuang]

code and test case has been updated.

[jiajunhuang]

hi, do this need more test case or something else to be merged? please let me know :)

[vstinner]

Please remove "import ipdb; ipdb.set_trace() # TODO remove it" before posting patches ;-)

[jiajunhuang]

sorry about that, fixed.

[berkerpeksag]

The patch looks good to me. There are some styling issues in the test code (e.g. no need to add trailing spaces after commas in some cases), but I can take care of them before committing the patch :) Thanks!

[vstinner]

The latest patch (Jiajun Huang, 2017-01-03 14:34) LGTM.

[voidspace]

Yep, LGTM as well. Nicely spotted!

[python-dev]

New changeset 50424a903593 by Victor Stinner in branch '3.6':
Fix unittest.mock._Call: don't ignore name
https://hg.python.org/cpython/rev/50424a903593

[vstinner]

I applied the latest mock.patch to Python 3.6 and default (future 3.7). I prefer to wait for the 3.5.3 release before backporting the fix to 3.5, the fix is minor, I don't want to annoy the release manager yet.

[berkerpeksag]

IIRC 3.5.3rc1 is already tagged so the 3.5 branch is open for 3.5.4. Anything that needs to be in 3.5.3 should be cherry-picked by the RM at this point if I'm not mistaken (and note that I don't have time check, but 3.5.3 may be the last bugfix release of 3.5 series so you may just skip 3.5 :))

Like I already said in msg284589, the test code doesn't follow the current style in Lib/unittest/test/testmock/testhelpers.py and it can be simplified like the following patch:

     def test_call_with_name(self):
-        self.assertEqual(
-            'foo',
-            _Call((), 'foo')[0],
-        )
-        self.assertEqual(
-            '',
-            _Call((('bar', 'barz'), ), )[0]
-        )
-        self.assertEqual(
-            '',
-            _Call((('bar', 'barz'), {'hello': 'world'}), )[0]
-        )
+        self.assertEqual(_Call((), 'foo')[0], 'foo')
+        self.assertEqual(_Call((('bar', 'barz'),),)[0], '')
+        self.assertEqual(_Call((('bar', 'barz'), {'hello': 'world'}),)[0], '')

[berkerpeksag]

New changeset fae59e1 by Berker Peksag in branch '3.6':
bpo-28961: Address my comments from earlier code review (#306)
fae59e1

[berkerpeksag]

New changeset 5aa3856 by Berker Peksag in branch 'master':
bpo-28961: Address my comments from earlier code review (#305)
5aa3856