[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Raymond Hettinger]

Change by Raymond Hettinger :


--
resolution:  -> fixed
stage: patch review -> resolved
status: open -> closed
versions: +Python 3.9 -Python 3.7, Python 3.8

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Chris Withers]

Chris Withers  added the comment:


New changeset 1d0c5e16eab29d55773cc4196bb90d2bf12e09dd by Chris Withers 
(Emmanuel Arias) in branch 'master':
bpo-24928: Add test case for patch.dict using OrderedDict (GH -11437)
https://github.com/python/cpython/commit/1d0c5e16eab29d55773cc4196bb90d2bf12e09dd


--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Chris Withers]

Chris Withers  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Emmanuel Arias]

Emmanuel Arias  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Berker Peksag]

Change by Berker Peksag :


--
pull_requests:  -10872

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Berker Peksag]

Berker Peksag  added the comment:

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.

--
nosy: +berker.peksag

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Emmanuel Arias]

Emmanuel Arias  added the comment:

Ping :-)

Thanks Karthikeyan for the PR review. 

Would someone else like review it, please?

Thanks!
Regards

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Emmanuel Arias]

Change by Emmanuel Arias :


--
pull_requests: +10871, 10872, 10873

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Emmanuel Arias]

Change by Emmanuel Arias :


--
pull_requests: +10871, 10872

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Emmanuel Arias]

Change by Emmanuel Arias :


--
pull_requests: +10871

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

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!

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Emmanuel Arias]

Emmanuel Arias  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

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)

--
versions:  -Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Emmanuel Arias]

Emmanuel Arias  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Vaibhav Gupta]

Vaibhav Gupta  added the comment:

Hi Emmanuel

Please go ahead and make a PR. :)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Emmanuel Arias]

Emmanuel Arias  added the comment:

ping Vaibhav :-)

> I would like to make a PR for this.

--
nosy: +eamanu

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Vaibhav Gupta]

Vaibhav Gupta  added the comment:

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?

--
nosy: +dojutsu-user

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Mario Corchero]

Mario Corchero  added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

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.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Chris Withers]

Chris Withers  added the comment:

More tests are generally a good thing, so go for it :-)

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Karthikeyan Singaravelan]

Karthikeyan Singaravelan  added the comment:

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.

--
nosy: +cjw296, mariocj89, xtreak
versions: +Python 3.7, Python 3.8 -Python 3.4, Python 3.5

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Michael Foord]

Michael Foord added the comment:

Patch looks good to me.

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Raymond Hettinger]

Changes by Raymond Hettinger :


--
assignee:  -> michael.foord

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Serhiy Storchaka]

Changes by Serhiy Storchaka :


--
keywords: +needs review
nosy: +michael.foord
stage: needs patch -> patch review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Yu Tomita]

Yu Tomita added the comment:

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.

--
keywords: +patch
nosy: +nekobon
Added file: http://bugs.python.org/file40488/issue24928.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Adam]

Changes by Adam :


--
nosy: +azsorkin

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[R. David Murray]

Changes by R. David Murray rdmur...@bitdance.com:


--
keywords: +easy
stage:  - needs patch
versions: +Python 3.5, Python 3.6 -Python 2.7

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Alexander Oblovatniy]

New submission from Alexander Oblovatniy:

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!

--
components: Library (Lib)
messages: 249069
nosy: Alexander Oblovatniy
priority: normal
severity: normal
status: open
title: mock.patch.dict spoils order of items in collections.OrderedDict
type: behavior
versions: Python 2.7, Python 3.4

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Alexander Oblovatniy]

Alexander Oblovatniy added the comment:

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!

--

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[R. David Murray]

R. David Murray added the comment:

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

--
nosy: +r.david.murray, rbcollins

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue24928] mock.patch.dict spoils order of items in collections.OrderedDict

[Raymond Hettinger]

Raymond Hettinger added the comment:

 Perhaps replacing the dict call with a copy call would be sufficient?

I think that would do it.

--
nosy: +rhettinger

___
Python tracker rep...@bugs.python.org
http://bugs.python.org/issue24928
___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com