gh-111495: Add `PyFile_*` CAPI tests #111709

sobolevn · 2023-11-03T18:29:37Z

Looks like PyFile_SetOpenCodeHook is already tested here:

Lines 1177 to 1232 in 20cfab9

    
           static int test_open_code_hook(void) 
        
           { 
        
               int result = 0; 
        
               /* Provide a hook */ 
        
               result = PyFile_SetOpenCodeHook(_open_code_hook, &result); 
        
               if (result) { 
        
                   printf("Failed to set hook\n"); 
        
                   return 1; 
        
               } 
        
               /* A second hook should fail */ 
        
               result = PyFile_SetOpenCodeHook(_open_code_hook, &result); 
        
               if (!result) { 
        
                   printf("Should have failed to set second hook\n"); 
        
                   return 2; 
        
               } 
        
               Py_IgnoreEnvironmentFlag = 0; 
        
               _testembed_Py_InitializeFromConfig(); 
        
               result = 0; 
        
               PyObject *r = PyFile_OpenCode("$$test-filename"); 
        
               if (!r) { 
        
                   PyErr_Print(); 
        
                   result = 3; 
        
               } else { 
        
                   void *cmp = PyLong_AsVoidPtr(r); 
        
                   Py_DECREF(r); 
        
                   if (cmp != &result) { 
        
                       printf("Did not get expected result from hook\n"); 
        
                       result = 4; 
        
                   } 
        
               } 
        
               if (!result) { 
        
                   PyObject *io = PyImport_ImportModule("_io"); 
        
                   PyObject *r = io 
        
                       ? PyObject_CallMethod(io, "open_code", "s", "$$test-filename") 
        
                       : NULL; 
        
                   if (!r) { 
        
                       PyErr_Print(); 
        
                       result = 5; 
        
                   } else { 
        
                       void *cmp = PyLong_AsVoidPtr(r); 
        
                       Py_DECREF(r); 
        
                       if (cmp != &result) { 
        
                           printf("Did not get expected result from hook\n"); 
        
                           result = 6; 
        
                       } 
        
                   } 
        
                   Py_XDECREF(io); 
        
               } 
        
               Py_Finalize(); 
        
               return result; 
        
           }

Issue: Add more C API tests #111495

sobolevn · 2023-11-03T20:15:33Z

Tests fail on Windows (I have a very limited experience with this platform):

 ======================================================================
ERROR: test_file_get_line (test.test_capi.test_file.TestPyFileCAPI.test_file_get_line)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\test\test_capi\test_file.py", line 40, in test_file_get_line
    f.writelines([first_line])
  File "D:\a\cpython\cpython\Lib\encodings\cp1252.py", line 19, in encode
    return codecs.charmap_encode(input,self.errors,encoding_table)[0]
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
UnicodeEncodeError: 'charmap' codec can't encode characters in position 10-15: character maps to <undefined>

Is it correct?

serhiy-storchaka

This family has little functions, but they should be tested with many cases.

Lib/test/test_capi/test_file.py

serhiy-storchaka · 2023-11-04T11:30:46Z

Tests fail on Windows

Because the default encoding on Windows is not UTF-8. Always specify encoding for text files.

Lib/test/test_capi/test_file.py

sobolevn · 2023-11-05T14:24:28Z

@serhiy-storchaka thanks a lot for your detailed review! You are one of the best reviewers I know :)

sobolevn · 2023-11-05T15:27:23Z

Address sanitizer build fails with:

 ======================================================================
FAIL: test_string_args_as_invalid_utf (test.test_capi.test_file.TestPyFile_FromFd.test_string_args_as_invalid_utf) (arg_pos=5)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython/Lib/test/test_capi/test_file.py", line 77, in test_string_args_as_invalid_utf
    self.assertRaises(
AssertionError: (<class 'ValueError'>, <class 'LookupError'>) not raised by file_from_fd

----------------------------------------------------------------------

Maybe I should use a different string? Suggestions?

vstinner

LGTM.

vstinner · 2023-11-10T12:40:03Z

Lib/test/test_capi/test_file.py

@@ -0,0 +1,296 @@
+import unittest
+import io
+import os


nitpick: sort imports :-)

vstinner · 2023-11-10T12:57:11Z

Address sanitizer build fails with:
FAIL: test_string_args_as_invalid_utf (test.test_capi.test_file.TestPyFile_FromFd.test_string_args_as_invalid_utf) (arg_pos=5)
AssertionError: (<class 'ValueError'>, <class 'LookupError'>) not raised by file_from_fd

It's unrelated to Address sanitizer. It's just that this CI builds Python is release mode. And in release mode, the error handler is only used if the string cannot be decoded (decoding error). In debug mode, the error handler is always checked.

You can skip this test if support.Py_DEBUG is false.

vstinner · 2023-11-10T12:58:03Z

To reproduce the Address Sanitizer issue, I used:

./configure --with-address-sanitizer CC=clang
ASAN_OPTIONS='detect_leaks=0:allocator_may_return_null=1:handle_segv=0' make
ASAN_OPTIONS='detect_leaks=0:allocator_may_return_null=1:handle_segv=0' ./python -m test test_capi.test_file -v

serhiy-storchaka

Sorry, I have not finished the review yet. It is difficult with so many tests. So I can find other issues later.

The main problem is that they incorrectly create non-decodable files. You should use binary files to write them.

It would be nice also to reduce the number of lines where it is possible.

serhiy-storchaka · 2023-11-05T21:20:08Z

Lib/test/test_capi/test_file.py

+    def test_name_invalid_utf(self):
+        with open(os_helper.TESTFN, "w", encoding="utf-8") as f:
+            file_obj = _testcapi.file_from_fd(
+                f.fileno(), "abc\xe9", "w",


It is not invalid UTF-8. When you pass the Python string, it is encoded to UTF-8, therefore the C string is always valid UTF-8. You have to pass a bytes object, e.g. b'\xff'. See for example tests for PyDict_GetItemString() or PyObject_GetAttrString().

serhiy-storchaka · 2023-11-05T21:25:46Z

Lib/test/test_capi/test_file.py

+                TypeError,
+                r"open\(\) argument 'mode' must be str, not None",
+                _testcapi.file_from_fd,
+                f.fileno(), "abc\xe9", NULL,


Use TESTFN?

When the assertRaisesRegex() call is multiline, it may be clearer to write it in the context manager form. The functional form is convenient when you can fit all in a single line.

serhiy-storchaka · 2023-11-05T21:37:11Z

Lib/test/test_capi/test_file.py

+            file_obj = _testcapi.file_from_fd(
+                f.fileno(), os_helper.TESTFN, "w",
+                1, "utf-8", "strict", "\n", 0,
+            )


I would write this in more compact form. First, add shorted name from_fd = _testcapi.file_from_fd, then this call can fit in just two lines:

file_obj = from_fd(f.fileno(), os_helper.TESTFN, "w", 1, "utf-8", "strict", "\n", 0)

serhiy-storchaka · 2023-11-05T21:41:27Z

Lib/test/test_capi/test_file.py

+                f.fileno(), os_helper.TESTFN, "w",
+                1, "utf-8", "strict", "\n", 0,
+            )
+        self.assertIsInstance(file_obj, io.TextIOWrapper)


Add a check for the .name attribute.

serhiy-storchaka · 2023-11-05T21:48:17Z

Lib/test/test_capi/test_file.py

+            )
+
+    def test_string_args_as_null(self):
+        for arg_pos in (4, 5, 6):


It is better to write it without loop. So you can see what case is crashed if there is a crash. And it is easier to modify every case.

For example, I suggest to add checks for corresponding attributes (.encoding, .errors, .newlines).

serhiy-storchaka · 2023-11-06T07:24:46Z

Lib/test/test_capi/test_file.py

+    def test_file_empty_line(self):
+        first_line = ""
+        with open(os_helper.TESTFN, "w", encoding="utf-8") as f:
+            f.writelines([first_line])


No need to write an empty line.

serhiy-storchaka · 2023-11-06T07:26:39Z

Lib/test/test_capi/test_file.py

+        first_line = "\xc3\x28\n"
+        with open(os_helper.TESTFN, "w", encoding="utf-8") as f:
+            f.writelines([first_line])


Again, it does not create invalid UTF-8.

serhiy-storchaka · 2023-11-10T13:57:07Z

Lib/test/test_capi/test_file.py

+        )
+        self.assertEqual(self.write_and_return(False), "False")
+
+    def test_file_write_custom_obj(self):


Could you please add a test for object that raises an exception in __str__() or __repr__()?

serhiy-storchaka · 2023-11-10T13:58:36Z

Lib/test/test_capi/test_file.py

+        self.assertRaises(AttributeError, self.write, NULL, object(), 0)
+        self.assertRaises(TypeError, self.write, NULL, NULL, 0)


Please test these cases also with Py_PRINT_RAW.

Add cases for writing NULL to StringIO.

serhiy-storchaka · 2023-11-10T14:07:50Z

Lib/test/test_capi/test_file.py

+        with open(os_helper.TESTFN, "w", encoding="utf-8") as f:
+            f.writelines([first_line, second_line])


Many tests can use StringIO. E.g.

f = io.StringIO('first_line\nsecond_line\n')

pythongh-111495: Add PyFile_* CAPI tests

77afe78

sobolevn requested a review from serhiy-storchaka November 3, 2023 18:29

bedevere-app bot added the awaiting review label Nov 3, 2023

bedevere-app bot mentioned this pull request Nov 3, 2023

Add more C API tests #111495

Open

10 tasks

sobolevn added the skip news label Nov 3, 2023

skirpichev mentioned this pull request Nov 4, 2023

Shouldn't Sir classify changes in test modules by "tests" label? python/bedevere#605

Open

serhiy-storchaka reviewed Nov 4, 2023

View reviewed changes

Merge branch 'main' into issue-111495

340d256

skirpichev reviewed Nov 5, 2023

View reviewed changes

Lib/test/test_capi/test_file.py Outdated Show resolved Hide resolved

Address review

52c5918

vstinner approved these changes Nov 10, 2023

View reviewed changes

Lib/test/test_capi/test_file.py

@@ -0,0 +1,296 @@

import unittest

import io

import os

Copy link

Member

vstinner Nov 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: sort imports :-)

bedevere-app bot added awaiting merge and removed awaiting review labels Nov 10, 2023

serhiy-storchaka reviewed Nov 10, 2023

View reviewed changes

gh-111495: Add `PyFile_*` CAPI tests #111709

gh-111495: Add `PyFile_*` CAPI tests #111709

sobolevn commented Nov 3, 2023 •

edited by bedevere-app bot

sobolevn commented Nov 3, 2023 •

edited

serhiy-storchaka left a comment

serhiy-storchaka commented Nov 4, 2023

sobolevn commented Nov 5, 2023

sobolevn commented Nov 5, 2023

vstinner left a comment

vstinner Nov 10, 2023

vstinner commented Nov 10, 2023

vstinner commented Nov 10, 2023

serhiy-storchaka left a comment

serhiy-storchaka Nov 5, 2023

serhiy-storchaka Nov 5, 2023

serhiy-storchaka Nov 5, 2023

serhiy-storchaka Nov 5, 2023

serhiy-storchaka Nov 5, 2023

serhiy-storchaka Nov 6, 2023

serhiy-storchaka Nov 6, 2023

serhiy-storchaka Nov 10, 2023

serhiy-storchaka Nov 10, 2023

serhiy-storchaka Nov 10, 2023

	static int test_open_code_hook(void)
	{
	int result = 0;

	/* Provide a hook */
	result = PyFile_SetOpenCodeHook(_open_code_hook, &result);
	if (result) {
	printf("Failed to set hook\n");
	return 1;
	}
	/* A second hook should fail */
	result = PyFile_SetOpenCodeHook(_open_code_hook, &result);
	if (!result) {
	printf("Should have failed to set second hook\n");
	return 2;
	}

	Py_IgnoreEnvironmentFlag = 0;
	_testembed_Py_InitializeFromConfig();
	result = 0;

	PyObject *r = PyFile_OpenCode("$$test-filename");
	if (!r) {
	PyErr_Print();
	result = 3;
	} else {
	void *cmp = PyLong_AsVoidPtr(r);
	Py_DECREF(r);
	if (cmp != &result) {
	printf("Did not get expected result from hook\n");
	result = 4;
	}
	}

	if (!result) {
	PyObject *io = PyImport_ImportModule("_io");
	PyObject *r = io
	? PyObject_CallMethod(io, "open_code", "s", "$$test-filename")
	: NULL;
	if (!r) {
	PyErr_Print();
	result = 5;
	} else {
	void *cmp = PyLong_AsVoidPtr(r);
	Py_DECREF(r);
	if (cmp != &result) {
	printf("Did not get expected result from hook\n");
	result = 6;
	}
	}
	Py_XDECREF(io);
	}

	Py_Finalize();
	return result;
	}

		self.assertRaises(AttributeError, self.write, NULL, object(), 0)
		self.assertRaises(TypeError, self.write, NULL, NULL, 0)

		with open(os_helper.TESTFN, "w", encoding="utf-8") as f:
		f.writelines([first_line, second_line])

gh-111495: Add PyFile_* CAPI tests #111709

Are you sure you want to change the base?

gh-111495: Add PyFile_* CAPI tests #111709

Conversation

sobolevn commented Nov 3, 2023 • edited by bedevere-app bot

sobolevn commented Nov 3, 2023 • edited

serhiy-storchaka left a comment

Choose a reason for hiding this comment

serhiy-storchaka commented Nov 4, 2023

sobolevn commented Nov 5, 2023

sobolevn commented Nov 5, 2023

vstinner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vstinner commented Nov 10, 2023

vstinner commented Nov 10, 2023

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

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

gh-111495: Add `PyFile_*` CAPI tests #111709

gh-111495: Add `PyFile_*` CAPI tests #111709

sobolevn commented Nov 3, 2023 •

edited by bedevere-app bot

sobolevn commented Nov 3, 2023 •

edited