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 ''.