Skip to content

Conversation

@akaihola
Copy link

@akaihola akaihola commented Sep 10, 2021

Description

patch("io.TextIOWrapper") wouldn't actually cause io.TextIOWrapper() to return the StringIO object created by the test. This didn't cause any problems though until I added a test in #2484 in which content= is non-empty:

>               f.write(dst)
E               ValueError: I/O operation on closed file

src/black/__init__.py:849: ValueError

In the debugger we can see that io.TextIOWrapper used on src/black/__init__.py:842–844:

            f = io.TextIOWrapper(
                sys.stdout.buffer, encoding=encoding, newline=newline, write_through=True
            )

is not the patched lambda:

(Pdb) io.TextIOWrapper
<class '_io.TextIOWrapper'>

With the changes in the PR, it is properly patched:

> src/black/__init__.py(850)format_stdin_to_stdout()
-> f.write(dst)
(Pdb) io.TextIOWrapper
<Mock name='mock.TextIOWrapper' id='139749471475408'>

Checklist - did you ...

  • Add a CHANGELOG entry if necessary?
  • Add / update tests if necessary?

`patch("io.TextIOWrapper")` wouldn't actually cause
`io.TextIOWrapper()` to return the `StringIO` object created by the
test. This started to fail as soon as `content=` is non-empty.
@akaihola akaihola force-pushed the fix-textwrapper-patching branch from 0f837d3 to 9a1897c Compare September 10, 2021 10:50
Copy link
Collaborator

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify why the previous way didn't work? It looks to me like it should.

Co-authored-by: Jelle Zijlstra <[email protected]>
@akaihola
Copy link
Author

Can you clarify why the previous way didn't work? It looks to me like it should.

@JelleZijlstra, a very good question, which was a tough one to sort out!

Indeed, in the main branch, it works just fine. But in #2484 I had to introduce the extra call to decodebytes() in format_str() (see diff). And since decodebytes() instantiates an io.TextIOWrapper() and closes it, the io.TextIOWrapper() invocation we intended to patch in format_stdin_to_stdout() wouldn't work anymore.

In other words, this is a case of a common problem when patching and asserting library classes/functions: You can't be sure that the patched item won't in the future get extra invocations in addition to the one you're originally testing for.

For some reason, patching only the io module imported in black/__init__.py fixes this issue. I can't really figure out now why that is the case, since decodebytes() is in that same module.

As a conclusion from looking into this deeper, the modification to patching in the test function really belongs in #2484 since it's only the changes there which break the patching. So closing this PR.

@akaihola akaihola closed this Sep 11, 2021
@akaihola akaihola deleted the fix-textwrapper-patching branch September 11, 2021 16:52
akaihola pushed a commit to akaihola/black that referenced this pull request Sep 11, 2021
We need to "surgically" make sure only the intended invocation of
`io.TextIOWrapper()` is patched, and other calls get through to the
original implementation.

See discussion in psf#2489
@JelleZijlstra
Copy link
Collaborator

Thanks for the thorough explanation!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants