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 — [3.11] gh-87846: test_io: Ignore OpenWrapper in test___all__ by encukou · Pull Request #126478 · python/cpython

Conversation

@encukou

@encukou encukou commented Nov 6, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

@pablogsal, are you interested in fixing 3.11 buildbots that consistently warn, rather than fail?

test_io fails if test_openwrapper runs before test___all__. This consistently happens on some refleaks buildbots, for example Windows or Fedora, but it's only marked as a warning: when only test___all__ is re-run in a fresh process, it succeeds.

Locally, this can be reproduced by running test_openwrapper and test__all__, twice (since in a single run, test_openwrapper comes after test___all__). I don't know of a more elegant way than:

./python -m test test_io test_io -m '*test_[o_][p_][ea][nl][wl]*' -v

The reason is that the deprecated OpenWrapper is added to the module on demand in __getattr__, after which it shows up in dir(io).

Add it to not_exported so that check__all__ ignores it when it exists.

  • Issue: Deprecate undocumented io.OpenWrapper and _pyio.OpenWrapper #87846

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@encukou
[3.11] pythongh-87846: test_io: Ignore OpenWrapper in test___all__
79c838c 
The deprecated `OpenWrapper` is added to the module on demand
in `__getattr__`, so it might or might not show up in `dir(io)`
depending on whether e.g. `test_openwrapper` was run.

Add it to `not_exported` so that check__all__ ignores it
when it exists.

The test consistently failed on some refleaks buildbots (but
not when the test is re-run, so it was only marked as a warning).
Locally, it can be reproduced by running `test_openwrapper` and
`test__all__`, twice:

    ./python -m test test_io test_io -m '*test_[o_][p_][ea][nl][wl]*' -v
@encukou
encukou requested a review from pablogsal November 6, 2024 10:01
@bedevere-app bedevere-app Bot added tests Tests in the Lib/test dir awaiting core review labels Nov 6, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Nov 6, 2024
Deprecate undocumented io.OpenWrapper and _pyio.OpenWrapper #87846
Closed
@encukou encukou added the skip news label Nov 6, 2024
vstinner
vstinner approved these changes Nov 6, 2024
View reviewed changes

@vstinner vstinner left a comment

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.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Nov 6, 2024
pablogsal
pablogsal approved these changes Nov 6, 2024
View reviewed changes
@pablogsal

pablogsal commented Nov 6, 2024

Copy link
Copy Markdown
Member

I'm fine to backport since this is a change in tests
encukou reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@pablogsal
pablogsal merged commit 8c6885d into python:3.11 Nov 6, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Nov 6, 2024
@encukou
encukou deleted the openwrapper-test-3.11 branch November 6, 2024 14:50
@encukou

encukou commented Nov 7, 2024 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

After this was merged, all the 3.11 refleaks buildbots turned red. But, that's an improvement!

There's a pre-existing refleak that was hidden like this:

  • the first test run failed, so the leak check was skipped
  • the second run only ran the failing test_all, which does not leak

I'll find and write/backport a fix. (update: see #111942)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@encukou encukou mentioned this pull request Nov 7, 2024
TextIOWrapper.reconfigure() crashes if encoding is not string or None #111942
Closed
encukou added a commit to encukou/cpython that referenced this pull request Nov 7, 2024
@encukou
[3.11] pythongh-111942: Remove an extra incref in textiowrapper_chang…
8e08bf8 
…e_encoding

There's an issue with the 3.11 backport commit e2421a3
The chane for the main branch was:

```diff
+    Py_INCREF(errors);
 ...
     Py_SETREF(self->encoding, encoding);
-    Py_SETREF(self->errors, Py_NewRef(errors));
+    Py_SETREF(self->errors, errors);
```

but in 3.11 this became:

```diff
+    Py_INCREF(errors);
...
     Py_INCREF(errors);
     Py_SETREF(self->encoding, encoding);
     Py_SETREF(self->errors, errors);
```
i.e. there's an extra incref, but it looks -- at least to Git -- like the change
that removes `Py_NewRef` was already applied.

This was not caught because `test_io` refleaks tests were ineffective on 3.11
(see python#126478 (comment)).

Remove the extraneous incref.
@encukou encukou mentioned this pull request Nov 7, 2024
[3.11] gh-111942: Remove an extra incref in textiowrapper_change_encoding #126542
Merged
@encukou

encukou commented Nov 11, 2024

Copy link
Copy Markdown
Member Author

FWTW, 3.10 is unaffected: it doesn't have #111370.
All reactions

Sorry, something went wrong.

Uh oh!

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

Uh oh!

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

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

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

@encukou @pablogsal @vstinner
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.