New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
mock.patch.dict spoils order of items in collections.OrderedDict #69116
Comments
Hi! Current implementation of Lines 1559 to 1560 in 923527f
# support any argument supported by dict(...) constructor
self.values = dict(values) Most obvious way is to check if if not isinstance(values, dict):
values = dict(values)
self.values = values This will work for if not hasattr(values, 'update'):
values = dict(values)
self.values = values Here is a question existence of what attrs to check. Any ideas? Thanks! |
Hi! Current implementation of Lines 1559 to 1560 in 923527f
# support any argument supported by dict(...) constructor
self.values = dict(values) Most obvious way is to check if if not isinstance(values, dict):
values = dict(values)
self.values = values This will work for if not hasattr(values, 'update'):
values = dict(values)
self.values = values Here is a question existence of what attrs to check. Any ideas? Thanks! |
Based on reading the patch.dict doct, I'm guessing that that dict call is making a copy in order to do a restore later. Perhaps replacing the dict call with a copy call would be sufficient? (I haven't looked at the dict.patch code). |
I think that would do it. |
Submitting a patch. To support both iterable and mapping in the same way as with dict(...), Patch includes unittest which tests the reported problem. |
Patch looks good to me. |
Thanks @nekobon for the patch. I am triaging old mock related issues. I think dict insertion order is maintained from 3.6 and guaranteed with 3.7 and above. But it would be good to add the unit test in the patch as a PR. I ran the test on master and it passes. |
More tests are generally a good thing, so go for it :-) |
Thinking about it further the attached test is based on the ordering of dict and hence works only on 3.6 and above. But this test will be backported to mock on PyPI where this will fail on 2.7 and 3.4, 3.5. Is it okay to apply the fix that makes backporting easy though it has no effect on current CPython implementation or to only apply the test since it's fixed in CPython due to dict implementation detail and make sure the cabal backporting is done with this detail taken into account to run test only on 3.6+. But the latter makes the issue still prevalent in cabal's mock repo. I think it's better to apply the fix along with the test that will make it easy on both ends. I am not aware of the backport process for mock on PyPI so feedback will be helpful on this. |
I would suggest applying the fix with the latest version in mind to keep the codebase clean. If you want to backport it to previous versions of Python you can do it manually through a PR targetting the right branch. Alternative 1: Might be easier is to send a patch that works in all version and another one that "modernises" it to python3.7. Basically, write all tests with OrderedDict and then just shoot an extra PR that converts that to a plain dict. Alternative 2: This is only a problem on versions on versions < 3.6, isn't it? If so you might want to close this issue or just have a PR that targets the 3.5 if worth backporting and/or PyPI's. (This is my conservative mind as you know I usually push for changing as less as possible hehe). |
Thanks Mario, I will convert the unit test as a PR before closing the issue since I feel the test is a good one for inclusion and can help if dict order guarantee is changed in future. I will raise a backport PR to cabal's mock repo where the fix with test can be merged with reference to this issue. |
Hi. |
Hi Vaibhav, As noted in the thread the issue is fixed in 3.6 and above due to dict order being guaranteed. But it would be nice to have the test in the patch converted as a unit test. With respect to backport the fixes are backported to https://github.com/testing-cabal/mock to make mock library available for older versions of Python which would required the fix since dict order is not guaranteed in older versions. Once the test to CPython is merged you can make a PR to the mock repo with the fix and the test. I haven't started working on a PR for this so feel free to go ahead. |
ping Vaibhav :-)
|
Hi Emmanuel Please go ahead and make a PR. :) |
Hi xtreak,
I am running the test on master and fail. I don't think that the orderdict on patch.dict is implement. Or maybe I am wronging somewhere |
Can you please post the error and the command to run the test? On applying the patch on master I cannot see any errors with below commands : # Applying the patch with only test $ wget https://bugs.python.org/file40488/issue24928.patch
$ git apply issue24928.patch
$ git checkout Lib/unittest/mock.py # Only tests are needed # Running tests with no errors
I can see an error running the file separately using ./python.exe Lib/unittest/test/testmock/testpatch.py Traceback (most recent call last):
File "Lib/unittest/test/testmock/testpatch.py", line 1870, in test_special_attrs
self.assertEqual(foo.__module__, 'unittest.test.testmock.testpatch')
AssertionError: '__main__' != 'unittest.test.testmock.testpatch'
- __main__
+ unittest.test.testmock.testpatch Ran 97 tests in 0.704s FAILED (failures=1) Build info $ ./python.exe
Python 3.8.0a0 (heads/master:47a2fced84, Jan 4 2019, 10:36:15) |
Sorry I was wrong. On foo = OrderedDict()
foo['a'] = object()
foo['b'] = 'something' I was write "first" and "second" like key and fail in @patch.dict(foo, OrderedDict(update_values))
def test():
self.assertEqual(list(foo), sorted(foo))
test() Sorry. Now I am sending the PR |
No problem :) I think the test can use a context manager instead of using test() and a decorator but that can be discussed on the PR. Thanks! |
Ping :-) Thanks Karthikeyan for the PR review. Would someone else like review it, please? Thanks! |
While I agree having more tests are a good thing, I'm not sure if the test in PR 11437 should be merged as it's not specifically testing a feature of the mock module. patch.dict() basically does the following operation (ignoring possible AttributeErrors): # Keep the original one to use later.
d = original_dict.copy()
original_dict.update(new_dict) I think the relationship between dict and OrderedDict (including any other dict subclasses and dict-like objects) and anything related to insertion order should be tested either in test_dict or in test_ordered_dict. Also, the test can be simplified, but I will let other core developers chime in with their thoughts before reviewing PR 11437 on GitHub. |
Hi Berker, Thanks for you response. I agree when you say that patch.dict is a simply operation, but I think that this test can be necessary, if for some reason the implementation of patch.dict (or its parts) decide change.
I am not sure if this has to be tested on test_dict or test_oredered_dict, because we are not testing specifically dict-like objects. This test, can be written on test_patch_dict and not create a new |
As I said before, I can't see an additional test like this hurting, especially if it highlights problems with earlier python versions when it's backported. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: