bpo-46301: cover uncomparable values in Enum._convert_

[sobolevn]

https://bugs.python.org/issue46301

[sobolevn]

Moreover, _convert_ was not tested on plain Enum

[ethanfurman]

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

[ethanfurman]

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

[sobolevn]

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

[ethanfurman]

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'

[ethanfurman]

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

[bedevere-bot]

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!

[sobolevn]

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

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

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

[sobolevn]

Thank you! 👍
Do we need to backport this?

[pablogsal]

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

[sobolevn]

@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 🤔

[sobolevn]

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

[sobolevn]

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

[pablogsal]

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.

[sobolevn]

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.

[pablogsal]

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

[sobolevn]

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!

[ethanfurman]

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.

[ethanfurman]

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

[sobolevn]

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.