classification
Title: mock.patch.dict spoils order of items in collections.OrderedDict
Type: behavior Stage: patch review
Components: Library (Lib) Versions: Python 3.8, Python 3.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: michael.foord Nosy List: Alexander Oblovatniy, azsorkin, berker.peksag, cjw296, dojutsu-user, eamanu, mariocj89, michael.foord, nekobon, r.david.murray, rbcollins, rhettinger, xtreak
Priority: normal Keywords: easy, needs review, patch

Created on 2015-08-24 19:19 by Alexander Oblovatniy, last changed 2019-01-20 15:03 by eamanu.

Files
File name Uploaded Description Edit
issue24928.patch nekobon, 2015-09-17 06:20 patch for issue 24928 with unittest review
Pull Requests
URL Status Linked Edit
PR 11437 open eamanu, 2019-01-05 14:13
Messages (22)
msg249069 - (view) Author: Alexander Oblovatniy (Alexander Oblovatniy) Date: 2015-08-24 19:19
Hi!

Current implementation of `patch.dict` spoils order of items in `collections.OrderedDict`, because it explicitly converts passed `values` to `dict` (https://github.com/python/cpython/blob/923527f560acd43d4cc11293060900e56c7cb39b/Lib/unittest/mock.py#L1559-L1560):

```python

# support any argument supported by dict(...) constructor
self.values = dict(values)
```

Most obvious way is to check if `values` is an instance of `dict`. If it's not, then we need to convert it to dict:

```python

if not isinstance(values, dict):
    values = dict(values)

self.values = values
```

This will work for `OrderedDict`, because it's a subclass of `dict`. But this will not work for `UserDict.UserDict`, `UserDict.DictMixin`, `collections.MutableMapping`, etc., because they do not subclass `dict`. So, better way is to less strict check and look if `values` implements `dict`-like interface, e.g.:

```python

if not hasattr(values, 'update'):
    values = dict(values)

self.values = values
```

Here is a question existence of what attrs to check.

Any ideas?

Thanks!
msg249070 - (view) Author: Alexander Oblovatniy (Alexander Oblovatniy) Date: 2015-08-24 19:21
Hi!

Current implementation of `patch.dict` spoils order of items in `collections.OrderedDict`, because it explicitly converts passed `values` to `dict` (https://github.com/python/cpython/blob/923527f560acd43d4cc11293060900e56c7cb39b/Lib/unittest/mock.py#L1559-L1560):

    # support any argument supported by dict(...) constructor
    self.values = dict(values)


Most obvious way is to check if `values` is an instance of `dict`. If it's not, then we need to convert it to dict:

    if not isinstance(values, dict):
        values = dict(values)

    self.values = values


This will work for `OrderedDict`, because it's a subclass of `dict`. But this will not work for `UserDict.UserDict`, `UserDict.DictMixin`, `collections.MutableMapping`, etc., because they do not subclass `dict`. So, better way is to less strict check and look if `values` implements `dict`-like interface, e.g.:

    if not hasattr(values, 'update'):
        values = dict(values)

    self.values = values


Here is a question existence of what attrs to check.

Any ideas?

Thanks!
msg249073 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-08-24 20:02
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).
msg249102 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2015-08-25 02:56
> Perhaps replacing the dict call with a copy call would be sufficient?

I think that would do it.
msg250874 - (view) Author: Yu Tomita (nekobon) * Date: 2015-09-17 06:20
Submitting a patch.

To support both iterable and mapping in the same way as with dict(...), `values` is updated to be a list of length-2 iterables instead of using copy call. 

Patch includes unittest which tests the reported problem.
msg254725 - (view) Author: Michael Foord (michael.foord) * (Python committer) Date: 2015-11-16 10:36
Patch looks good to me.
msg331473 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-12-10 07:37
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.
msg331584 - (view) Author: Chris Withers (cjw296) * (Python committer) Date: 2018-12-11 06:34
More tests are generally a good thing, so go for it :-)
msg332282 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-12-21 07:39
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.
msg332286 - (view) Author: Mario Corchero (mariocj89) * (Python triager) Date: 2018-12-21 10:14
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.
I think the process is to set up a label and then use https://github.com/python/core-workflow/tree/master/cherry_picker as I'd expect the tests to fail to apply the patch "manually.

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).
msg332291 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-12-21 12:45
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.
msg332540 - (view) Author: Vaibhav Gupta (dojutsu-user) * Date: 2018-12-26 14:17
Hi.
I would like to make a PR for this.
Also, I am not very familiar with the process of backporting. Is something specific needs to be done for that which is related to this?
msg332575 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2018-12-27 05:39
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.
msg332976 - (view) Author: Emmanuel Arias (eamanu) * Date: 2019-01-04 13:19
ping Vaibhav :-)

