gh-111495: Add tests for PyCodec_* C API
#123343
Conversation
I think you should open an issue and create a separate pr with bugfix. |
Good idea. I separated the commit so that it can easily be cherry-picked (hehe). |
41c01a6 to
8048ae1
Compare
There was a problem hiding this comment.
A test does crash if run more than once.
$ ./python -m test -m test_codec_error_handlers test_capi.test_codecs
0:00:00 load avg: 2.46 [1/2] test_capi.test_codecs
0:00:00 load avg: 2.46 [2/2] test_capi.test_codecs
Fatal Python error: Segmentation fault
Current thread 0x00007f8ceb3a3740 (most recent call first):
File "/home/vstinner/python/main/Lib/test/test_capi/test_codecs.py", line 800 in test_codec_error_handlers
File "/home/vstinner/python/main/Lib/unittest/case.py", line 606 in _callTestMethod
(...)
I was trying to check if there are refleaks.
|
So we need a way to unregister the codecs. Otherwise I don't know how to do it... (would reloading the module work? or do you want an API for explicitly unregistering an error handler?) |
That would be nice, yes. It should be a separated PR if you want to work on that. I'm not sure if it's strictly needed right now if your test creates/loads a separated extension just for C tests. But the test should not leak :-) |
I'll work on that tomorrow (I wanted to work today but I have no energy at all).
Before working on the above feature, I'll try to see how I can make it work without it. (at least we should be able to check if it leaks...). |
|
So, the issue is with So something is wrong when I add those lines and uses self.enterContext(import_helper.isolated_modules())
self.enterContext(import_helper.CleanImport('codecs'))(Still searching where it leaks...) So I managed to fix the leaks. The crash seems to be because of this line in if (ucnhash_capi->getname(c, buffer, sizeof(buffer), 1))I have no idea why no using fresh imports seem fine. However, the |
|
Why do you need a fresh import? |
|
I don't need them anymore so I can remove them normally. I'll now work on the PR for removing a registered error handler so that we can run the test suite. For now, the PR is not ready since there will be a leak due to the error handler being held. |
850a441 to
7be1f55
Compare
|
Mmh, apparently tests were backported for the other modules, but this one relies on the internal private feature of being able to unregister error handlers. I don't really want to add it 3.12 and 3.13 so I think we can keep these tests for 3.14+. |
|
Yeah, it makes sense to not backport these tests. |
|
Merged, thanks for this nice enhancement! |
By adding a test, I found a bug in
PyUnicodeEncodeError_GetStartwhere constructing anUnicodeEncodeErrorwith start = 0 and an empty string object raises an assertion error. While this should not happen at runtime, this is something that should still be avoided.Test coverage is currently insufficient I think but I want an initial feedback first.
EDIT 1: Tests that are known to crash due to UnicodeError's start handling are disabled. I'd like to merge this one even though UnicodeError crashes (I removed the temporary fix from this PR).