Skip to content

Comments

Fix patching of io.TextWrapper in tests#2489

Closed
akaihola wants to merge 3 commits intopsf:mainfrom
akaihola:fix-textwrapper-patching
Closed

Fix patching of io.TextWrapper in tests#2489
akaihola wants to merge 3 commits intopsf:mainfrom
akaihola:fix-textwrapper-patching

Conversation

@akaihola
Copy link

@akaihola akaihola commented Sep 10, 2021 •
edited
Loading

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?

Fix patching of io.TextWrapper in tests
9a1897c 
`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
JelleZijlstra
JelleZijlstra reviewed Sep 10, 2021
View reviewed changes
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.

@akaihola @JelleZijlstra
Update CHANGES.md
a99ad3a 
Co-authored-by: Jelle Zijlstra <[email protected]>
@akaihola
Copy link
Author

akaihola commented Sep 11, 2021

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
Fix patching of io.TextIOWrapper
5806822 
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

JelleZijlstra commented Sep 11, 2021

Thanks for the thorough explanation!

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

Reviewers

@JelleZijlstra JelleZijlstra JelleZijlstra left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@akaihola @JelleZijlstra