python / cpython

The Python programming language
https://www.python.org
Other
63.51k stars 30.42k forks source link

Hide __enter__ calls in mock_open #88669

Open d3de1998-5581-4e9d-9d98-72eb1cbd3618 opened 3 years ago

d3de1998-5581-4e9d-9d98-72eb1cbd3618 commented 3 years ago
BPO 44503
Nosy @voidspace, @CendioOssman

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: ```python assignee = None closed_at = None created_at = labels = ['type-feature', 'library'] title = 'Hide __enter__ calls in mock_open' updated_at = user = 'https://github.com/CendioOssman' ``` bugs.python.org fields: ```python activity = actor = 'terry.reedy' assignee = 'none' closed = False closed_date = None closer = None components = ['Library (Lib)'] creation = creator = 'CendioOssman' dependencies = [] files = [] hgrepos = [] issue_num = 44503 keywords = [] message_count = 2.0 messages = ['396469', '396470'] nosy_count = 2.0 nosy_names = ['michael.foord', 'CendioOssman'] pr_nums = [] priority = 'normal' resolution = None stage = None status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue44503' versions = [] ```

d3de1998-5581-4e9d-9d98-72eb1cbd3618 commented 3 years ago

I'd like to write this test case:

  with patch('builtins.open') as pyopen:
    mock_open(pyopen, read_data="foo")
    run()
    pyopen.assert_has_calls([call("filename", "wt"),
                             call().write("gazonk"),
                             call().close()])

and I shouldn't have to care if the code is written like this:

  def run():
    f = open("filename", "wt")
    try:
      write("gazonk")
    finally:
      f.close()

or like this:

  def run():
    with open("filename", "wt") as f:
      write("gazonk")
d3de1998-5581-4e9d-9d98-72eb1cbd3618 commented 3 years ago

Also see bpo-44185 for __exit__.

tirkarthi commented 2 years ago

The __enter__ and __exit__ calls are tested to be part of executed methods for the object and are recorded. Removing them might result in case where context manager style is changed to use manual close and the test behavior might not catch these. I don't think it's worth the cost to special case mock_open to behave differently filtering out dunder methods and along with explaining them in the docs since there could be existing code depending on this.

https://github.com/python/cpython/blob/2fadde7e6645e45e090b0187c28877300b07cba3/Lib/unittest/test/testmock/testwith.py#L153-L163

CendioOssman commented 2 years ago

I can be sympathetic to existing code relying on current behaviour, so this could be gated by an option to mock_open(). But I think the existing behaviour is forcing us to write bad tests. We should be testing behaviour, not implementation.

I assume the whole point of mock_open() is to avoid forcing everyone to keep duplicating wrappers for handling the common case of opening a file. Which we are now forced to do since mock_open() isn't agnostic to how the file is closed.