gh-136336: add tests for PySys_Audit() and PySys_AuditTuple() #136337

alex-semenyuk · 2025-07-06T10:52:33Z

Cover by tests PySys_Audit() and PySys_AuditTuple() as mentioned at TODO

Issue: Add tests for PySys_Audit() and PySys_AuditTuple() #136336

LamentXU123

Edited: In general (or at least formar test functions) we put a comment of the tested function at the first line

LamentXU123 · 2025-07-06T11:34:58Z

Lib/test/test_capi/test_sys.py

+ sys_audittuple = _testlimitedcapi.sys_audittuple
+
+ # Test with audit hook to verify internal behavior


Suggested change

sys_audittuple = _testlimitedcapi.sys_audittuple

# Test with audit hook to verify internal behavior

# Test PySys_AuditTuple()

sys_audittuple = _testlimitedcapi.sys_audittuple

# Test with audit hook to verify internal behavior

Lib/test/test_capi/test_sys.py

serhiy-storchaka · 2025-07-06T12:02:24Z

Modules/_testlimitedcapi/sys.c

+ return NULL;
+ }
+
+ if (!PyTuple_Check(tuple_args)) {


Don't check the argument here. Let PySys_AuditTuple() to check it. And test it with wrong type of argument.

serhiy-storchaka · 2025-07-06T12:05:13Z

Modules/_testlimitedcapi/sys.c

+ const char *event;
+ PyObject *tuple_args;
+
+ if (!PyArg_ParseTuple(args, "sO", &event, &tuple_args)) {


Make the second argument optional and NULL by default. This will allow you to test with NULL.

Use z# for the first argument. This will allow you to test with non-UTF-8 bytestring and with NULL.

serhiy-storchaka · 2025-07-06T12:05:56Z

Modules/_testlimitedcapi/sys.c

+ const char *argFormat;
+ PyObject *arg1 = NULL, *arg2 = NULL, *arg3 = NULL;
+
+ if (!PyArg_ParseTuple(args, "ss|OOO", &event, &argFormat, &arg1, &arg2, &arg3)) {


Use z# for even and format. Test with non-UTF-8 bytestring and with NULL.

serhiy-storchaka · 2025-07-06T12:09:07Z

Lib/test/test_capi/test_sys.py

+ self.assertRaises(TypeError, sys_audittuple, 123, ("arg",))
+ self.assertRaises(TypeError, sys_audittuple, None, ("arg",))
+ self.assertRaises(TypeError, sys_audittuple, ["not", "a", "string"], ("arg",))
+
+ self.assertRaises(TypeError, sys_audittuple, "test.event", "not_a_tuple")
+ self.assertRaises(TypeError, sys_audittuple, "test.event", ["list", "not", "tuple"])
+ self.assertRaises(TypeError, sys_audittuple, "test.event", {"dict": "not_tuple"})
+ self.assertRaises(TypeError, sys_audittuple, "test.event", None)


These tests don't test PySys_AuditTuple. They test the wrapper.

serhiy-storchaka · 2025-07-06T12:10:57Z

Lib/test/test_capi/test_sys.py

+ result = sys_audittuple("cpython.run_file", ())
+ self.assertEqual(result, 0)
+
+ result = sys_audittuple("ctypes.dlopen", ("libc.so.6",))
+ self.assertEqual(result, 0)
+
+ result = sys_audittuple("sqlite3.connect", ("test.db",))
+ self.assertEqual(result, 0)


What do these test add in comparison with the above tests?

serhiy-storchaka · 2025-07-06T12:12:54Z

Lib/test/test_capi/test_sys.py

+ audit_events = []
+ def audit_hook(event, args):
+ audit_events.append((event, args))
+ return None


This is redundant.

Suggested change

return None

serhiy-storchaka · 2025-07-06T12:13:25Z

Lib/test/test_capi/test_sys.py

+ audit_events.append((event, args))
+ return None
+
+ import sys


Import it at the top level.

serhiy-storchaka · 2025-07-06T12:19:58Z

Lib/test/test_capi/test_sys.py

+ result = sys_audittuple("cpython.run_command", ())
+ self.assertEqual(result, 0)
+ self.assertEqual(len(audit_events), 1)
+ self.assertEqual(audit_events[-1][0], "cpython.run_command")
+ self.assertEqual(audit_events[-1][1], ())
+
+ result = sys_audittuple("os.chdir", ("/tmp",))
+ self.assertEqual(result, 0)
+ self.assertEqual(len(audit_events), 2)
+ self.assertEqual(audit_events[-1][0], "os.chdir")
+ self.assertEqual(audit_events[-1][1], ("/tmp",))
+
+ result = sys_audittuple("open", ("test.txt", "r", 0))
+ self.assertEqual(result, 0)
+ self.assertEqual(len(audit_events), 3)
+ self.assertEqual(audit_events[-1][0], "open")
+ self.assertEqual(audit_events[-1][1], ("test.txt", "r", 0))
+
+ test_dict = {"key": "value"}
+ test_list = [1, 2, 3]
+ result = sys_audittuple("test.objects", (test_dict, test_list))
+ self.assertEqual(result, 0)
+ self.assertEqual(len(audit_events), 4)
+ self.assertEqual(audit_events[-1][0], "test.objects")
+ self.assertEqual(audit_events[-1][1][0], test_dict)
+ self.assertEqual(audit_events[-1][1][1], test_list)
+
+ result = sys_audittuple("test.complex", ("text", 3.14, True, None))
+ self.assertEqual(result, 0)
+ self.assertEqual(len(audit_events), 5)
+ self.assertEqual(audit_events[-1][0], "test.complex")
+ self.assertEqual(audit_events[-1][1][0], "text")
+ self.assertEqual(audit_events[-1][1][1], 3.14)
+ self.assertEqual(audit_events[-1][1][2], True)
+ self.assertEqual(audit_events[-1][1][3], None)


There are too many tests which don't add much to coverage. You only need one test with non-empty tuple, one test with NULL as the second argument, one test with non-tuple as the second argument, and few tests for the event argument: non-ASCII string, non-UTF-8 bytes string, NULL.

serhiy-storchaka

Thank you for your update. Here is a new batch of comments.

serhiy-storchaka · 2025-07-09T03:38:38Z

Modules/_testlimitedcapi/sys.c

+ if (event == NULL) {
+ RETURN_INT(0);
+ }
+
+ if (tuple_args != NULL && !PyTuple_Check(tuple_args)) {
+ PyErr_SetString(PyExc_TypeError, "second argument must be a tuple");
+ return NULL;
+ }
+


We should test how PySys_AuditTuple() handles this.

Suggested change

if (event == NULL) {

RETURN_INT(0);

}

if (tuple_args != NULL && !PyTuple_Check(tuple_args)) {

PyErr_SetString(PyExc_TypeError, "second argument must be a tuple");

return NULL;

}

serhiy-storchaka · 2025-07-09T03:39:16Z

Lib/test/test_capi/test_sys.py

+ audit_events = []
+ def audit_hook(event, args):
+ audit_events.append((event, args))
+ return None


Suggested change

return None

serhiy-storchaka · 2025-07-09T03:49:17Z

Lib/test/test_capi/test_sys.py

+ with self.assertRaises(UnicodeDecodeError):
+ sys_audittuple(b"test.non_utf8\xff", (1,))
+ finally:
+ sys.audit_hooks = []


Unfortunately, this does not work. There is no way to remove the audit hook. This is why these functions remained untested -- because the only way to test them is to use a subprocess. Look like other audit tests are implemented.

serhiy-storchaka · 2025-07-09T03:53:10Z

Modules/_testlimitedcapi/sys.c

+ if (event == NULL) {
+ RETURN_INT(0);
+ }
+


Suggested change

if (event == NULL) {

RETURN_INT(0);

}

serhiy-storchaka · 2025-07-09T04:05:36Z

Lib/test/test_capi/test_sys.py

+
+ with self.assertRaises(UnicodeDecodeError):
+ sys_audit(b"test.non_utf8\xff", "O", 1)
+ finally:


Add also tests for sys_audit("test.event", "("), sys_audit("test.event", "&"), sys_audit("test.event", b"\xff") and sys_audit("test.event", "{OO}", [], []).

Add also tests for sys_audit(NULL, "O", 1) and sys_audit("test.event", NULL). If they crash, just add comments.

serhiy-storchaka · 2025-07-09T04:05:59Z

Lib/test/test_capi/test_sys.py

+
+ with self.assertRaises(UnicodeDecodeError):
+ sys_audittuple(b"test.non_utf8\xff", (1,))
+ finally:


Add also test for sys_audittuple(NULL, (1,)). If it crashes, just add a comment, like in other tests above.

pythongh-136336: add tests for PySys_Audit() and PySys_AuditTuple()

2b65d6b

bedevere-app bot added the awaiting review label Jul 6, 2025

bedevere-app bot mentioned this pull request Jul 6, 2025

Add tests for PySys_Audit() and PySys_AuditTuple() #136336

Open

LamentXU123 reviewed Jul 6, 2025

View reviewed changes

serhiy-storchaka reviewed Jul 6, 2025

View reviewed changes

StanFromIreland added the skip news label Jul 6, 2025

Fix based on comments

a0faa3f

serhiy-storchaka reviewed Jul 9, 2025

View reviewed changes

Address comments

5b20d17

alex-semenyuk force-pushed the add_tests_sys branch from b659194 to 5b20d17 Compare July 9, 2025 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

gh-136336: add tests for PySys_Audit() and PySys_AuditTuple() #136337

gh-136336: add tests for PySys_Audit() and PySys_AuditTuple() #136337

alex-semenyuk commented Jul 6, 2025 •

edited by bedevere-app bot

Loading

LamentXU123 left a comment •

edited

Loading

LamentXU123 Jul 6, 2025

Uh oh!

serhiy-storchaka Jul 6, 2025

serhiy-storchaka Jul 6, 2025

serhiy-storchaka Jul 6, 2025

serhiy-storchaka Jul 6, 2025

serhiy-storchaka Jul 6, 2025

serhiy-storchaka Jul 6, 2025

serhiy-storchaka Jul 9, 2025

serhiy-storchaka Jul 6, 2025

serhiy-storchaka Jul 6, 2025

serhiy-storchaka left a comment

serhiy-storchaka Jul 9, 2025

serhiy-storchaka Jul 9, 2025

serhiy-storchaka Jul 9, 2025

serhiy-storchaka Jul 9, 2025

serhiy-storchaka Jul 9, 2025

serhiy-storchaka Jul 9, 2025

		sys_audittuple = _testlimitedcapi.sys_audittuple

		# Test with audit hook to verify internal behavior

Uh oh!

gh-136336: add tests for PySys_Audit() and PySys_AuditTuple() #136337

Are you sure you want to change the base?

gh-136336: add tests for PySys_Audit() and PySys_AuditTuple() #136337

Conversation

alex-semenyuk commented Jul 6, 2025 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

LamentXU123 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alex-semenyuk commented Jul 6, 2025 •

edited by bedevere-app bot

Loading

LamentXU123 left a comment •

edited

Loading