mock_open data is visible only once for the life of the class

[pkoning]

BPO	21750
Nosy	@rbtcollins, @voidspace, @berkerpeksag
Files	mock-280.py: reproduction example. issue-21750.patch: patch vs 3.4 issue-21750-2.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 = 'https://github.com/rbtcollins'
closed_at = <Date 2015-07-25.18:33:38.884>
created_at = <Date 2014-06-13.18:14:49.141>
labels = ['type-bug', 'library']
title = 'mock_open data is visible only once for the life of the class'
updated_at = <Date 2015-07-25.18:33:38.883>
user = 'https://bugs.python.org/pkoning'

bugs.python.org fields:

activity = <Date 2015-07-25.18:33:38.883>
actor = 'rbcollins'
assignee = 'rbcollins'
closed = True
closed_date = <Date 2015-07-25.18:33:38.884>
closer = 'rbcollins'
components = ['Library (Lib)']
creation = <Date 2014-06-13.18:14:49.141>
creator = 'pkoning'
dependencies = []
files = ['39926', '39928', '39967']
hgrepos = []
issue_num = 21750
keywords = ['patch']
message_count = 23.0
messages = ['220474', '237352', '246739', '246749', '246840', '246842', '246843', '246844', '246848', '246866', '247049', '247050', '247058', '247061', '247076', '247087', '247119', '247121', '247127', '247194', '247195', '247213', '247219']
nosy_count = 6.0
nosy_names = ['rbcollins', 'michael.foord', 'python-dev', 'quasipedia', 'berker.peksag', 'pkoning']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue21750'
versions = ['Python 3.4', 'Python 3.5', 'Python 3.6']

[pkoning]

The read_data iterator that supplies bits of read data when asked from unittest.mock.mock_open is a class attribute. The result is that, if you instantiate the class multiple times, that iterator is shared. This isn't documented and it seems counterintuitive. The purpose of mock_open is to act like a file, and read_data is that file's data. A file contains the same data each time you read it. So it would seem better for the data iterator to be an instance attribute, initialized from the mock_open read_data value each time mock_open is called to create a new file instance.

[quasipedia]

I'm a bit lost in the interface of this bugtracker, so I apologise if I am writing in the wrong spot, anyhow...

I just wanted to signal that this bug breaks the inter-operability between mock (external library) and unittest.mock, breaking tests that have been originally written for Python2.

[rbtcollins]

This is a regression in 3.5 vs 3.3 I think. It would have worked with the initial mock import.

testing-cabal/mock#280

Minimal reproducer case attached (With commentted out bits to switch out the mock for the rolling backport etc).

[rbtcollins]

I think its worth noting that both the original mock_open and the new one are entirely broken for mocking access to multiple files.

But, the breakage was not deliberate, and as the mock-280 example shows, folk subclassing a test suite will be surprisingly broken vs the long table releases of mock itself.

So, I think its ok to fix this - relying on the second file appearing empty is IMO unlikely, and being more compatible with the prior releases of mock is good.

We probably need to rethink this interface and provide a better one though, so that you can mock a filesystem easily. Thats a different discussion though.

[berkerpeksag]

LGTM.

A minor comment:

+ def test_mock_open_reuse_issue_21750(self):

We can also add an assert to check the data is actually "data" (e.g. assertEqual(f1.read(), 'data')).

[rbtcollins]

There are already explicit tests for that, do you want another one?

[berkerpeksag]

There are already explicit tests for that

Great, then the test is fine :) Thanks for writing the patch.

[rbtcollins]

Ok, so - good to commit to 3.4 and up?

[python-dev]

New changeset 41d55ac50dea by Robert Collins in branch '3.4':
Issue bpo-21750: mock_open.read_data can now be read from each instance, as it
https://hg.python.org/cpython/rev/41d55ac50dea

New changeset 0da764c58322 by Robert Collins in branch '3.5':
Issue bpo-21750: mock_open.read_data can now be read from each instance, as it
https://hg.python.org/cpython/rev/0da764c58322

New changeset 92a90e469424 by Robert Collins in branch 'default':
Issue bpo-21750: mock_open.read_data can now be read from each instance, as it
https://hg.python.org/cpython/rev/92a90e469424

[rbtcollins]

The fix for this uncovered more testing / scenarios that folk use mock_open for that were not accounted for. I'm reverting it from mock, and am going to roll-forward for Python: I should have a fix in a day or two and we can fix it more completely then.

testing-cabal/mock#288

[rbtcollins]

Fixup patch. I've tested this with the reported failures and they all work.

[rbtcollins]

But - its worth discussing. Perhaps we should roll this all back, and just say 'use a vfs layer for tests like this'. The problem in doing that, is that the
@patch
def test_foo...

