classification
Title: mock_open data is visible only once for the life of the class
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6, Python 3.4, Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rbcollins Nosy List: berker.peksag, michael.foord, pkoning, python-dev, quasipedia, rbcollins
Priority: normal Keywords: patch

Created on 2014-06-13 18:14 by pkoning, last changed 2015-07-25 18:33 by rbcollins. This issue is now closed.

Files
File name Uploaded Description Edit
mock-280.py rbcollins, 2015-07-14 20:13 reproduction example.
issue-21750.patch rbcollins, 2015-07-15 00:47 patch vs 3.4
issue-21750-2.patch rbcollins, 2015-07-21 19:17
Messages (23)
msg220474 - (view) Author: Paul Koning (pkoning) Date: 2014-06-13 18:14
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.
msg237352 - (view) Author: Mac Ryan (quasipedia) Date: 2015-03-06 13:46
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.
msg246739 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-14 20:13
This is a regression in 3.5 vs 3.3 I think. It would have worked with the initial mock import.

https://github.com/testing-cabal/mock/issues/280

Minimal reproducer case attached (With commentted out bits to switch out the mock for the rolling backport etc).
msg246749 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-15 00:23
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.
msg246840 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-07-17 05:10
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')).
msg246842 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-17 07:08
There are already explicit tests for that, do you want another one?
msg246843 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-07-17 07:16
> There are already explicit tests for that

Great, then the test is fine :) Thanks for writing the patch.
msg246844 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-17 07:33
Ok, so - good to commit to 3.4 and up?
msg246848 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-07-17 08:11
New changeset 41d55ac50dea by Robert Collins in branch '3.4':
Issue #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 #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 #21750: mock_open.read_data can now be read from each instance, as it
https://hg.python.org/cpython/rev/92a90e469424
msg246866 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-17 19:44
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. 

https://github.com/testing-cabal/mock/issues/288
msg247049 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-21 19:17
Fixup patch. I've tested this with the reported failures and they all work.
msg247050 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-21 19:20
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 ''.
msg247058 - (view) Author: Paul Koning (pkoning) Date: 2015-07-21 19:48
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.
msg247061 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-21 20:18
@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
msg247076 - (view) Author: Paul Koning (pkoning) Date: 2015-07-21 23:54
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.
msg247087 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-22 04:23
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'.
msg247119 - (view) Author: Paul Koning (pkoning) Date: 2015-07-22 14:41
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.
msg247121 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-22 14:43
Which part of the docs specifically?
msg247127 - (view) Author: Paul Koning (pkoning) Date: 2015-07-22 15:04
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.
msg247194 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2015-07-23 13:34
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.
msg247195 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-23 13:42
@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.
msg247213 - (view) Author: Robert Collins (rbcollins) * (Python committer) Date: 2015-07-23 15:00
Discussed with Michael Foord; we're going to go with the -2 patch - the new behaviour.
msg247219 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-07-23 15:49
New changeset 83e28ee348bf by Robert Collins in branch '3.4':
Issue #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 #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 #21750: Further fixup to be styled like other mock APIs.
https://hg.python.org/cpython/rev/c896ab62ac75
History
Date User Action Args
2015-07-25 18:33:38rbcollinssetstatus: open -> closed
resolution: fixed
stage: needs patch -> resolved
2015-07-23 15:49:29python-devsetmessages: + msg247219
2015-07-23 15:00:32rbcollinssetmessages: + msg247213
2015-07-23 13:42:42rbcollinssetmessages: + msg247195
2015-07-23 13:34:19michael.foordsetmessages: + msg247194
2015-07-22 15:04:59pkoningsetmessages: + msg247127
2015-07-22 14:43:05rbcollinssetmessages: + msg247121
2015-07-22 14:41:30pkoningsetmessages: + msg247119
2015-07-22 04:23:07rbcollinssetmessages: + msg247087
2015-07-21 23:54:34pkoningsetmessages: + msg247076
2015-07-21 20:18:36rbcollinssetmessages: + msg247061
2015-07-21 19:48:06pkoningsetmessages: + msg247058
2015-07-21 19:20:28rbcollinssetmessages: + msg247050
2015-07-21 19:17:44rbcollinssetfiles: + issue-21750-2.patch

messages: + msg247049
2015-07-17 19:44:14rbcollinssetmessages: + msg246866
stage: commit review -> needs patch
2015-07-17 08:11:12python-devsetnosy: + python-dev
messages: + msg246848
2015-07-17 07:33:35rbcollinssetmessages: + msg246844
2015-07-17 07:16:59berker.peksagsetmessages: + msg246843
2015-07-17 07:08:45rbcollinssetmessages: + msg246842
2015-07-17 05:10:26berker.peksagsetmessages: + msg246840
stage: patch review -> commit review
2015-07-17 04:55:14berker.peksagsetstage: patch review
2015-07-15 00:47:46rbcollinssetfiles: + issue-21750.patch
keywords: + patch
2015-07-15 00:23:36rbcollinssetmessages: + msg246749
2015-07-15 00:09:32rbcollinssetassignee: rbcollins
versions: + Python 3.5, Python 3.6
2015-07-14 20:13:44rbcollinssetfiles: + mock-280.py
nosy: + rbcollins
messages: + msg246739

2015-03-06 13:48:33berker.peksagsetnosy: + berker.peksag
2015-03-06 13:46:52quasipediasetnosy: + quasipedia
messages: + msg237352
2014-06-13 22:18:47ned.deilysetnosy: + michael.foord
2014-06-13 18:14:49pkoningcreate