Adds new backup_attributes context manager for tests
#1948
Conversation
This context manager will makes it possible to rollback an object state after leaving the context manager. This is a follow up of <kivy#1867 (comment)> Also address docstring format as suggested by @opacam in <kivy#1933 (comment)>
| with ( | ||
| patch_sys_argv(argv)), ( | ||
| patch_sys_stdout()) as m_stdout, ( | ||
| backup_attributes(Recipe, {'recipes'})): |
There was a problem hiding this comment.
in Python3 that could also be used as a decorator, e.g.
@backup_attributes(Recipe, {'recipes'})
def test_recipes(self):
pass
There was a problem hiding this comment.
I'm wondering, will this work when used alongside with mock.patch decorator?
There was a problem hiding this comment.
Depends of what you patch and if you patch before the backup_attributes or after I guess
|
Nice work ❤️ as I already stated, I think merging this too quickly though might be risky: It seems to me like waiting for other code someone writes in the future to blow up that uses |
|
On further inspection it also seems a bit over the top, but I like predictable tests so I get the motivation & it might be worth it. So I kinda like it still. But I think as long as it masks the actual issue I don't think it's the right point in time to merge this Edit: ahaha, just your test for it is over the top not the implementation, as I am seeing now 😍 nice work 👍 it really looks useful as a general tool for the future, although I'm not too sure yet how much we need it |
| @@ -2,6 +2,7 @@ | |||
| import sys | |||
| import pytest | |||
| import mock | |||
| from util import backup_attributes | |||
There was a problem hiding this comment.
I'm not sure if the name you give to the file can cause troubles in some import situations...since we already have a file with the same name in pythonforandroid.util 🤔...I would give it another filename to avoid strange situations...but maybe there is no need...what do you think about this?
There was a problem hiding this comment.
Yes it also bugged me. Actually I usually call it utils.py, then I changed it to be consistent. But still yes it also bugs me that it looks the same and one is for tests only. I don't have preferences, tell me how you like it to be called and I can call it that way 😄
There was a problem hiding this comment.
toolsutilitiesassets
...
whatever it's fine...just be careful to not have one already in p4a source code...
I don't have preferences for this neither 😆
There was a problem hiding this comment.
@Jonast, I would avoid test_utils...I think that unittests looks into the first part of the filename to determine if a file is a test or not...am I right?
Plus the mentioned file is not a test...it's a set of utilities to help us testing...I think that it's confusing...
|
Ok I thought about this some more after sleeping, and now I'm not so sure this pull request is a good idea: Why are we trying to arbitrarily reset a small part of p4a's entire global state? Wouldn't it be a much cleaner idea to just use a different interpreter per test if we really wanted to guarantee that? Otherwise, why are we just caring about this specific small part now and not resetting everything else, what's the meaning behind that? Because just for this bug it doesn't really make sense to me, that bug should rather be test-covered & fixed instead of avoided through a state reset anyway So in overall I'm really not sure where this pull request is getting us and I find it hard to see it as anything but a complication with little benefit and a weird partial focus 😶 would be happy to be convinced of something else though |
This context manager will makes it possible to rollback an object state
after leaving the context manager.
This is a follow up of
#1867 (comment)
Also address docstring format as suggested by @opacam in
#1933 (comment)