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

bpo-24928: Add test case for patch.dict using OrderedDict #11437

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@eamanu
Copy link
Contributor

eamanu commented Jan 5, 2019

bpo-24928: mock.patch.dict spoils order of items in collections.OrderedDict

This PR add test case for patch.dict using OrderedDict.

This is just test, don't need NEWS.

Co-authored-by: Yu Tomita nekobon@users.noreply.github.com

https://bugs.python.org/issue24928

@ZackerySpytz

This comment has been minimized.

Copy link
Contributor

ZackerySpytz commented Jan 5, 2019

Your PR is almost an exact copy of the Lib/unittest/test/testmock/testpatch.py changes
in https://bugs.python.org/file40488/issue24928.patch. That patch wasn't written
by you, so you should provide attribution to the original author, as stated in the developer's guide
(https://devguide.python.org/pullrequest/#converting-an-existing-patch-from-b-p-o-to-github).

original = foo.copy()
update_values = zip('cdefghijklmnopqrstuvwxyz', range(26))

@patch.dict(foo, OrderedDict(update_values))

This comment has been minimized.

@tirkarthi

tirkarthi Jan 5, 2019

Contributor

The assert looks incorrect that it's testing list(foo) == sorted(foo) but it should actually be testing whether foo contains update_values which on testing foo['c'] gives KeyError. Sorry, I just noticed it now since the tests passed with original patch.

I checked this and update_values is a zip that becomes empty once it's iterated over and can be converted to a list to make sure that update_values can be used for subsequent calls. So how about below to make sure order is preserved?

update_values = list(zip('cdefghijklmnopqrstuvwxyz', range(26)))
patched_items = list(foo.items()) + update_values

with patch.dict(foo, OrderedDict(update_values)):
    self.assertEqual(list(foo.items()), patched_items)

I would personally prefer a context manager here though other tests use @patch.dict with test() but that can wait for other reviewers.

This comment has been minimized.

@eamanu

eamanu Jan 6, 2019

Contributor

Hi @tirkarthi

Yes I checked it manually and the foo['c'] gives the KeyError. I will change for the list(zip()) that you say. And I will use context manager, I think that is more readable.

Maybe, we could think about support convert zip to dict on OrderedDict()?

This comment has been minimized.

@tirkarthi

tirkarthi Jan 6, 2019

Contributor

Maybe, we could think about support convert zip to dict on OrderedDict()?

Sorry, are you saying to use patch.dict(foo, OrderedDict(dict(update_values))) ? dict guarantees order currently and would help in catching the error if it changes in future. So I would prefer having OrderedDict(update_values).

This comment has been minimized.

@eamanu

eamanu Jan 7, 2019

Contributor

I understand now.

@eamanu

This comment has been minimized.

Copy link
Contributor

eamanu commented Jan 5, 2019

Your PR is almost an exact copy of the Lib/unittest/test/testmock/testpatch.py changes
in https://bugs.python.org/file40488/issue24928.patch. That patch wasn't written
by you, so you should provide attribution to the original author, as stated in the developer's guide
(https://devguide.python.org/pullrequest/#converting-an-existing-patch-from-b-p-o-to-github).

Thanks!. I added it to the PR description. In the next commit I will added it on commit message. Or is there other way to do it?

improve test with coments of @tirkarthi
Co-authored-by: Yu Tomita nekobon@users.noreply.github.com
@tirkarthi
Copy link
Contributor

tirkarthi left a comment

LGTM. I will wait for review from someone else. Thanks for the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment