Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32.4k
gh-136336: add tests for PySys_Audit() and PySys_AuditTuple()#136337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
LamentXU123 left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Edited: In general (or at least formar test functions) we put a comment of the tested function at the first line
Lib/test/test_capi/test_sys.py Outdated
| sys_audittuple = _testlimitedcapi.sys_audittuple | ||
| # Test with audit hook to verify internal behavior |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| 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 |
Uh oh!
There was an error while loading.Please reload this page.
Modules/_testlimitedcapi/sys.c Outdated
| return NULL; | ||
| } | ||
| if (!PyTuple_Check(tuple_args)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Don't check the argument here. LetPySys_AuditTuple() to check it. And test it with wrong type of argument.
Modules/_testlimitedcapi/sys.c Outdated
| const char *event; | ||
| PyObject *tuple_args; | ||
| if (!PyArg_ParseTuple(args, "sO", &event, &tuple_args)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Make the second argument optional and NULL by default. This will allow you to test with NULL.
Usez# for the first argument. This will allow you to test with non-UTF-8 bytestring and with NULL.
Modules/_testlimitedcapi/sys.c Outdated
| const char *argFormat; | ||
| PyObject *arg1 = NULL, *arg2 = NULL, *arg3 = NULL; | ||
| if (!PyArg_ParseTuple(args, "ss|OOO", &event, &argFormat, &arg1, &arg2, &arg3)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Usez# for even and format. Test with non-UTF-8 bytestring and with NULL.
Lib/test/test_capi/test_sys.py Outdated
| 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
These tests don't testPySys_AuditTuple. They test the wrapper.
Lib/test/test_capi/test_sys.py Outdated
| 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What do these test add in comparison with the above tests?
Lib/test/test_capi/test_sys.py Outdated
| audit_events = [] | ||
| def audit_hook(event, args): | ||
| audit_events.append((event, args)) | ||
| return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This is redundant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| return None |
Lib/test/test_capi/test_sys.py Outdated
| audit_events.append((event, args)) | ||
| return None | ||
| import sys |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Import it at the top level.
Lib/test/test_capi/test_sys.py Outdated
| 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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thank you for your update. Here is a new batch of comments.
Modules/_testlimitedcapi/sys.c Outdated
| 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; | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We should test howPySys_AuditTuple() handles this.
| if (event==NULL) { | |
| RETURN_INT(0); | |
| } | |
| if (tuple_args!=NULL&& !PyTuple_Check(tuple_args)) { | |
| PyErr_SetString(PyExc_TypeError,"second argument must be a tuple"); | |
| returnNULL; | |
| } |
Lib/test/test_capi/test_sys.py Outdated
| audit_events = [] | ||
| def audit_hook(event, args): | ||
| audit_events.append((event, args)) | ||
| return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| return None |
Lib/test/test_capi/test_sys.py Outdated
| with self.assertRaises(UnicodeDecodeError): | ||
| sys_audittuple(b"test.non_utf8\xff", (1,)) | ||
| finally: | ||
| sys.audit_hooks = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Modules/_testlimitedcapi/sys.c Outdated
| if (event == NULL) { | ||
| RETURN_INT(0); | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| if (event==NULL) { | |
| RETURN_INT(0); | |
| } |
Lib/test/test_capi/test_sys.py Outdated
| with self.assertRaises(UnicodeDecodeError): | ||
| sys_audit(b"test.non_utf8\xff", "O", 1) | ||
| finally: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Add also tests forsys_audit("test.event", "("),sys_audit("test.event", "&"),sys_audit("test.event", b"\xff") andsys_audit("test.event", "{OO}", [], []).
Add also tests forsys_audit(NULL, "O", 1) andsys_audit("test.event", NULL). If they crash, just add comments.
Lib/test/test_capi/test_sys.py Outdated
| with self.assertRaises(UnicodeDecodeError): | ||
| sys_audittuple(b"test.non_utf8\xff", (1,)) | ||
| finally: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Add also test forsys_audittuple(NULL, (1,)). If it crashes, just add a comment, like in other tests above.
Uh oh!
There was an error while loading.Please reload this page.
Cover by tests PySys_Audit() and PySys_AuditTuple() as mentioned atTODO