use case is actually pretty common IME, and this conflicts with the
@patch
...
A = open()
B = open()
A.read()
B.read()

case where the reset in the open() side effect will cause B.read to end up returning ''.

[pkoning]

Sure, you can use a vfs. That's true for a lot of mock functions; the benefit of mock, including mock_open, is that it provides an easier and better packaged way. The behavior expected is "be like a file". So in that last example, if you open it twice, you've got two views of the same "file" data, independent of each other, and reads of each file will see that data in full. A.read() has no effect on B.read().

I don't think I understand what the latest discussion/issue is all about.

[rbtcollins]

@pkoning in Python3.3 == mock 1.0.1, 
>>> m = mock_open(read_data='f')
>>> m().read()
'f'
>>> m().read()
'f'
>>> x = m()
>>> x.read()
'f'
>>> x.read()
'f'
>>> x = m()
>>> y = m()
>>> x.read()
'f'
>>> y.read()
'f'

in 3.4 == mock 1.1.{0,1,2,3}, and 1.2.0
>>> m = mock_open(read_data='f')
>>> m().read()
'f'
>>> m().read()
''
>>> x = m()
>>> x.read()
''
>>> x.read()
''
>>> x = m()
>>> y = m()
>>> x.read()
'f'
>>> y.read()
''

Right now, in 3.5==mock 1.1.4
>>> m = mock_open(read_data='f')
>>> m().read()
'f'
>>> m().read()
'f'
>>> x = m()
>>> x.read()
'f'
>>> x.read()
''
>>> x = m()
>>> y = m()
>>> x.read()
'f'
>>> y.read()
'f'


With the patch I just attached:
>>> m = mock_open(read_data='f')
>>> m().read()
'f'
>>> m().read()
'f'
>>> x = m()
>>> x.read()
'f'
>>> x.read()
''
>>> x = m()
>>> y = m()
>>> x.read()
'f'
>>> y.read()
''

All different points in the solution space :)
HTH

[pkoning]

So if I understand right, it seems to me the 3.5/mock 1.1.4 behavior is correct. mock_open(read_data="f") acts like a file that contains f, and m() acts like an open() of that file. So if I call open once, I should read the f, then EOF. If I open twice, then each stream (x and y) is independent, and each sees f then EOF.

[rbtcollins]

So the 1.1.4 behaviour matches that of a VFS most closely. But, see the earlier messages, it does do only and precisely because it breaks regular mock idioms. Thus I think we're better off with the new patch, which addresses the issue with reuse of the mocks in subclassed tests(with the patch decorator), as well as with repeated opens, while still preserving the basic structure and feel of 'Mock'.

[pkoning]

I suppose. And it certainly is much better than the original behavior. But if this is the approach chosen, it should be clearly called out in the documentation, because the current wording suggests the 1.1.4 behavior, not the one you recommended.

[rbtcollins]

Which part of the docs specifically?

[pkoning]

Section 26.7.7 of the library manual describes mock_open with the words:

A helper function to create a mock to replace the use of open. It works for open called directly or used as a context manager.

which implies that it works just like open. Given that it doesn't (not if you do two opens and use both streams concurrently) that difference should be called out as a difference, or limitation.

[voidspace]

So the problem with the testing-cabal issue 280 is *really* a problem with decorators - the decorator is applied at method creation time and mock_open is only called once rather than once *per call*.

Better would be to use mock.patch as a context manager inside the test, so that mock_open is (correctly) called each time.

From a purist point of view I think that the Python 3.5==mock 1.1.4 behaviour is *better*. Whether that's enough justification to break existing code is a difficult question.

[rbtcollins]

@paul

So the problem is that its never been a high fidelity thing in that sense.... In that:
3.3 -> read() is a constant for the thing opened from the mock
3.4 -> read() works once and only once across all opened files from the mock
3.5 today -> read() works once for *each* opened file from the mock, but you *can't access* each file object (because the mock.returnvalue is replaced each time, which is unmocklike)

With this patch: -> read() works once for each opened file, as long as the sequence open -> read -> open -> read is followed, and mock.returnvalue is a constant, mocklike.

[rbtcollins]

Discussed with Michael Foord; we're going to go with the -2 patch - the new behaviour.

[python-dev]

New changeset 83e28ee348bf by Robert Collins in branch '3.4':
Issue bpo-21750: Further fixup to be styled like other mock APIs.
https://hg.python.org/cpython/rev/83e28ee348bf

New changeset b30fc1de006c by Robert Collins in branch '3.5':
Issue bpo-21750: Further fixup to be styled like other mock APIs.
https://hg.python.org/cpython/rev/b30fc1de006c

New changeset c896ab62ac75 by Robert Collins in branch 'default':
Issue bpo-21750: Further fixup to be styled like other mock APIs.
https://hg.python.org/cpython/rev/c896ab62ac75