> I would like to make a PR for this.
msg332977 - (view) Author: Vaibhav Gupta (dojutsu-user) * Date: 2019-01-04 13:31
Hi Emmanuel

Please go ahead and make a PR. :)
msg333056 - (view) Author: Emmanuel Arias (eamanu) * Date: 2019-01-05 13:38
Hi xtreak,

> 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.

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
msg333057 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2019-01-05 13:58
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

* ./python.exe Lib/unittest/test/testmock/ 
* ./python.exe -m unittest -v unittest.test.testmock
* ./python.exe -m unittest -v unittest.test.testmock.testpatch

I can see an error running the file separately using `./python.exe Lib/unittest/test/testmock/testpatch.py` but I don't think it's related to the patch : 

./python.exe Lib/unittest/test/testmock/testpatch.py
............................................................................................F....
======================================================================
FAIL: test_special_attrs (__main__.PatchTest)
----------------------------------------------------------------------
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)
msg333058 - (view) Author: Emmanuel Arias (eamanu) * Date: 2019-01-05 14:03
Sorry I was wrong.

On 

```python
foo = OrderedDict()
foo['a'] = object()
foo['b'] = 'something'
```

I was write "first" and "second" like key and fail in

```python
@patch.dict(foo, OrderedDict(update_values))
def test():
    self.assertEqual(list(foo), sorted(foo))

test()
```

Sorry.

Now I am sending the PR
msg333059 - (view) Author: Karthikeyan Singaravelan (xtreak) * (Python triager) Date: 2019-01-05 14:07
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!
msg333380 - (view) Author: Emmanuel Arias (eamanu) * Date: 2019-01-10 14:13
Ping :-)

Thanks Karthikeyan for the PR review. 

Would someone else like review it, please?

Thanks!
Regards
msg333890 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2019-01-17 18:25
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.
msg334085 - (view) Author: Emmanuel Arias (eamanu) * Date: 2019-01-20 15:03
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 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.

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 `test_patch_dict_with_orderdict`.
History
Date User Action Args
2019-01-20 15:03:19eamanusetmessages: + msg334085
2019-01-17 18:26:04berker.peksagsetpull_requests: - pull_request10872
2019-01-17 18:25:29berker.peksagsetnosy: + berker.peksag
messages: + msg333890
2019-01-10 14:13:00eamanusetmessages: + msg333380
2019-01-05 14:13:42eamanusetpull_requests: + pull_request10872
2019-01-05 14:13:17eamanusetpull_requests: + pull_request10871
2019-01-05 14:07:58xtreaksetmessages: + msg333059
2019-01-05 14:03:40eamanusetmessages: + msg333058
2019-01-05 13:58:38xtreaksetmessages: + msg333057
versions: - Python 3.6
2019-01-05 13:38:43eamanusetmessages: + msg333056
2019-01-04 13:31:36dojutsu-usersetmessages: + msg332977
2019-01-04 13:19:22eamanusetnosy: + eamanu
messages: + msg332976
2018-12-27 05:39:56xtreaksetmessages: + msg332575
2018-12-26 14:17:46dojutsu-usersetnosy: + dojutsu-user
messages: + msg332540
2018-12-21 12:45:28xtreaksetmessages: + msg332291
2018-12-21 10:14:38mariocj89setmessages: + msg332286
2018-12-21 07:39:38xtreaksetmessages: + msg332282
2018-12-11 06:34:51cjw296setmessages: + msg331584
2018-12-10 07:37:56xtreaksetnosy: + cjw296, mariocj89, xtreak

messages: + msg331473
versions: + Python 3.7, Python 3.8, - Python 3.4, Python 3.5
2015-11-16 10:36:31michael.foordsetmessages: + msg254725
2015-11-15 22:30:50rhettingersetassignee: michael.foord
2015-11-14 08:34:21serhiy.storchakasetkeywords: + needs review
nosy: + michael.foord

stage: needs patch -> patch review
2015-09-17 06:20:37nekobonsetfiles: + issue24928.patch

nosy: + nekobon
messages: + msg250874

keywords: + patch
2015-09-08 18:36:09azsorkinsetnosy: + azsorkin
2015-08-25 12:39:32r.david.murraysetkeywords: + easy
stage: needs patch
versions: + Python 3.5, Python 3.6, - Python 2.7
2015-08-25 02:56:18rhettingersetnosy: + rhettinger
messages: + msg249102
2015-08-24 20:02:24r.david.murraysetnosy: + r.david.murray, rbcollins
messages: + msg249073
2015-08-24 19:21:03Alexander Oblovatniysetmessages: + msg249070
2015-08-24 19:19:55Alexander Oblovatniycreate