bpo-46301: cover uncomparable values in `Enum._convert_` by sobolevn · Pull Request #30472 · python/cpython · GitHub

https://bugs.python.org/issue46301

Moreover, _convert_ was not tested on plain Enum

_convert_ was not intended to be called on plain Enum -- it's purpose it to convert existing constants in a module to enums, and existing constants will already be str or int (or possibly something else, like complex).

Change the UNCOMPARABLE_* values as suggested above, and also add a test using a complex type.

@AlexWaygood can you please add "skip news" to this PR? I can't add labels myself 🙂

All we need for this test is for UNCOMPARABLE_* to be different types of values:

UNCOMPABLE_A = 5
UNCOMPABLE_C = (9, 4)
UNCOMPABLE_B = 'hello'

Change the UNCOMPARABLE_* values as suggested above, and also add a test using a complex type.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be put in the comfy chair!

Thanks! I've added one more case with complex and simplified UNCOMPARABLE_ defs.

I have made the requested changes; please review again

Thanks for making the requested changes!

@ethanfurman: please review the changes made to this pull request.

Thank you! 👍
Do we need to backport this?

Seems that this PR has broken the refleak buildbots:

https://buildbot.python.org/all/#/builders/320/builds/269/
https://buildbot.python.org/all/#/builders/384/builds/255

According to our buildbot policy, if this is not fixed in 24h we will need to revert

@pablogsal thanks for the info! I would love to help to get this fixed, but I am not familiar with how refleak works. And since this is a test-only PR I am bit surprised that this happened 🤔

test_enum leaked [220, 220, 220] references, sum=660
test_enum leaked [56, 56, 56] memory blocks, sum=168
.test test_unittest failed -- multiple errors occurred; run in verbose mode for details
0:53:31 load avg: 0.00 Re-running failed tests in verbose mode
0:53:31 load avg: 0.00 Re-running test_enum in verbose mode
test_enum leaked [220, 220, 220] references, sum=660
test_enum leaked [56, 56, 56] memory blocks, sum=168

From https://buildbot.python.org/all/#/builders/384/builds/255 🤔

I was able to reproduce this locally on macos with ./python.exe -m test -R : -v test_enum

test_enum leaked [220, 220, 220, 220] references, sum=880
test_enum leaked [56, 56, 56, 56] memory blocks, sum=224
test_enum failed (reference leak) in 39.1 sec

== Tests result: FAILURE ==

1 test failed:
    test_enum

Total duration: 39.1 sec
Tests result: FAILURE

PYTHONDUMPREFSFILE=ex.txt ./python.exe -m test -R : -v test_enum produces this:
ex.txt

I am not at my computer so I cannot test, but the problem that happens in these cases where only tests are added is that the new tests are either modifying the global state or adding some sort of cache.

Ok, looks like I've found the problem:

cpython/Lib/enum.py

Line 833 in 1bee9a4

body['__module__'] = module

When I comment this line out - tests pass, with it - we have a refleak.

Ok, looks like I've found the problem:

Yeah, this is an example of "modifying the global state". Tests should be idempotent, and they should leave everything as it was before

I am not at my computer so I cannot test, but the problem that happens in these cases where only tests are added is that the new tests are either modifying the global state or adding some sort of cache.

Yes, you are right! These tests do modify the global state. They add a new class to the current module.

Yeah, this is an example of "modifying the global state". Tests should be idempotent, and they should leave everything as it was before

But this line is from enum.py, not from tests 🤔

Either way, I will open a new PR with test-with-buildbots later today. I hope that I will be able to fix it!

The line body['__module__'] = module is not modifying global state: body is a dict that will be used to create a new enum class.

The clue was good, though -- the global constants that are converted into enums need to be reset to their original values, either before or after the test runs.

…-30472)" This reverts commit 8d59d25.

@sobolevn add

    global COMPLEX_A, COMPLEX_B, COMPLEX_C

to the top of test_convert_complex, and

    COMPLEX_A = 2j
    COMPLEX_B = 3j
    COMPLEX_C = 1j

