Skip to content

Conversation

@cmaloney
Copy link
Contributor

@cmaloneycmaloney 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_generalgh-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
ContributorAuthor

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--UnraisableexceptionExceptionignoredwhilefinalizingfile<_io.BufferedRWPairobjectat0x000001AC6C4654F0>: Traceback (mostrecentcalllast): File"D:\a\cpython\cpython\Lib\re\_parser.py", line176, inappenddefappend(self, code): ValueError: flushofclosedfileWarning--UnraisableexceptionExceptionignoredwhilefinalizingfile<_io.BufferedWriter>: Traceback (mostrecentcalllast): File"D:\a\cpython\cpython\Lib\re\_parser.py", line176, inappenddefappend(self, code): ValueError: flushofclosedfile

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

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
ContributorAuthor

cmaloney commented Sep 9, 2025

Ubuntu Github Action got a better backtrace:

Exceptionignoredwhilefinalizingfile<_io.BufferedWriter>: Traceback (mostrecentcalllast): File"/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_io/_support.py", line41, inwritabledefwritable(self): ValueError: flushofclosedfileWarning--UnraisableexceptionExceptionignoredwhilefinalizingfile<_io.BufferedRWPairobjectat0x200017dfb50>: Traceback (mostrecentcalllast): File"/home/runner/work/cpython/cpython-ro-srcdir/Lib/test/test_io/_support.py", line41, inwritabledefwritable(self): ValueError: flushofclosedfile

@cmaloney
Copy link
ContributorAuthor

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

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

@vstinnervstinnerSep 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
ContributorAuthor

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
ContributorAuthor

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
ContributorAuthor

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
@cmaloneycmaloney changed the title gh-138013: Move buffered tests to test_bufferediogh-138013: Move I/O test infrastructre to test_io.utilSep 10, 2025
@cmaloneycmaloney changed the title gh-138013: Move I/O test infrastructre to test_io.utilgh-138013: Move I/O test infrastructre to test_io.utilsSep 10, 2025
@vstinnervstinner merged commit 1011724 into python:mainSep 11, 2025
47 checks passed
@vstinner
Copy link
Member

Merged, thanks.

@cmaloneycmaloney deleted the split_buffered branch September 15, 2025 21:55
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@cmaloney@vstinner@AA-Turner