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

staticinttest_open_code_hook(void) 
{
intresult=0; 
/* Provide a hook */
result=PyFile_SetOpenCodeHook(_open_code_hook, &result); 
if (result){
printf("Failed to set hook\n"); 
return1; 
 } 
/* A second hook should fail */
result=PyFile_SetOpenCodeHook(_open_code_hook, &result); 
if (!result){
printf("Should have failed to set second hook\n"); 
return2; 
 } 
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(); 
returnresult; 
 } 

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

+deftest_name_invalid_utf(self):
+withopen(os_helper.TESTFN, "w", encoding="utf-8") asf:
+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"
+withopen(os_helper.TESTFN, "w", encoding="utf-8") asf:
+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

+withopen(os_helper.TESTFN, "w", encoding="utf-8") asf:
+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:
deftest_file_get_multiple_lines(self): first_line="text with юникод 统一码\n"second_line="second line\n"withopen(os_helper.TESTFN, "w", encoding="utf-8") asf: f.writelines([first_line, second_line]) withopen(os_helper.TESTFN, encoding="utf-8") asf: self.assertEqual(self.get_line(f, 0), first_line) self.assertEqual(self.get_line(f, 0), second_line) deftest_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)){returnNULL}

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

staticPyObject*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

+raiseValueError("str raised")
+
+withself.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: AddPyFile_*CAPI tests
77afe78

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

bedevere-appbot added the awaiting review label Nov 3, 2023

bedevere-appbot 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 resolvedHide resolved

Address review
52c5918

vstinner approved these changes Nov 10, 2023
View reviewed changes

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

bedevere-appbot 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

	staticinttest_open_code_hook(void)
	{
	intresult=0;

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

	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();
	returnresult;
	}

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

Uh oh!

gh-111495: Add PyFile_* CAPI tests#111709

gh-111495: Add PyFile_* CAPI tests #111709

Uh oh!

Conversation

sobolevn commented Nov 3, 2023• edited by bedevere-app botLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn commented Nov 3, 2023• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka commented Nov 4, 2023

Uh oh!

Uh oh!

sobolevn commented Nov 5, 2023

Uh oh!

sobolevn commented Nov 5, 2023

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Nov 10, 2023

Uh oh!

vstinner commented Nov 10, 2023

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchakaNov 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchakaNov 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

serhiy-storchakaNov 10, 2023

Choose a reason for hiding this comment

Uh oh!

sobolevnSep 7, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner commented Aug 26, 2024

Uh oh!

sobolevn commented Aug 26, 2024

Uh oh!

sobolevn commented Sep 7, 2024

Uh oh!

serhiy-storchaka commented Sep 7, 2024

Uh oh!

serhiy-storchakaSep 7, 2024

Choose a reason for hiding this comment

Uh oh!

serhiy-storchakaSep 7, 2024

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jan 30, 2025• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sobolevn 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

sobolevnSep 7, 2024•
edited
Loading

vstinner commented Jan 30, 2025•
edited
Loading