mock_open file handle __exit__ does not call close

[williamsjoblom]

BPO	44185
Nosy	@voidspace, @tirkarthi, @CendioOssman, @williamsjoblom, @samety
PRs	bpo-44185: Added close() to mock_open __exit__ #26902

^{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 = None
created_at = <Date 2021-05-20.08:17:05.886>
labels = ['type-feature', 'library', '3.9', '3.10', '3.11']
title = 'mock_open file handle __exit__ does not call close'
updated_at = <Date 2021-06-26.15:05:02.423>
user = 'https://github.com/williamsjoblom'

bugs.python.org fields:

activity = <Date 2021-06-26.15:05:02.423>
actor = 'terry.reedy'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2021-05-20.08:17:05.886>
creator = 'williamsjoblom'
dependencies = []
files = []
hgrepos = []
issue_num = 44185
keywords = ['patch']
message_count = 1.0
messages = ['394005']
nosy_count = 5.0
nosy_names = ['michael.foord', 'xtreak', 'CendioOssman', 'williamsjoblom', 'sametyaslan']
pr_nums = ['26902']
priority = 'normal'
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue44185'
versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

[williamsjoblom]

A common testing scenario is assuring that opened files are closed. Since unittest.mock.mock_open() can be used as a context manager, it would be reasonable to expect its __exit__ to invoke close so that one can easily assert that the file was closed, regardless of if the file was opened with a plain call to open or with a context manager.

[cjw296]

@MaxwellDupre / @samety - following up here as it's the issue that resulted in #26902... (/cc @tirkarthi )

What's your actual need for recording the close here?
The call to __exit__ is already recorded, as you can see here:

cpython/Lib/test/test_unittest/testmock/testwith.py

Line 175 in ce39aaf

call().__exit__(None, None, None),

While I don't see anything bad about the PR to fix this, I'm reticent to merge without a really good reason as it may result in many test expectations having to be updated.

[CendioOssman]

We'd like this change to uphold a good test principle of testing behaviour, not implementation. I.e. we want to test that the code closes the file, not how it closes it. So changing the code from explicitly calling close() to using a context manager (or vice versa) shouldn't require a change to the test.

[cjw296]

@carljm / @itamaro - as folks looking after what sounds like a test suite that makes heavy use of mock, how would you feel about this change?

[itamaro]

@carljm / @itamaro - as folks looking after what sounds like a test suite that makes heavy use of mock, how would you feel about this change?

thanks for the heads up!
nothing jumps at me with this change as concerning.
I can't think of existing tests we have we will need to change as a result.

[cjw296]

Okay, let's land #26902 :-)