bpo-37008: make mock_open handle able to honor next() #13492

Anvil · 2019-05-22T11:23:52Z

I've reported the issue on https://bugs.python.org/issue37008 and now I'm trying to bring a solution to this minor issue.

I think it could be trivially backported to 3.7 branch.

https://bugs.python.org/issue37008

the-knights-who-say-ni · 2019-05-22T11:23:56Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Our records indicate we have not received your CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

mariocj89 · 2019-05-22T12:54:01Z

Can you add a test that reproduces the issue today and shows it is fixed (to make sure this does not break in the future). The example you have in the issue is good, you can just add it to the testmock.py file.

You also need to add a news entry for this change, something in the lines of:

Add support for calling next with the mock resulting from :func:unittest.mock.mock_open.

Great fix by the way! :)

tirkarthi

Please also test __next__ along with next(mock) and also your next calls after exhausting state[0] raises StopIteration so add a test for that too.

I would prefer having these tests grouped under the class for reference

cpython/Lib/unittest/test/testmock/testwith.py

Line 142 in 33e71e0

class TestMockOpen(unittest.TestCase):

This is a code change and would require CLA. Please sign it. Change requires a NEWS entry : https://devguide.python.org/committing/?highlight=news#what-s-new-and-news-entries

Thanks

Anvil · 2019-05-22T13:32:51Z

@mariocj89 @tirkarthi thank you for your support and your advice. It's been appreciated. I've updated the NEWS and the unittests.

Sorry about the push force, btw, I realized in the mean time i was still using an old user.email to commit and had to amend the commits.

Anvil · 2019-05-22T13:45:34Z

CLA has been signed, btw.

tirkarthi · 2019-05-22T13:49:51Z

Lib/unittest/test/testmock/testmock.py

+    def test_mock_open_using_next(self):
+        mocked_open = mock.mock_open(read_data='1st line\n2nd line\n3rd line')
+        f1 = mocked_open('a-name')
+        self.assertEqual(next(f1), '1st line\n')


Personally, grouping the expected data with variables from assert calls reads nice.

line1 = next(f1) line2 = f1.__next__() # line 3 self.assertEqual(line1, '1st line\n') # assert for line 2 # assert for line 3

Also add a test for StopIteration after exhausting f1. You can use

with self.assertRaises(StopIteration): next(f1)

Would prefer moving it to TestMockOpen for these tests but would wait for other's opinion on this

I've applied the changed you've suggested and also added the missing StopIteration test.

Also, would it be more consistent to test __next__() prior to next() ?

Sorry I implied moving the testmock test to testwith. It seems like two copies.

Yeah, I've understood that :) but since I see some other already-redundant tests in both files and and since @mariocj89 said testmock and you said testwith.py, I've added both. No offense but I dont know either of you two so I honestly dont who is right. :)

Well, it was just an opinion of mine that to have tests grouped under the class. It's up to the core dev who will be merging this.

Anvil · 2019-05-22T13:52:30Z

I've added tests to both testmock.py and testwith.py.

…Equal lines together

mariocj89 · 2019-05-22T14:42:43Z

Misc/NEWS.d/next/Library/2019-05-22-15-26-08.bpo-37008.WPbv31.rst

@@ -0,0 +1,2 @@
+Add support for calling `next` with the mock resulting from


Sorry, I should have been more specific. This should probably be :func:`next` or double backtick.
I'd suggest :func:`next`

My bad. Fixed.

mariocj89

Looking great IMHO! Thanks, great fix for mock_open.

asvetlov

LGTM

miss-islington · 2019-05-23T10:03:30Z

Thanks @Anvil for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

asvetlov · 2019-05-23T10:03:33Z

Thanks!

bedevere-bot · 2019-05-23T10:04:05Z

GH-13521 is a backport of this pull request to the 3.7 branch.

I've reported the issue on https://bugs.python.org/issue37008 and now I'm trying to bring a solution to this minor issue. I think it could be trivially backported to 3.7 branch. https://bugs.python.org/issue37008 (cherry picked from commit 394119a) Co-authored-by: Damien Nadé <Anvil@users.noreply.github.com>

tirkarthi · 2019-05-23T10:33:22Z

@cjw296 __next__ doesn't seem to exist on python 2 and this might need to be python 3 only code during backport. Perhaps the test could be marked as python 3 only such in the backport test suite.

import mock

mocked_open = mock.mock_open(read_data='one\n')
f1 = mocked_open('a-name')
line1 = f1.__next__()

mock with this patch applied to backport

