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.

Lib/test/test_capi/test_file.py

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().

Lib/test/test_capi/test_file.py

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.

Lib/test/test_capi/test_file.py

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')

I have explicit tests for both file object and io.StringIO:

def test_file_get_multiple_lines(self): first_line = "text with юникод 统一码\n" second_line = "second line\n" with open(os_helper.TESTFN, "w", encoding="utf-8") as f: f.writelines([first_line, second_line]) with open(os_helper.TESTFN, encoding="utf-8") as f: self.assertEqual(self.get_line(f, 0), first_line) self.assertEqual(self.get_line(f, 0), second_line) def test_file_get_line_from_file_like(self): first_line = "text with юникод 统一码\n" second_line = "second line\n" contents = io.StringIO(f"{first_line}{second_line}") self.assertEqual(self.get_line(contents, 0), first_line) self.assertEqual(self.get_line(contents, 0), second_line)

vstinner · 2024-08-26T13:56:48Z

@sobolevn: What's the status of this PR? Do you plan to attempt to address @serhiy-storchaka's latest review?

sobolevn · 2024-08-26T14:26:28Z

yes, sure! adding this to my queue.

sobolevn · 2024-09-07T07:16:15Z

@serhiy-storchaka @vstinner I partially addressed your review. The only part that I didn't implement is invalid utf8 tests. I want to ask for advice on how it should be done.

For example, right now I cannot pass bytes to _testcapi.file_from_fd in test_string_args_as_invalid_utf. Because it parses args as:

if (!PyArg_ParseTuple(args, "izzizzzi",
                          &fd,
                          &name, &mode,
                          &buffering,
                          &encoding, &errors, &newline,
                          &closefd)) {
        return NULL;
    }

What is the best way to pass bytes here? Create one more function like:

static PyObject *
file_from_fd_with_bytes(PyObject *Py_UNUSED(self), PyObject *args)

and allow passing bytes there?

serhiy-storchaka · 2024-09-07T15:42:30Z

What are your issues with passing a bytes object?

serhiy-storchaka · 2024-09-07T16:04:59Z

Lib/test/test_capi/test_file.py

+                raise ValueError("str raised")
+
+        with self.assertRaisesRegex(ValueError, "str raised"):
+            self.write_and_return(StrRaises(), flags=_testcapi.Py_PRINT_RAW)


It is not clear what is the difference between these tests if it raises in any case. You should either define __str__ and __repr__ that do not raise in corresponding classes and test both classes with and without Py_PRINT_RAW, or just make both __str__ and __repr__ in the same class raising different exceptions and test that writing with and without Py_PRINT_RAW gives different errors. The former option will duplicate other tests, so I suggest the later way.

Oh, and you do not need to use write_and_return here.

serhiy-storchaka · 2024-09-07T16:09:54Z

Lib/test/test_capi/test_file.py

-        self.assertRaises(AttributeError, self.write, NULL, object(), 0)
-        self.assertRaises(TypeError, self.write, NULL, NULL, 0)
+        wr = self.write
+        self.assertRaises(TypeError, wr, object(), io.BytesIO(), 0)


Use a string instead of object(). It will be clearer what you write and why this fails.

vstinner · 2025-01-30T17:17:10Z

Oh no, I did it again :-( I forgot about this PR and I wrote a new one (that I just merged): #129449. Sorry about that. It seems like this PR has more tests.

sobolevn · 2025-01-30T17:50:03Z

@vstinner thanks a lot for your PR, I forgot about that one several times already :)

You can port some of the tests from here to your version if it helps.
Thanks for the reviews, @serhiy-storchaka! 👍

vstinner · 2025-01-30T19:27:13Z

I will try to add tests from this PR.

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

Closed

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

Closed

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 Outdated Show resolved Hide resolved

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

serhiy-storchaka reviewed Nov 10, 2023

View reviewed changes

sobolevn added 2 commits September 7, 2024 09:00

Merge branch 'main' into issue-111495

8022299

Partially address review, stil need to change invalid utf8 tests

3781efb

serhiy-storchaka reviewed Sep 7, 2024

View reviewed changes

skirpichev mentioned this pull request Oct 3, 2024

gh-111495: Add tests for PyFile C API #124915

Closed

sobolevn closed this Jan 30, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sobolevn commented Nov 3, 2023 •

edited by bedevere-app bot

Loading

sobolevn commented Nov 3, 2023 •

edited

Loading

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 commented Nov 10, 2023

vstinner commented Nov 10, 2023

serhiy-storchaka left a comment

serhiy-storchaka Nov 5, 2023

serhiy-storchaka Nov 6, 2023

serhiy-storchaka Nov 10, 2023

sobolevn Sep 7, 2024 •

edited

Loading

vstinner commented Aug 26, 2024

sobolevn commented Aug 26, 2024

sobolevn commented Sep 7, 2024

serhiy-storchaka commented Sep 7, 2024

serhiy-storchaka Sep 7, 2024

serhiy-storchaka Sep 7, 2024

vstinner commented Jan 30, 2025 •

edited

Loading

sobolevn commented Jan 30, 2025

vstinner commented Jan 30, 2025

	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;
	}

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

gh-111495: Add PyFile_* CAPI tests #111709

gh-111495: Add PyFile_* CAPI tests #111709

Conversation

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

sobolevn commented Nov 3, 2023 • edited Loading

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

vstinner commented Nov 10, 2023

vstinner commented Nov 10, 2023

serhiy-storchaka left a comment

Choose a reason for hiding this comment

serhiy-storchaka Nov 5, 2023

Choose a reason for hiding this comment

serhiy-storchaka Nov 6, 2023

Choose a reason for hiding this comment

serhiy-storchaka Nov 10, 2023

Choose a reason for hiding this comment

sobolevn Sep 7, 2024 • edited Loading

Choose a reason for hiding this comment

vstinner commented Aug 26, 2024

sobolevn commented Aug 26, 2024

sobolevn commented Sep 7, 2024

serhiy-storchaka commented Sep 7, 2024

serhiy-storchaka Sep 7, 2024

Choose a reason for hiding this comment

serhiy-storchaka Sep 7, 2024

Choose a reason for hiding this comment

vstinner commented Jan 30, 2025 • edited Loading

sobolevn commented Jan 30, 2025

vstinner commented Jan 30, 2025

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

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

sobolevn commented Nov 3, 2023 •

edited by bedevere-app bot

Loading

sobolevn commented Nov 3, 2023 •

edited

Loading

sobolevn Sep 7, 2024 •

edited

Loading

vstinner commented Jan 30, 2025 •

edited

Loading