gh-99593: Add tests for Unicode C API (part 2) by serhiy-storchaka · Pull Request #99868 · python/cpython

serhiy-storchaka · 2022-11-29T10:20:48Z

Add tests for lower-level functions.

Issue: Add tests for Unicode C API #99593

Add tests for lower-level functions.

vstinner · 2022-12-01T15:42:04Z

Lib/test/test_capi/test_unicode.py

 class CAPITest(unittest.TestCase):
+    # TODO: Test the following function:
+    #
+    #   PyUnicode_ClearFreeList


PyUnicode_ClearFreeList was removed in Python 3.9!

Oh, these tests were written when it was here.

Lib/test/test_capi/test_unicode.py

vstinner · 2022-12-01T15:48:15Z

Lib/test/test_capi/test_unicode.py

+            self.assertEqual(new(0, maxchar), '')
+            self.assertEqual(new(5, maxchar), chr(maxchar)*5)
+        self.assertEqual(new(0, 0x110000), '')
+        self.assertRaises(SystemError, new, 5, 0x110000)


Maybe this error should become a ValueError, but it can be changed outside this PR, since you seem to want to backport these news tests.

I think that SystemError is better exception type here. It is a misuse of the C API, you cannot get this error from Python code.

vstinner · 2022-12-01T15:52:41Z

Modules/_testcapi/unicode.c

