GH-120754: Add a strace helper and test set of syscalls for open().read()
General goal here is to be able to add tests that behavior isn't unintentionally changed around some common python code patterns, in particular Path("README.rst").read_text().
This started with the code that validated subprocess used vfork, building more general utilities from that core code. The work was motivated in https://github.com/python/cpython/pull/120755 for how to test that particular change.
TODO
- [x] Resolve how to fix subprocess._USE_VFORK. this PR currently just comments out that test. See: https://discuss.python.org/t/subprocess-use-vfork-escape-hatch-broken-fix-or-remove/56915/2
- [x] ~~Figure out why the
print()of markers needs flush=True to not buffer them. Likely add a test for two print statements not being merged using this infrastructure.~~ -- This is the documented behavior of stdout https://docs.python.org/3/library/sys.html#sys.stdout. Because they're files / not interactive it gets block buffered meaning only once a buffer is full does it flush rather than write() when a print() happens. - [x] ~~Validate at least once CI bot actually runs and passes the test~~ -- Ubuntu / Build and test found that glibc there uses newfstat which caused the test to fail. Updated the test to allow libc to use other fstat variants.
- [x] Where should test for Path().read_bytes() and .read_text() live?
- [x] Update syscall sequence after https://github.com/python/cpython/pull/120755 is merged -- 06066779ec3a938dad8e7cc9feeadb65fd4d1f69
- Issue: gh-120754
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.
If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.
If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.
If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.
If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.
If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.
If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.
If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.
Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.
I have made the requested changes; please review again
also: Could this get the "skip news" label? From my perspective not a python user visible behavior change
Thanks for making the requested changes!
@gpshead: please review the changes made to this pull request.
Yea, I think I've addressed all of @gpshead's review comments, I haven't clicked 'resolved conversation' though to keep the review visible for context
Thank you again! (gpshead, if you come back to this and want some change, just let me know)
:warning::warning::warning: Buildbot failure :warning::warning::warning:
Hi! The buildbot ARM Raspbian 3.x has failed when building commit e38d0afe3548b856ccf0b05c01ed3eefc69cb3e7.
What do you need to do:
- Don't panic.
- Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
- Go to the page of the buildbot that failed (https://buildbot.python.org/#/builders/424/builds/7966) and take a look at the build logs.
- Check if the failure is related to this commit (e38d0afe3548b856ccf0b05c01ed3eefc69cb3e7) or if it is a false positive.
- If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.
You can take a look at the buildbot page here:
https://buildbot.python.org/#/builders/424/builds/7966
Failed tests:
- test_fileio
Summary of the results of the build (if available):
==
Click to see traceback logs
Traceback (most recent call last):
File "/var/lib/buildbot/workers/3.x.gps-raspbian.nondebug/build/Lib/test/test_fileio.py", line 396, in check_readall
self.assertEqual(count_similarname('fstat'), 1)
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 0 != 1
Hmm, I'd need a few more logs to figure this out. Let's revert for now https://github.com/python/cpython/pull/123303 and fix in another PR? My bad, I should have thought to run the buildbots on this :-)
Sounds good to me. Think i have all the bits to try that raspbian locally, need to figure out what its syscall name for fstat looks like
Let me go strace on my Raspbian bot to see what it says. This type of test is also okay to just restrict to a known set of platforms if it comes down to that.
Okay, I changed the new assertEqual(count_similarname("fstat"), 1) lines assertEqual(count_similar("fstat"), 1, msg=f"set(syscalls)=") to get useful info in the error message. Here's a couple examples:
======================================================================
FAIL: test_syscalls_read (test.test_fileio.PyAutoFileTests.test_syscalls_read) (name='pathlib read_bytes')
Check that the set of system calls produced by the I/O stack is what
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/pi/repro-pr-120754/cpython-e38d0afe3548b856ccf0b05c01ed3eefc69cb3e7/Lib/test/test_fileio.py", line 396, in check_readall
self.assertEqual(count_similarname('fstat'), 1, msg=f"{set(syscalls)=}")
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 0 != 1 : set(syscalls)={'openat', 'read', 'close', 'statx'}
======================================================================
FAIL: test_syscalls_read (test.test_fileio.PyAutoFileTests.test_syscalls_read) (name='pathlib read_text')
Check that the set of system calls produced by the I/O stack is what
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/pi/repro-pr-120754/cpython-e38d0afe3548b856ccf0b05c01ed3eefc69cb3e7/Lib/test/test_fileio.py", line 396, in check_readall
self.assertEqual(count_similarname('fstat'), 1, msg=f"{set(syscalls)=}")
~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
AssertionError: 0 != 1 : set(syscalls)={'read', 'ioctl', 'openat', 'close', '_llseek', 'statx'}
----------------------------------------------------------------------
statx is indeed a valid Linux system call that could be used for the purpose.