python /tmp/foo.py
Traceback (most recent call last):
  File "/tmp/foo.py", line 3, in <module>
    mocked_open = mock.mock_open(read_data='one\n')
  File "/Users/karthikeyansingaravelan/stuff/python/27-venv/lib/python2.7/site-packages/mock/mock.py", line 2567, in mock_open
    handle.__next__.side_effect = _next_side_effect
  File "/Users/karthikeyansingaravelan/stuff/python/27-venv/lib/python2.7/site-packages/mock/mock.py", line 695, in __getattr__
    raise AttributeError("Mock object has no attribute %r" % name)
AttributeError: Mock object has no attribute '__next__'

Anvil · 2019-05-23T10:48:31Z

Thank you everyone.

asvetlov · 2019-05-23T10:51:25Z

@tirkarthi good point.
I think this is not a reason to reject the PR though :)

tirkarthi · 2019-05-23T11:37:58Z

@asvetlov Chris is maintaining the mock backport in PyPI that supports Python 2 and hence I was just wanted to make a note of this for Python 2. I am happy with this improving Python 3 :)

the-knights-who-say-ni added the CLA not signed label May 22, 2019

bedevere-bot added the awaiting review label May 22, 2019

tirkarthi reviewed May 22, 2019

View reviewed changes

Anvil added 2 commits May 22, 2019 15:18

bpo-37008: unittest.mock.mock_open handle is now able to honor next()

2421832

bpo-37008: test mock_open with next and __next__

c35f1d6

tirkarthi reviewed May 22, 2019

View reviewed changes

Anvil added 4 commits May 22, 2019 15:53

bpo-37008: make mock_open handle able to honor next()

0437d87

test unittest.mock.mock_open and next() iteractions

a6947b0

assert that next can raise StopIteration on mock_open handle

001f989

improve test_mock_open_using_next readability by gathering all assert…

69372d7

…Equal lines together

mariocj89 reviewed May 22, 2019

View reviewed changes

fixup! bpo-37008: make mock_open handle able to honor next()

0cc78ca

mariocj89 approved these changes May 22, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels May 22, 2019

asvetlov approved these changes May 23, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels May 23, 2019

asvetlov removed the CLA not signed label May 23, 2019

the-knights-who-say-ni added the CLA signed label May 23, 2019

asvetlov added needs backport to 3.7 and removed CLA signed labels May 23, 2019

the-knights-who-say-ni added the CLA signed label May 23, 2019

asvetlov added the 🤖 automerge label May 23, 2019

miss-islington merged commit 394119a into python:master May 23, 2019

bedevere-bot removed the awaiting merge label May 23, 2019

bedevere-bot removed the needs backport to 3.7 label May 23, 2019

Anvil deleted the next_mock_open branch May 23, 2019 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-37008: make mock_open handle able to honor next() #13492

bpo-37008: make mock_open handle able to honor next() #13492

Anvil commented May 22, 2019 •

edited by bedevere-bot

the-knights-who-say-ni commented May 22, 2019

mariocj89 commented May 22, 2019 •

edited

tirkarthi left a comment

Anvil commented May 22, 2019

Anvil commented May 22, 2019

tirkarthi May 22, 2019

tirkarthi May 22, 2019

Anvil May 22, 2019

Anvil May 22, 2019

tirkarthi May 22, 2019

Anvil May 22, 2019

tirkarthi May 22, 2019

Anvil commented May 22, 2019

mariocj89 May 22, 2019

Anvil May 22, 2019

mariocj89 left a comment

asvetlov left a comment

miss-islington commented May 23, 2019

asvetlov commented May 23, 2019

bedevere-bot commented May 23, 2019

tirkarthi commented May 23, 2019

Anvil commented May 23, 2019

asvetlov commented May 23, 2019

tirkarthi commented May 23, 2019

		@@ -0,0 +1,2 @@
		Add support for calling `next` with the mock resulting from

bpo-37008: make mock_open handle able to honor next() #13492

bpo-37008: make mock_open handle able to honor next() #13492

Conversation

Anvil commented May 22, 2019 • edited by bedevere-bot

the-knights-who-say-ni commented May 22, 2019

mariocj89 commented May 22, 2019 • edited

tirkarthi left a comment

Choose a reason for hiding this comment

Anvil commented May 22, 2019

Anvil commented May 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anvil commented May 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariocj89 left a comment

Choose a reason for hiding this comment

asvetlov left a comment

Choose a reason for hiding this comment

miss-islington commented May 23, 2019

asvetlov commented May 23, 2019

bedevere-bot commented May 23, 2019

tirkarthi commented May 23, 2019

Anvil commented May 23, 2019

asvetlov commented May 23, 2019

tirkarthi commented May 23, 2019

Anvil commented May 22, 2019 •

edited by bedevere-bot

mariocj89 commented May 22, 2019 •

edited