mock_open()().readline() fails at EOF

[rbtcollins]

BPO	26807
Nosy	@rbtcollins, @yrobla
Files	issue26807.diff

^{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 2016-05-16.03:30:31.964>
created_at = <Date 2016-04-20.08:18:03.635>
labels = ['type-bug', 'library']
title = 'mock_open()().readline() fails at EOF'
updated_at = <Date 2016-05-16.03:30:31.963>
user = 'https://github.com/rbtcollins'

bugs.python.org fields:

activity = <Date 2016-05-16.03:30:31.963>
actor = 'rbcollins'
assignee = 'none'
closed = True
closed_date = <Date 2016-05-16.03:30:31.964>
closer = 'rbcollins'
components = ['Library (Lib)']
creation = <Date 2016-04-20.08:18:03.635>
creator = 'rbcollins'
dependencies = []
files = ['42590']
hgrepos = []
issue_num = 26807
keywords = ['patch']
message_count = 14.0
messages = ['263814', '263826', '263994', '264177', '264181', '264182', '264183', '264237', '264545', '265361', '265362', '265364', '265661', '265662']
nosy_count = 3.0
nosy_names = ['rbcollins', 'python-dev', 'yolanda.robla']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue26807'
versions = ['Python 3.5', 'Python 3.6']

[rbtcollins]

>>> import unittest.mock
>>> o = unittest.mock.mock_open(read_data="fred")
>>> f = o()
>>> f.read()
'fred'
>>> f.read()
''
>>> f.readlines()
[]
>>> f.readline()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/robertc/work/cpython/Lib/unittest/mock.py", line 935, in __call__
    return _mock_self._mock_call(*args, **kwargs)
  File "/home/robertc/work/cpython/Lib/unittest/mock.py", line 994, in _mock_call
    result = next(effect)
StopIteration

[yrobla]

diff --git a/Lib/unittest/mock.py b/Lib/unittest/mock.py
index 7400fb7..9e47cd2 100644
--- a/Lib/unittest/mock.py
+++ b/Lib/unittest/mock.py
@@ -991,7 +991,10 @@ class CallableMixin(Base):
                 raise effect
 
             if not _callable(effect):
-                result = next(effect)
+                try:
+                    result = next(effect)
+                except StopIteration:
+                    result = None
                 if _is_exception(result):
                     raise result
                 if result is DEFAULT:

[rbtcollins]

Thanks Yolanda, that looks sane - could you perhaps add a test for this in Lib/unittest/tests/test_mock/ ?

[yrobla]

diff --git a/Lib/unittest/test/testmock/testwith.py b/Lib/unittest/test/testmock/testwith.py
index a7bee73..efe6391 100644
--- a/Lib/unittest/test/testmock/testwith.py
+++ b/Lib/unittest/test/testmock/testwith.py
@@ -297,5 +297,16 @@ class TestMockOpen(unittest.TestCase):
         self.assertEqual(handle.readline(), 'bar')
 
 
+    def test_readlines_after_eof(self):
+        # Check that readlines does not fail after the end of file
+        mock = mock_open(read_data='foo')
+        with patch('%s.open' % __name__, mock, create=True):
+            h = open('bar')
+            h.read()
+            h.read()
+            data = h.readlines()
+            self.assertEqual(data, [])
+
+
 if __name__ == '__main__':
     unittest.main()

[rbtcollins]

Thanks Yolanda. I've touched up the test a little and run a full test run (make test) - sadly it fails: there is an explicit test that StopIteration gets raised if you set it as a side effect.

======================================================================
FAIL: test_mock_open_after_eof (unittest.test.testmock.testmock.MockTest)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/robertc/work/cpython-3.5.hg/Lib/unittest/test/testmock/test                                      mock.py", line 1428, in test_mock_open_after_eof
    self.assertEqual('', h.readline())
AssertionError: '' != None

======================================================================
FAIL: test_side_effect_iterator (unittest.test.testmock.testmock.MockTest )
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/robertc/work/cpython-3.5.hg/Lib/unittest/test/testmock/test                                      mock.py", line 980, in test_side_effect_iterator
    self.assertRaises(StopIteration, mock)
AssertionError: StopIteration not raised by <Mock id='140302027307384'>

======================================================================
FAIL: test_side_effect_setting_iterator (unittest.test.testmock.testmock. MockTest)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/robertc/work/cpython-3.5.hg/Lib/unittest/test/testmock/test                                      mock.py", line 1015, in test_side_effect_setting_iterator
    self.assertRaises(StopIteration, mock)
AssertionError: StopIteration not raised by <Mock id='140302027307720'>

---

Ran 705 tests in 1.251s

FAILED (failures=3, skipped=3)

Of those, I think the first failure is a bug in the patch; the second and third are genuine failures - you'll need to make your change in mock_open itself, not in 'mock'.

I've attached an updated patch which has ACKS, NEWS filled out and tweaked your test to be more comprehensive.

[yrobla]

So... the failures are because i'm capturing the StopIteration exception in that case. then it's normal that it's not raised. How should you solve that problem instead?

[rbtcollins]

You're changing _mock_call but readline is actually implemented in _readline_side_effect - just changing that should be all thats needed (to deal with StopIteration). You will however need to fixup the return values since the test I added is a bit wider than just the single defect you uncovered.

[yrobla]

I tried patching the readline_side_effect call. But i cannot trap StopIteration there, and i don't see any clear way to detect that the generator has reached its end at that level.

[yrobla]

How about...

@@ -2339,9 +2339,12 @@ def mock_open(mock=None, read_data=''):
         if handle.readline.return_value is not None:
             while True:
                 yield handle.readline.return_value
-        for line in _state[0]:
-            yield line
 
+        try:
+            while True:
+                yield next(_state[0])
+        except StopIteration:
+            yield ''

[rbtcollins]

./python Lib/unittest/test/testmock/testmock.py
..........................................s.....................................
----------------------------------------------------------------------
Ran 80 tests in 0.180s

OK (skipped=1)

So thats great. I am going to have to stare that this quite hard to be sure I understand why it fails without the change away from 'for line in ' - but yes, this is the right place, and I'll commit it to 3.5 and 3.6 and backport it to the external mock tomorrow.

Thanks!

[rbtcollins]

Actually, further inspection and a teddybear with Angus Lees uncovers this:

diff --git a/Lib/unittest/test/testmock/testmock.py b/Lib/unittest/test/testmock/testmock.py
--- a/Lib/unittest/test/testmock/testmock.py
+++ b/Lib/unittest/test/testmock/testmock.py
@@ -1419,6 +1419,18 @@
self.assertEqual('abc', first)
self.assertEqual('abc', second)

+ def test_mock_open_after_eof(self):
+ # read, readline and readlines should work after end of file.
+ _open = mock.mock_open(read_data='foo')
+ h = _open('bar')
+ h.read()
+ self.assertEqual('', h.read())
+ self.assertEqual('', h.read())
+ self.assertEqual('', h.readline())
+ self.assertEqual('', h.readline())
+ self.assertEqual([], h.readlines())
+ self.assertEqual([], h.readlines())
+
def test_mock_parents(self):
for Klass in Mock, MagicMock:
m = Klass()

./python Lib/unittest/test/testmock/testmock.py
..........................................s........E............................
======================================================================
ERROR: test_mock_open_after_eof (main.MockTest)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "Lib/unittest/test/testmock/testmock.py", line 1430, in test_mock_open_after_eof
    self.assertEqual('', h.readline())
  File "/home/robertc/work/cpython-3.5.hg/Lib/unittest/mock.py", line 917, in __call__
    return _mock_self._mock_call(*args, **kwargs)
  File "/home/robertc/work/cpython-3.5.hg/Lib/unittest/mock.py", line 976, in _mock_call
    result = next(effect)
StopIteration

---

Ran 80 tests in 0.197s

FAILED (errors=1, skipped=1)

That is, we need the yield '' to be infinite - while True: yield '', or the while True: to be outside the try:except.

[rbtcollins]

So something like

  for line in _state[0]:
      yield line
  while True:
    yield ''

Will probably do it just fine.

[python-dev]

New changeset cbb31b2a3dba by Robert Collins in branch '3.5':
Issue bpo-26807: mock_open 'files' no longer error on readline at end of file.
https://hg.python.org/cpython/rev/cbb31b2a3dba

New changeset 72a798e27117 by Robert Collins in branch 'default':
Issue bpo-26807: mock_open 'files' no longer error on readline at end of file.
https://hg.python.org/cpython/rev/72a798e27117