Skip to content

bpo-32107: Remove the check of the multicast bit in test_uuid.#4572

Closed
serhiy-storchaka wants to merge 1 commit intopython:masterfrom
serhiy-storchaka:test-uuid-multicast-bit
Closed

bpo-32107: Remove the check of the multicast bit in test_uuid.#4572
serhiy-storchaka wants to merge 1 commit intopython:masterfrom
serhiy-storchaka:test-uuid-multicast-bit

Conversation

@serhiy-storchaka
Copy link
Copy Markdown
Member

@serhiy-storchaka serhiy-storchaka commented Nov 26, 2017

While it is required that the multicast bit (1<<40) should be set in random generated addresses, there is no requirement that it should be cleared in addresses obtained from network cards.

https://bugs.python.org/issue32107

While it is required that the multicast bit (1<<40) should be set in
random generated addresses, there is no requirement that it should be
cleared in addresses obtained from network cards.
@warsaw
Copy link
Copy Markdown
Member

warsaw commented Nov 26, 2017

I don't really want to land this in isolation of #4494. I see what you're saying now that the comment was wrong, but it was also really confusing because it never mentioned the multicast bit. So I had to infer what the intention was, and that led me down the path of #4494, which I still think is a good change to make.

The question is whether it's enough to ensure that the MAC address is within the 48-bit range, as that's the only test you have left in this branch. My branch also tests that the returned MAC is a universally administered MAC, which can't succeed on Travis because Travis has no universally administered MACs, as the comments on that PR explains.

Copy link
Copy Markdown
Member

@warsaw warsaw left a comment

Choose a reason for hiding this comment

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

I agree that we shouldn't be testing the multicast bit, since that's only guaranteed for random MACs. I really don't want to land just this PR without landing #4494 also, or instead.

@bedevere-bot
Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

warsaw added a commit to warsaw/cpython that referenced this pull request Nov 26, 2017
…torchaka/cpython into test_uuid

This merges the relevant bits of python#4572 and should be considered instead of
that PR.
@warsaw
Copy link
Copy Markdown
Member

warsaw commented Nov 26, 2017

@serhiy-storchaka - I've merged this branch into #4494 and tweaked it just a bit, and added comments. Can we please approve and land #4494 instead?

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

Labels

awaiting changes skip news tests Tests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants