classification
Title: mock_open does not support iteration around text files.
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.8, Python 3.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: anthony-flury, bbayles, berker.peksag, eric.araujo, ezio.melotti, michael.foord, ned.deily, rbcollins
Priority: normal Keywords: patch

Created on 2018-02-24 08:58 by anthony-flury, last changed 2018-09-14 21:30 by berker.peksag. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 5974 merged anthony-flury, 2018-03-04 14:56
PR 5975 closed anthony-flury, 2018-03-04 15:43
PR 5976 closed anthony-flury, 2018-03-04 15:50
PR 5977 closed anthony-flury, 2018-03-04 16:43
PR 9311 merged miss-islington, 2018-09-14 17:03
Messages (16)
msg312706 - (view) Author: Anthony Flury (anthony-flury) * Date: 2018-02-24 08:58
Using the unittest.mock helper mock_open with multi-line read data, although readlines method will work on the mocked open data, the commonly used iterator idiom on an open file returns the equivalent of an empty file.

    from unittest.mock import mock_open

    read_data = 'line 1\nline 2\nline 3\nline 4\n'
    with patch('builtins.open', mock_open) as mocked:
       with open('a.txt', 'r') as fp:
           assert [l for l in StringIO(read_data)] ==
                  [l for l in fp] 

will fail although it will work on a normal file with the same data, and using [l for l in fp.readlines()] will also work.  

There is a relatively simple fix which I have a working local version - but I don't know how to provide that back to the library - or even if i should.
msg315025 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2018-04-06 16:49
Is this related to #33236 ?
msg315026 - (view) Author: Anthony Flury (anthony-flury) * Date: 2018-04-06 17:14
No - it isn't related.

In the case of mock_open; it isn't intended to be a simple MagicMock - it is meant to be a mocked version of open, and so to be useful as a testing tool, it should emulate a file as much as possible.  

When a mock_open is created, you can provide an argument 'read_data' which is meant to be the data from your mocked file, so it is key that the dunder iter method actually returns an iterator. The mock_open implementation already provides special versions of read, readline and readlines methods which use the 'read_data' initial value as the content.
Currently though the dunder iter method isn't set at all - so the returned value would currently be an empty iterator (which makes mock_open unable to be used to test idiomatic python : 

    def display(file_name):
        with open('a.txt', 'r') as fp:
            for line in fp:
                print(line)  

As a trivial example the above code when mock_open is used will be equivalent of opening an empty file,  but this code : 

    def display(file_name):
        with open('a.txt', 'r') as fp:
            while True:
                line = readline(fp)
                if line == '':
                    break
                print(line)

Will work correctly with the data provided to mock_open.

Regardless of how and when #33236 is solved - a fix would still be needed for mock_open to make it provide an iterator for the mocked file.
msg320278 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-06-22 21:43
Anthony's PR is awaiting merge.  Although Yury has reviewed it, as the core developers mocktest experts, it would be good if Michael and/or Robert could also take a look.
msg321334 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-07-09 16:31
This is basically a duplicate of bpo-21258, but I haven't closely look at the patches in both issues yet.

We should probably consider adding support for __next__ as well.
msg321336 - (view) Author: Anthony Flury (anthony-flury) * Date: 2018-07-09 17:04
But the __next__ is a method on the iterator; 

So long as __iter__ returns a valid iterator (which it does in my pull request), it will by definition support __next___

Although it is entirely possible that I have misunderstood what you are saying.
msg325197 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-09-12 22:21
New changeset 2087023fdec2c89070bd14f384a3c308c548a94a by Berker Peksag (Tony Flury) in branch 'master':
bpo-32933: Implement __iter__ method on mock_open() (GH-5974)
https://github.com/python/cpython/commit/2087023fdec2c89070bd14f384a3c308c548a94a
msg325198 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-09-12 22:24
Thanks for the patch, Anthony.I consider this a new feature, so I removed 3.6 and 3.7 from the versions field. We can backport to 3.7 if other core developers think that it's worth to fix in the latest maintenance branch.
msg325201 - (view) Author: Anthony Flury (anthony-flury) * Date: 2018-09-12 22:34
Berker,
Thanks for your work on getting this complete.

I would strongly support backporting if possible.

3.5 and 3.6 will be in common use for a while (afaik 3.6 has only now got delivered to Ubuntu as the default Python 3), and this does fix does allow full testing of what would be considered pythonic code.
msg325300 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-09-13 22:19
Ned, as release manager of 3.6 and 3.7, what do you think about backporting this to maintenance releases?
msg325309 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-09-13 23:47
While I think arguments could be made either way, this seems to me to be somewhat more of a bugfix (rather than a feature) in the sense that mock_open did not correctly emulate a real textfile open at least for an idiom that is commonly used (while acknowledging that mock_open does not claim to fully implement open or all classes of IO objects).  The key question to me is would backporting this change likely cause any change in behavior to existing programs running on 3.7.x or 3.6.x.  If yes, then we definitely shouldn't backport it.  If not, then there is now the issue that people using mock_open on 3.7.x (or possibly 3.6.x) still can't depend on its behavior unless they explicitly check for, say, 3.7.1 or 3.6.7.  That's not particularly user friendly, either.  So perhaps it *is* best to not backport; if the functionality is needed in earlier releases, one could create a PyPI package to provide it, for example.
msg325335 - (view) Author: Anthony Flury (anthony-flury) * Date: 2018-09-14 08:39
I still support backporting to 3.6 and 3.7 : 

