Skip to content

Conversation

cmaloney
Copy link
Contributor

@cmaloney cmaloney commented Sep 3, 2025

Moves the I/O test support code to test.test_io.utils leaving all test cases as they are. The code was moved via copy/paste then adjusted imports as needed (remove unneded + add all required).


I plan to do a code update + two more direct code movement to split tests from test_general to increase parallelism and logical layout. Times are runtime on my 64 bit linux debug build output by slow tests when running

./python -m test test_io -uall,walltime,largefile,extralargefile -M16G -o -j0
  1. Move buffered tests into test_bufferedio
  2. Remove custom load_tests w/ test list from test_general gh-138013: Remove load_tests in test_io.test_general #138771 (see: gh-127647: Fix and enable I/O protocol tests #138369)
  3. Move SignalsTest to test_signals (21.4s)
  4. Move TextIOWrapperTest and IncrementalDecoder tests to test_textio (1.1s)

The longest remaining test in test_general after that for me is test_daemon_threads_shutdown_*_deadlock which takes ~7s (and is marked with requires_resource('walltime')). Runtime of test_io reduces from ~34.1s -> 21.4s (SignalsTest is now longest) via increased parallelism.

This moves test support code to `test_io._support` and buffered test
cases to `test_io.test_bufferedio` via copy/paste code movement then
adjusts imports as needed (remove unneded + add all required).
@cmaloney
Copy link
Contributor Author

cmaloney commented Sep 3, 2025

Windows bot failure looks real and a weird intermittent issue I've been running into locally some. Investigating:

test_uninitialized (test.test_io.test_bufferedio.CBufferedRWPairTest.test_uninitialized) ... Warning -- Unraisable exception
Exception ignored while finalizing file <_io.BufferedRWPair object at 0x000001AC6C4654F0>:
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\re\_parser.py", line 176, in append
    def append(self, code):
    
ValueError: flush of closed file
Warning -- Unraisable exception
Exception ignored while finalizing file <_io.BufferedWriter>:
Traceback (most recent call last):
  File "D:\a\cpython\cpython\Lib\re\_parser.py", line 176, in append
    def append(self, code):
    
ValueError: flush of closed file

@cmaloney cmaloney marked this pull request as ready for review September 9, 2025 19:46
@cmaloney
Copy link
Contributor Author

cmaloney commented Sep 9, 2025

I ran all the test_io tests in a loop for two days without reproducing... Reading through the BufferedRWPair code I think there are some tear-down race conditions but this PR doesn't change them, just effects test timing (which may make them a little more likely). That the stack trace points to a distinct module is really weird for me / I don't understand (likely a distinct bug). I plan to make a PR for some of the things I found in manual review in BufferedRWPair, but it is cases I've been able to figure out a good test case for; just hunches.

tl; dr: I think this PR splitting the tests is sound. It might make an existing flaky / race case more visible

@cmaloney
Copy link
Contributor Author

cmaloney commented Sep 9, 2025

Ubuntu Github Action got a better backtrace:

Exception ignored while finalizing file <_io.BufferedWriter>:
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_io/_support.py", line 41, in writable
    def writable(self):
    
ValueError: flush of closed file
Warning -- Unraisable exception
Exception ignored while finalizing file <_io.BufferedRWPair object at 0x200017dfb50>:
Traceback (most recent call last):
  File "/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_io/_support.py", line 41, in writable
    def writable(self):
    
ValueError: flush of closed file

@cmaloney
Copy link
Contributor Author

cmaloney commented Sep 9, 2025

Found a reproducer and filed an issue for the BufferedRWPair GC: gh-138720

@@ -0,0 +1,317 @@
import array
Copy link
Member

@vstinner vstinner Sep 10, 2025

Choose a reason for hiding this comment

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

What do you think of test_io.utils name instead? test_asyncio, test_ast and test_interpreters packages use this name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Works for me; This PR needs GH-138724 (or another fix for gh-138720) as that modifies a buffered test which is moved int his PR.

I see sort of three options

  1. close this PR and make the "split" of utils a PR of its own (no test moving) with these two requested changes; then make the test moving a distinct PR
  2. close this PR and re-make it with the new name (utils) doing the pure copy-paste after a fix for GC of unclosed io.BuffererdRWPair after .readinto results in unraisable exception #138720 is landed
  3. hand-tweak inside of the copy part of this PR; I had been avoiding that to try and keep this pure-"copy paste" + "add imports"

Happy to do any of the above, my leaning is 2, but 1 would make it so more of the work of splitting test_general can be landed sooner potentially

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thought of a 4:

  1. Remove the test_buffered moving from this, make it just the moving to test_io.utils

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Decided to implement 4

The buffered tests are being modified, get into a parallel lane
@cmaloney cmaloney changed the title gh-138013: Move buffered tests to test_bufferedio gh-138013: Move I/O test infrastructre to test_io.util Sep 10, 2025
@cmaloney cmaloney changed the title gh-138013: Move I/O test infrastructre to test_io.util gh-138013: Move I/O test infrastructre to test_io.utils Sep 10, 2025
@vstinner vstinner merged commit 1011724 into python:main Sep 11, 2025
47 checks passed
@vstinner
Copy link
Member

Merged, thanks.

@cmaloney cmaloney deleted the split_buffered branch September 15, 2025 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants