Skip to content

bpo-36144: Fixed test in test_collections.py #19221

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.

Sign up for GitHub

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

Jump to bottom
Merged
gvanrossum merged 1 commit into from
Mar 30, 2020
Merged

bpo-36144: Fixed test in test_collections.py #19221

gvanrossum merged 1 commit into from
Mar 30, 2020

Conversation

@curtisbucher
Copy link
Contributor

@curtisbucher curtisbucher commented Mar 30, 2020 •
edited by bedevere-bot
Loading

We should be using .copy() to compare key, value pairs and ChainMap in line 257. Otherwise it does not properly check union behavior.
@brandtbucher

https://bugs.python.org/issue36144

@curtisbucher
Fix bug in test_collections.py
0bd70cc 
Test in TestChainMap() line 257 did not properly check union behavior.
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Mar 30, 2020
brandtbucher
brandtbucher approved these changes Mar 30, 2020
View reviewed changes
Copy link
Member

@brandtbucher brandtbucher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch!

@brandtbucher brandtbucher requested a review from gvanrossum March 30, 2020 00:14
@brandtbucher
Copy link
Member

brandtbucher commented Mar 30, 2020

Ubuntu failure looks unrelated.

@aeros
Copy link
Contributor

aeros commented Mar 30, 2020

@brandtbucher

Ubuntu failure looks unrelated.

Yeah it's definitely unrelated, since it failed in the "Install Dependencies" stage. I'll close and re-open the PR to restart the CI checks.

(For some reason, GitHub Actions retains the old failing checks on the same commit, but should still prove that the failure was unrelated to the PR).

@aeros aeros closed this Mar 30, 2020
@aeros aeros reopened this Mar 30, 2020
aeros
aeros approved these changes Mar 30, 2020
View reviewed changes
Copy link
Contributor

@aeros aeros left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @curtisbucher.

We should be using .copy() to compare key, value pairs and ChainMap in line 257. Otherwise it does not properly check union behavior.

Agreed; if it's the same object, the test doesn't work as intended. After cm3 |= pairs, even the following would pass:

self.assertEqual(cm3.maps, [cm3.maps[0] | dict(), *cm3.maps[1:]])

(which somewhat defeats the purpose of the test)

@gvanrossum gvanrossum merged commit 0c5ad54 into python:master Mar 30, 2020
@bedevere-bot
Copy link

bedevere-bot commented Mar 30, 2020

@gvanrossum: Please replace # with GH- in the commit message next time. Thanks!

@curtisbucher curtisbucher deleted the chainmap584 branch May 5, 2020 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes

@brandtbucher brandtbucher brandtbucher approved these changes

@rhettinger rhettinger Awaiting requested review from rhettinger

+1 more reviewer

@aeros aeros aeros approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@curtisbucher @brandtbucher @aeros @bedevere-bot @gvanrossum @the-knights-who-say-ni