Hi Megan, thanks for the patch! What is an example of a standard library function with a builtin type? I'd like to exercise the use case that has been causing you trouble.
This change has a couple issues, for instance it treats anything with the same name as a builtin like a builtin. It also calls setattr() on standard library functions if you try to mock it a second time.
I've pushed an adaptation of your change to the 'mocking' branch of my personal repository... git://git.torproject.org/user/atagar/stem.git
change... https://gitweb.torproject.org/user/atagar/stem.git/commitdiff/5029e4b9b92dba...
It shouldn't have the issues that I mentioned, though some remaining bits of the change confuse me. As you've found the mocking code is pretty tricky so I won't be able to make sense of it until I see what was breaking.
Thanks! -Damian
PS. Very nice commit message on b8b7c22. It helped me a lot in making sense of the change - thanks for that.
Hi Damian,
To answer your first question, we ran into the trouble when mocking time.time(). This came up for us, as type(time.time) is 'builtin_function_or_method', which is the same as type(open) -> 'builtin_function_or_method'.
We also updated your adaptation to our patch so that no code is repeated. This should make the function cleaner and more readable. This new code can be found at: https://github.com/jacthinman/Tor-Stem/blob/mocking/test/mocking.py
Best, -Erik & Megan
On Wed, Jun 13, 2012 at 2:21 AM, Damian Johnson atagar@torproject.orgwrote:
Hi Megan, thanks for the patch! What is an example of a standard library function with a builtin type? I'd like to exercise the use case that has been causing you trouble.
This change has a couple issues, for instance it treats anything with the same name as a builtin like a builtin. It also calls setattr() on standard library functions if you try to mock it a second time.
I've pushed an adaptation of your change to the 'mocking' branch of my personal repository... git://git.torproject.org/user/atagar/stem.git
change...
https://gitweb.torproject.org/user/atagar/stem.git/commitdiff/5029e4b9b92dba...
It shouldn't have the issues that I mentioned, though some remaining bits of the change confuse me. As you've found the mocking code is pretty tricky so I won't be able to make sense of it until I see what was breaking.
Thanks! -Damian
PS. Very nice commit message on b8b7c22. It helped me a lot in making sense of the change - thanks for that.
We also updated your adaptation to our patch so that no code is repeated. This should make the function cleaner and more readable. This new code can be found at: https://github.com/jacthinman/Tor-Stem/blob/mocking/test/mocking.py
Ah ha, makes much more sense now - thanks. After some reflection I realized that the mocking code was more convoluted that it needs to be. What do you think of this change instead?
https://gitweb.torproject.org/user/atagar/stem.git/commitdiff/eb51eab0badf6b...
It seems to handle both of our use cases while also axing much of the special handling for builtins...
import test.mocking import time my_mock = lambda i: i test.mocking.mock(time.time, my_mock) time.time(5)
5
test.mocking.revert_mocking() time.time()
1339689308.64306
test.mocking.mock(open, my_mock) open(5)
5
test.mocking.revert_mocking() open("/some_file", "r")
Traceback (most recent call last): File "<stdin>", line 1, in <module> IOError: [Errno 2] No such file or directory: '/some_file'
If it looks good to you then I'll go ahead and merge this with master. Cheers! -Damian
PS. Thanks for converting the mocking module to reStrcutredText. Usually that would have been a welcome improvement, however in this case I'd already converted it in the master branch so that actually generated merge conflicts with your patch. It's helpful if there's separate commits for separate changes (ie, a commit for the mocking changes and another for the reStructuredText conversion). That said, I don't always follow this rule either. ;)
The changes look great! I see no issues with merging to the master branch. Sorry about the reStructuredText inconvenience. I actually didn't even think of it -- I just meant to clean up the comments a bit, but I'll definitely be more careful in the future.
- Erik & Megan
On Thu, Jun 14, 2012 at 4:28 PM, Damian Johnson atagar@torproject.orgwrote:
We also updated your adaptation to our patch so that no code is repeated. This should make the function cleaner and more readable. This new code
can
be found at: https://github.com/jacthinman/Tor-Stem/blob/mocking/test/mocking.py
Ah ha, makes much more sense now - thanks. After some reflection I realized that the mocking code was more convoluted that it needs to be. What do you think of this change instead?
https://gitweb.torproject.org/user/atagar/stem.git/commitdiff/eb51eab0badf6b...
It seems to handle both of our use cases while also axing much of the special handling for builtins...
import test.mocking import time my_mock = lambda i: i test.mocking.mock(time.time, my_mock) time.time(5)
5
test.mocking.revert_mocking() time.time()
1339689308.64306
test.mocking.mock(open, my_mock) open(5)
5
test.mocking.revert_mocking() open("/some_file", "r")
Traceback (most recent call last): File "<stdin>", line 1, in <module> IOError: [Errno 2] No such file or directory: '/some_file'
If it looks good to you then I'll go ahead and merge this with master. Cheers! -Damian
PS. Thanks for converting the mocking module to reStrcutredText. Usually that would have been a welcome improvement, however in this case I'd already converted it in the master branch so that actually generated merge conflicts with your patch. It's helpful if there's separate commits for separate changes (ie, a commit for the mocking changes and another for the reStructuredText conversion). That said, I don't always follow this rule either. ;)
-
Damian Johnson -
Erik I Islo