gh-111495: Add `PyFile_*` CAPI tests by sobolevn · Pull Request #111709 · python/cpython · GitHub

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

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?

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

Tests fail on Windows

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

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

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?

LGTM.

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.

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

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.

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

Again, it does not create invalid UTF-8.

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)

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

yes, sure! adding this to my queue.

@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?

What are your issues with passing a bytes object?

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.

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

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.

@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! 👍

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

Comment thread Lib/test/test_capi/test_file.py Outdated

Address review

52c5918

vstinner approved these changes Nov 10, 2023

View reviewed changes

Comment thread Lib/test/test_capi/test_file.py Outdated

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

+                  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",

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

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

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

gh-111495: Add `PyFile_*` CAPI tests by sobolevn · Pull Request #111709 · python/cpython · GitHub

Uh oh!

Uh oh!

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

gh-111495: Add PyFile_* CAPI tests#111709sobolevn wants to merge 5 commits intopython:mainpython/cpython:mainfrom sobolevn:issue-111495sobolevn/cpython:issue-111495Copy head branch name to clipboard

Conversation

sobolevn commented Nov 3, 2023 • edited by bedevere-app Bot Loading 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-storchaka Nov 5, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka Nov 10, 2023

Choose a reason for hiding this comment

Uh oh!

sobolevn Sep 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-storchaka Sep 7, 2024

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Sep 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!

gh-111495: Add PyFile_* CAPI tests#111709
sobolevn wants to merge 5 commits into
python:mainfrom
sobolevn:issue-111495

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