Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mock_open file handle __exit__ does not call close #88351

Closed
williamsjoblom mannequin opened this issue May 20, 2021 · 6 comments · Fixed by #26902
Closed

mock_open file handle __exit__ does not call close #88351

williamsjoblom mannequin opened this issue May 20, 2021 · 6 comments · Fixed by #26902
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@williamsjoblom
Copy link
Mannequin

williamsjoblom mannequin commented May 20, 2021

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
    Copy link
    Mannequin Author

    williamsjoblom mannequin commented May 20, 2021

    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.

    @williamsjoblom williamsjoblom mannequin added 3.7 (EOL) end of life 3.8 only security fixes 3.10 only security fixes 3.11 only security fixes 3.9 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels May 20, 2021
    @terryjreedy terryjreedy removed 3.7 (EOL) end of life 3.8 only security fixes labels Jun 26, 2021
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    @cjw296
    Copy link
    Contributor

    cjw296 commented Dec 27, 2022

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

    @cjw296 cjw296 mentioned this issue Dec 27, 2022
    Merged
    @CendioOssman
    Copy link
    Contributor

    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
    Copy link
    Contributor

    cjw296 commented Dec 27, 2022

    @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
    Copy link
    Contributor

    itamaro commented Dec 28, 2022

    @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 cjw296 linked a pull request Jun 9, 2023 that will close this issue
    bpo-44185: Added close() to mock_open __exit__ #26902
    Merged
    @cjw296
    Copy link
    Contributor

    cjw296 commented Jun 9, 2023

    Okay, let's land #26902 :-)

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Assignees
    No one assigned
    Labels
    3.9 only security fixes 3.10 only security fixes 3.11 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    Unittest issues
    Status: Done
    Milestone
    No milestone
    Development

    Successfully merging a pull request may close this issue.

    bpo-44185: Added close() to mock_open __exit__ samety/cpython
    4 participants
    @itamaro @cjw296 @CendioOssman @terryjreedy