Yes it is correct that this fix could change the behavior of existing test code, but only if someone has written a test case for a function where : 

 1. The function under test uses dunder_iter iteration
 2. The test case provides a read_data content to mock_open
 3. The test case expects a response from the function which 
    implies that the file provided is empty/invalid in all cases - regardless of the data provided.

I simply cannot see that someone would implement a test case such as this - if your file has data, you would expect that your function under test would recognize that the data exists, if that data is valid; and most code will differentiate between invalid data and empty data.

So the only time I think this fix would change the behavior of existing code is if someone has written an illogical test case, which is currently passing and would now fail (since the test function will no2 see the data being provided and respond as such).

Specifically the only change in behavior to existing code is to highlight an invalid test case and potentially a bug in the code under test. It is for this reason I support backporting.
msg325349 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2018-09-14 15:07
The potential change in behavior affecting existing code was one issue.  But the other was the fact that people writing tests to make use of the new behavior can't depend on that behavior being there for 3.7 or 3.6 without checking the patch level, for example, 3.6.7 vs 3.6.6.  That's one of the main reasons we generally do not backport behavior changes unless they are a clear bug; as I noted. this particular issue seems somewhere in between a bug and a feature.  Given how far along we are in the 3.6.x cycle, I think we definitely should not backport to 3.6.  Since 3.7 is near the beginning of its support cycle, I would not object if we did backport this for 3.7.1.  I'll leave it up to Berker.
msg325378 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-09-14 18:13
Thanks, Ned!

Anthony, I'm one of the maintainers of https://github.com/testing-cabal/mock and I'd be happy to merge a PR that backports the fix to the PyPI version of mock.
msg325379 - (view) Author: Anthony Flury (anthony-flury) * Date: 2018-09-14 18:20
Thank you.
msg325402 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2018-09-14 21:30
New changeset c83c375ed907bdd54361aa36ce76130360f323a4 by Berker Peksag (Miss Islington (bot)) in branch '3.7':
bpo-32933: Implement __iter__ method on mock_open() (GH-5974)
https://github.com/python/cpython/commit/c83c375ed907bdd54361aa36ce76130360f323a4
History
Date User Action Args
2018-09-14 21:30:07berker.peksagsetmessages: + msg325402
2018-09-14 18:20:43anthony-flurysetmessages: + msg325379
2018-09-14 18:13:48berker.peksagsetmessages: + msg325378
versions: + Python 3.7
2018-09-14 17:03:47miss-islingtonsetpull_requests: + pull_request8737
2018-09-14 15:07:21ned.deilysetmessages: + msg325349
2018-09-14 08:39:42anthony-flurysetmessages: + msg325335
2018-09-13 23:47:52ned.deilysetmessages: + msg325309
2018-09-13 22:19:22berker.peksagsetmessages: + msg325300
2018-09-12 22:34:52anthony-flurysetmessages: + msg325201
2018-09-12 22:27:20berker.peksaglinkissue21258 superseder
2018-09-12 22:24:40berker.peksagsetstatus: open -> closed
versions: - Python 3.6, Python 3.7
type: behavior -> enhancement
messages: + msg325198

resolution: fixed
stage: patch review -> resolved
2018-09-12 22:21:19berker.peksagsetmessages: + msg325197
2018-07-09 17:04:01anthony-flurysetmessages: + msg321336
2018-07-09 16:31:56berker.peksagsetnosy: + berker.peksag
messages: + msg321334
2018-06-22 21:43:42ned.deilysetnosy: + ned.deily
messages: + msg320278
2018-04-06 17:14:17anthony-flurysetmessages: + msg315026
2018-04-06 16:49:56eric.araujosetnosy: + eric.araujo
messages: + msg315025
2018-03-04 16:43:34anthony-flurysetpull_requests: + pull_request5743
2018-03-04 16:20:45bbaylessetnosy: + bbayles
2018-03-04 15:50:24anthony-flurysetpull_requests: + pull_request5742
2018-03-04 15:43:02anthony-flurysetpull_requests: + pull_request5741
2018-03-04 14:56:57anthony-flurysetkeywords: + patch
stage: patch review
pull_requests: + pull_request5740
2018-03-02 20:06:03terry.reedysetnosy: + rbcollins, ezio.melotti, michael.foord

versions: + Python 3.8, - Python 3.4, Python 3.5
2018-02-25 11:35:11anthony-flurysettype: behavior
2018-02-24 08:58:38anthony-flurycreate