GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-138013: Move I/O test infrastructre to test_io.utils by cmaloney · Pull Request #138475 · python/cpython

Conversation

@cmaloney

@cmaloney cmaloney commented Sep 3, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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.

  • Issue: Move Buffered tests from test_io to test_bufio #138013

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@cmaloney
pythongh-138013: Move buffered tests to test_bufferedio
c28ff5f 
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).
@bedevere-app bedevere-app Bot added the awaiting review label Sep 3, 2025
@bedevere-app bedevere-app Bot mentioned this pull request Sep 3, 2025
Move Buffered tests from test_io to test_bufio #138013
Closed
@cmaloney
cmaloney marked this pull request as draft September 3, 2025 20:19
@bedevere-app bedevere-app Bot removed the awaiting review label Sep 3, 2025
@cmaloney

cmaloney commented Sep 3, 2025

Copy link
Copy Markdown
Contributor Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@AA-Turner AA-Turner added the skip news label Sep 3, 2025
@cmaloney
Merge branch 'main' into split_buffered
2ba448a
@cmaloney
cmaloney marked this pull request as ready for review September 9, 2025 19:46
@bedevere-app bedevere-app Bot added the awaiting review label Sep 9, 2025
@cmaloney

cmaloney commented Sep 9, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cmaloney

cmaloney commented Sep 9, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@cmaloney cmaloney mentioned this pull request Sep 9, 2025
GC of unclosed io.BuffererdRWPair after .readinto results in unraisable exception #138720
Closed
@cmaloney

cmaloney commented Sep 9, 2025

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Sep 10, 2025
View reviewed changes
Comment thread Lib/test/test_io/_support.py
@@ -0,0 +1,317 @@
import array

@vstinner vstinner Sep 10, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cmaloney cmaloney Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cmaloney cmaloney Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@cmaloney cmaloney Sep 10, 2025

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Decided to implement 4

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_io/_support.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@cmaloney
Change to just split out util
08727a3 
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
util -> utils
6fb6fcd
@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 approved these changes Sep 11, 2025
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting review labels Sep 11, 2025
@vstinner
vstinner merged commit 1011724 into python:main Sep 11, 2025
@bedevere-app bedevere-app Bot removed the awaiting merge label Sep 11, 2025
@vstinner

vstinner commented Sep 11, 2025

Copy link
Copy Markdown
Member

Merged, thanks.
cmaloney reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

Reviewers

@vstinner vstinner vstinner approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@cmaloney @vstinner @AA-Turner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.