Issue 33722: Document builtins in mock_open (original) (raw)
Created on 2018-05-31 18:35 by jcrotts, last changed 2022-04-11 14:59 by admin. This issue is now closed.
Messages (6)
Author: Jay Crotts (jcrotts) *
Date: 2018-05-31 18:35
The examples on using mock_open only include instances where objects are mocked in the REPL, so 'main'.open is replaced. Commonly objects are mocked for use in other test modules, so builtins.open would be used instead.
A note about this in the documentation would be useful not familiar with python internals. I'm happy to do a PR for it.
Author: Terry J. Reedy (terry.reedy) * 
Date: 2019-04-11 02:25
My guess is that you created the branch for PR 7491 when your local master branch was months out of date. Or you branched off of the 3.7 branch. Whatever, your patch proposed to revert recent changes to master, touching about 1000 files. When you pushed the button on the github web page to compare and possibly make a PR, you should have seen a list of +- 1000 changes in the proposed PR, closed the window, deleted the branch on the fork and your repository, and started over with a new branch from freshly updated master. You are right that the review requests were automatic. However, I am not sure how the 6 month delay factors into what happened.
Since Guido closed the PR rather than suggest fixing it further, I suggest that you start over as suggested above and look closely before pressing 'Create PR'. It appears that you already deleted the branch from the fork.
From my limited knowledge of how to use unitest.mock correctly, I agree that the example looks plausible.
Author: Jay Crotts (jcrotts) *
Date: 2019-04-11 02:41
Yeah the PR had been stale for a while, and I re-based my fork without closing the PR, so when I pushed it to the fork it updated the PR reviewer list. Silly mistake by me, I should have made sure the diff wasn't huge before pushing it. I know review time is valuable and did not mean to waste it.
If you think it is a worthwhile change I'm happy to start over with another PR.
Author: Karthikeyan Singaravelan (xtreak) *
Date: 2019-04-11 03:05
The example looks like a good addition to me since mocking builtins.open is not documented for this use case. Please start a new PR.
Author: Terry J. Reedy (terry.reedy) *
Date: 2019-04-11 04:37
After looking at the context of the patch and thinking more about whether the patch is a good idea, I am reversing what I said before and think that this issue and the new patch (sorry) should be closed.
The new example duplicates the current example. The only difference is that one module name,'main', is replaced with another, 'builtins'. Mechanically, there is no difference, so the duplicate adds nothing.
The current example, contrary to the claim, "'main'.open is replaced", adds .open to main and thereby masks builtins.open in, and only in, main. In use, 'main' in all the current doc examples should be replaced by the name of the module where open is called. This would usually, but not necessarily, be the module being tested. This is explained in the section on where to patch.
Touching builtins is generally a bad idea unless one really understands and wants to affect all modules in the process, including the test code and unittest code. Beginners should not be encouraged to do this. If one replaces a builtin needed by test code or other untested mdoules before the replacement is undone, one disables the test code.
The real problem, if any, with the mock examples, is that they usually do everything in one module (usually main) while real use involves code and execution in at least two modules. Users are left to work out which examples lines belong in text_xyz, which would be in xyz, and what essential code is missing.
Author: Jay Crotts (jcrotts) *
Date: 2019-04-11 05:38
No worries, I think all of your points make sense, especially number one after looking at the patch again. I looked at the docs again, and there is even an example of another built in being patched.
Thanks for taking the time to review it.
I'm okay with closing it if you don't think there is any value add.
History
Date
User
Action
Args
2022-04-11 14:59:01
admin
set
github: 77903
2019-04-11 06:11:39
jcrotts
set
status: open -> closed
resolution: rejected
stage: patch review -> resolved
2019-04-11 05:38:20
jcrotts
set
messages: +
2019-04-11 04:37:25
terry.reedy
set
messages: +
2019-04-11 03:19:38
jcrotts
set
pull_requests: + <pull%5Frequest12700>
2019-04-11 03:05:49
xtreak
set
nosy: + xtreak
messages: +
2019-04-11 02:41:32
jcrotts
set
messages: +
2019-04-11 02:25:09
terry.reedy
set
nosy: + terry.reedy
messages: +
2018-06-07 19:13:50
jcrotts
set
keywords: + patch
stage: patch review
pull_requests: + <pull%5Frequest7116>
2018-05-31 18:35:34
jcrotts
create