+    if (!result) {
+        return NULL;
+    }
+    if (size > 0 && maxchar <= 0x10ffff &&


It sounds dangerous to return a string to the "Python space" if characters are not initialized when maxchar is greated than 0x10ffff. Can you remove maxchar <= 0x10ffff condition? PyUnicode_Fill() must fail if the fill character is too big (greater than 0x10ffff).

It is a test for PyUnicode_New(), not for PyUnicode_Fill(). We should see exceptions raised by PyUnicode_New(), not PyUnicode_Fill().

It never happens, because PyUnicode_New() returns NULL if size > 0 and maxchar > 0x10ffff. If it will not return NULL, it is better to get a malformed string than get an exception raised by PyUnicode_Fill() and think that it was raised by PyUnicode_New().

vstinner · 2022-12-01T16:00:33Z

Modules/_testcapi/unicode.c

+    }
+
+    result = PyUnicode_WriteChar(to_copy, index, (Py_UCS4)character);
+    if (result == -1 && PyErr_Occurred()) {


For me, -1 means an error. You shouldn't have to check if an exception was raised or not. Or it should become: assert(PyErr_Occurred()).

Same remark for other test wrapper like the one for PyUnicode_Resize().

If an exception was raised, but result is not -1, then what? The code will return NULL, and we will never know that something wrong happened with PyUnicode_WriteChar().

I do not use the C assert() in these tests, because I want to make tests working even in non-debug build.

I do not use the C assert() in these tests, because I want to make tests working even in non-debug build.

C assert() are enabled on release (no-debug) builds: see the following code in Modules/_testcapi/parts.h:

// Always enable assertions #undef NDEBUG

Many _testcapi tests are only implemented with assert().

Modules/_testcapi/unicode.c

vstinner · 2022-12-01T16:16:04Z

Modules/_testcapi/unicode.c


    if (!PyUnicode_AsUCS4(unicode, buffer, buf_len, copy_null)) {
-        PyMem_Free(buffer);
+        PyMem_FREE(buffer);


PyMem_FREE is a deprecated alias to PyMem_Free(). I would expect the opposite change, replace PyMem_FREE with PyMem_Free :-)

Reverted. The code in my branch was written a long time ago, when PyMem_FREE and PyMem_Free were different things. I just copied it over the current code.

Lib/test/test_capi/test_unicode.py

vstinner · 2022-12-01T16:23:16Z

Lib/test/test_capi/test_unicode.py

+        self.assertRaises(SystemError, append, 'abc', NULL)
+        # TODO: Test PyUnicode_Append() with modifiable unicode
+        # and with NULL as the address.
+        # TODO: Check reference counts.


That's done automatically by Refleaks buildbots, no?

Maybe. But since this C API does unusual things with reference counts, it would be better to test it explicitly, if possible.

vstinner · 2022-12-01T16:26:08Z

Lib/test/test_capi/test_unicode.py

+        if SIZEOF_WCHAR_T == 2:
+            if sys.byteorder == 'little':
+                encoding = 'utf-16le'
+            elif sys.byteorder == 'little':


This condition looks wrong. Maybe just use else:? I don't think that Python supports other endianness. Same a few lines below.

Good catch. I though that explicit check can be better, but made an error in it.

serhiy-storchaka · 2022-12-03T11:06:26Z

Thank you for your review Victor.

vstinner

LGTM. Thanks for my addressing my review.

vstinner · 2022-12-05T13:02:22Z

Modules/_testcapi/unicode.c

+    }
+
+    result = PyUnicode_WriteChar(to_copy, index, (Py_UCS4)character);
+    if (result == -1 && PyErr_Occurred()) {


I do not use the C assert() in these tests, because I want to make tests working even in non-debug build.

C assert() are enabled on release (no-debug) builds: see the following code in Modules/_testcapi/parts.h:

// Always enable assertions #undef NDEBUG

Many _testcapi tests are only implemented with assert().

hugovk · 2023-04-07T07:59:22Z

@serhiy-storchaka Victor approved this, ready to merge?

(I removed the 3.10 backport label, it's now security only)

vstinner

LGTM

vstinner · 2023-05-04T15:01:05Z

The CI test "DO-NOT-MERGE / unresolved review" was blocked on "Waiting for status to be reported". I tried to update the PR on main to see if it does unblock the PR.

miss-islington · 2023-05-04T15:25:13Z

Thanks @serhiy-storchaka for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

miss-islington · 2023-05-04T15:25:25Z

Sorry, @serhiy-storchaka and @vstinner, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2ba931ff727395cf89b290ed313a8e15db0bfcf1 3.11

vstinner · 2023-05-04T15:27:13Z

Merged, thanks @serhiy-storchaka.

@serhiy-storchaka: If you consider that this change should be backported to Python 3.11, go ahead. But the automated backport failed for an unknown reason.

* main: (61 commits) pythongh-64595: Argument Clinic: Touch source file if any output file changed (python#104152) pythongh-64631: Test exception messages in cloned Argument Clinic funcs (python#104167) pythongh-68395: Avoid naming conflicts by mangling variable names in Argument Clinic (python#104065) pythongh-64658: Expand Argument Clinic return converter docs (python#104175) pythonGH-103092: port `_asyncio` freelist to module state (python#104196) pythongh-104051: fix crash in test_xxtestfuzz with -We (python#104052) pythongh-104190: fix ubsan crash (python#104191) pythongh-104106: Add gcc fallback of mkfifoat/mknodat for macOS (pythongh-104129) pythonGH-104142: Fix _Py_RefcntAdd to respect immortality (pythonGH-104143) pythongh-104112: link from cached_property docs to method-caching FAQ (python#104113) pythongh-68968: Correcting message display issue with assertEqual (python#103937) pythonGH-103899: Provide a hint when accidentally calling a module (pythonGH-103900) pythongh-103963: fix 'make regen-opcode' in out-of-tree builds (python#104177) pythongh-102500: Add PEP 688 and 698 to the 3.12 release highlights (python#104174) pythonGH-81079: Add case_sensitive argument to `pathlib.Path.glob()` (pythonGH-102710) pythongh-91896: Deprecate collections.abc.ByteString (python#102096) pythongh-99593: Add tests for Unicode C API (part 2) (python#99868) pythongh-102500: Document PEP 688 (python#102571) pythongh-102500: Implement PEP 688 (python#102521) pythongh-96534: socketmodule: support FreeBSD divert(4) socket (python#96536) ...

miss-islington · 2023-07-10T13:18:46Z

Thanks @serhiy-storchaka for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

miss-islington · 2023-07-10T13:18:47Z

Sorry, @serhiy-storchaka and @vstinner, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 2ba931ff727395cf89b290ed313a8e15db0bfcf1 3.12

pythongh-99593: Add tests for Unicode C API (part 2)

f8754d7

Add tests for lower-level functions.

bedevere-bot mentioned this pull request Nov 29, 2022

Add tests for Unicode C API #99593

Closed

bedevere-bot added the awaiting core review label Nov 29, 2022

serhiy-storchaka added needs backport to 3.10 only security fixes needs backport to 3.11 only security fixes labels Nov 29, 2022

This was referenced Nov 29, 2022

gh-93649: Split unicode tests from _testcapimodule.c & add some more #95819

Merged

gh-99593: Add tests for Unicode C API #99594

Closed

serhiy-storchaka added the skip news label Nov 29, 2022

serhiy-storchaka marked this pull request as ready for review November 30, 2022 17:19

serhiy-storchaka requested a review from vstinner November 30, 2022 17:20

vstinner reviewed Dec 1, 2022

View reviewed changes

Address review comments.

1c1775c

Merge branch 'main' into test-unicode-capi-part2

4a60537

vstinner approved these changes Dec 5, 2022

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Dec 5, 2022

hugovk removed the needs backport to 3.10 only security fixes label Apr 7, 2023

Merge branch 'main' into test-unicode-capi-part2

41e8a15

vstinner enabled auto-merge (squash) May 4, 2023 14:59

vstinner approved these changes May 4, 2023

View reviewed changes

Merge branch 'main' into test-unicode-capi-part2

36970c7

vstinner merged commit 2ba931f into python:main May 4, 2023

bedevere-bot removed the awaiting merge label May 4, 2023

miss-islington assigned vstinner May 4, 2023

vstinner removed the needs backport to 3.11 only security fixes label May 4, 2023

serhiy-storchaka unassigned vstinner Jul 10, 2023

serhiy-storchaka added the needs backport to 3.12 only security fixes label Jul 10, 2023

miss-islington assigned vstinner Jul 10, 2023

serhiy-storchaka removed the needs backport to 3.12 only security fixes label Jul 10, 2023

Uh oh!

Conversation

serhiy-storchaka commented Nov 29, 2022 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka commented Dec 3, 2022

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hugovk commented Apr 7, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented May 4, 2023

Uh oh!

miss-islington commented May 4, 2023

Uh oh!

miss-islington commented May 4, 2023

Uh oh!

vstinner commented May 4, 2023

Uh oh!

miss-islington commented Jul 10, 2023

Uh oh!

miss-islington commented Jul 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

serhiy-storchaka commented Nov 29, 2022 •

edited by bedevere-bot

Loading