to the bottom. Make similar changes to test_convert_uncomparable.

Oh, sorry, I've missed your comment in the morning 🙂
I went with .swap_item technique. This ensures that we don't polute real modules.

bpo-46301: cover uncomparable values in Enum._convert_

da500c2

sobolevn requested a review from ethanfurman as a code owner January 7, 2022 23:16

the-knights-who-say-ni added the CLA signed label Jan 7, 2022

bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Jan 7, 2022

sobolevn commented Jan 7, 2022

View reviewed changes

AlexWaygood added the skip news label Jan 7, 2022

ethanfurman requested changes Jan 7, 2022

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jan 7, 2022

sobolevn mentioned this pull request Jan 8, 2022

skip-news and type-tests badges python/core-workflow#421

Closed

Address review

21d5672

bedevere-bot added awaiting change review and removed awaiting changes labels Jan 8, 2022

bedevere-bot requested a review from ethanfurman January 8, 2022 08:09

ethanfurman approved these changes Jan 8, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jan 8, 2022

ethanfurman merged commit 8d59d25 into python:main Jan 8, 2022

bedevere-bot removed the awaiting merge label Jan 8, 2022

ethanfurman added a commit that referenced this pull request Jan 10, 2022

Revert "bpo-46301 : [Enum] test uncomparable values in _convert_ (GH…

ef02fd4

…-30472)" This reverts commit 8d59d25.

sobolevn mentioned this pull request Jan 10, 2022

bpo-46301: remove refleak from Enum tests #30510

Merged

                                        [], msg='Names other than CONVERT_TEST_* found.')
+                  def test_convert_uncomparable(self):
+                      uncomp = enum.Enum._convert_(

               CONVERT_STRING_TEST_NAME_E = 5
               CONVERT_STRING_TEST_NAME_F = 5
+              # We also need values that cannot be compared:

bpo-46301: cover uncomparable values in `Enum._convert_` by sobolevn · Pull Request #30472 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-46301: cover uncomparable values in Enum._convert_ - #30472

bpo-46301: cover uncomparable values in Enum._convert_#30472ethanfurman merged 2 commits intopython:mainpython/cpython:mainfrom sobolevn:issue-46301sobolevn/cpython:issue-46301Copy head branch name to clipboard

Conversation

sobolevn commented Jan 7, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn Jan 7, 2022

Choose a reason for hiding this comment

Uh oh!

ethanfurman Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ethanfurman Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sobolevn commented Jan 7, 2022

Uh oh!

ethanfurman Jan 7, 2022

Choose a reason for hiding this comment

Uh oh!

ethanfurman Jan 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jan 7, 2022

Uh oh!

sobolevn commented Jan 8, 2022

Uh oh!

bedevere-bot commented Jan 8, 2022

Uh oh!

sobolevn commented Jan 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Jan 9, 2022

Uh oh!

sobolevn commented Jan 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn commented Jan 9, 2022

Uh oh!

sobolevn commented Jan 9, 2022

Uh oh!

pablogsal commented Jan 9, 2022

Uh oh!

sobolevn commented Jan 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pablogsal commented Jan 9, 2022

Uh oh!

sobolevn commented Jan 9, 2022

Uh oh!

ethanfurman commented Jan 10, 2022

Uh oh!

ethanfurman commented Jan 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn commented Jan 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-46301: cover uncomparable values in Enum._convert_#30472
ethanfurman merged 2 commits into
python:mainfrom
sobolevn:issue-46301

sobolevn commented Jan 7, 2022 •
edited by bedevere-bot
Loading

ethanfurman Jan 7, 2022 •
edited
Loading

ethanfurman Jan 7, 2022 •
edited
Loading

ethanfurman Jan 7, 2022 •
edited
Loading

sobolevn commented Jan 8, 2022 •
edited
Loading

sobolevn commented Jan 9, 2022 •
edited
Loading

sobolevn commented Jan 9, 2022 •
edited
Loading

ethanfurman commented Jan 10, 2022 •
edited
Loading