Downgraded skips in tests #6716

terryluan12 · 2026-01-12T21:12:02Z

Summary by CodeRabbit

Chores
- CI updated to include an additional Windows test in the suite, improving test coverage and reliability for Windows builds.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2026-01-12T21:12:10Z

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (9)

Lib/test/test_ast/test_ast.py is excluded by !Lib/**
Lib/test/test_bdb.py is excluded by !Lib/**
Lib/test/test_class.py is excluded by !Lib/**
Lib/test/test_configparser.py is excluded by !Lib/**
Lib/test/test_descr.py is excluded by !Lib/**
Lib/test/test_docxmlrpc.py is excluded by !Lib/**
Lib/test/test_ftplib.py is excluded by !Lib/**
Lib/test/test_marshal.py is excluded by !Lib/**
Lib/test/test_socket.py is excluded by !Lib/**

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

A single test identifier was added to the Windows environment-polluting tests list in the CI workflow configuration, expanding the set of tests flagged as environment-polluting for Windows CI runs.

Changes

Cohort / File(s)	Summary
CI Workflow Configuration `/.github/workflows/ci.yaml`	Added one test identifier to the `ENV_POLLUTING_TESTS_WINDOWS` environment variable (single-line addition).

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Suggested reviewers

youknowone

Poem

🐰 A test hops into the Windows stream,

A tiny change, but part of the team,
CI hums on with careful cheer,
One more case safely kept near. 🪄🐇

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The title 'Downgraded skips in tests' is vague and generic, using non-specific language that doesn't clearly convey what changes were made or why.	Provide a more descriptive title that clearly explains the specific change, such as 'Add test.test_tempfile to Windows polluting tests list' or clarify what 'downgrading skips' means in this context.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

terryluan12 · 2026-01-12T21:18:21Z

Double check tests in test_class.py, test_configparser.py, test_context.py, test_str.py, test_str.py, test_subprocess.py

terryluan12 · 2026-01-13T00:31:29Z

Note to self: testing if, given the new timeout, whether timeouts count as failures, or errors (expectedFailure vs skips)

fanninpm

A few extra things besides these inline comments:

for Lib/test/test_zipfile/test_core.py, we might need to add support for that shift_jis encoding.
for Lib/test/bdb.py, @youknowone Is #6626 going to fix the tests that are decorated?

Lib/test/test_sqlite3/test_dbapi.py

fanninpm · 2026-01-13T00:50:53Z

Lib/test/test_sqlite3/test_dbapi.py

    def setUp(self):
        self.cx = sqlite.Connection.__new__(sqlite.Connection)

+    @unittest.skip('TODO: RUSTPYTHON')


Any particular reason for the skip?

I'm not sure. For this PR, I mainly focused on downgrading/specifying the @unittest.skip's, so they were all already notated, I'm just downgrading/removing/specifying them.

This one specifically was skipping already, except the whole class was skipped, so I just moved it to the specific failing test. It didn't have a reason attached, so the moved reason also didn't.

If an underlying reason can be given, it will certainly be helpful when someone tries to fix the problem.

@fanninpm i understand the logic, but review is about the changes, not about the code seen

fanninpm · 2026-01-13T01:05:15Z

Lib/test/test_descr.py

+    # TODO: RUSTPYTHON; The `expectedFailure` here is from CPython, so this test actually fails. Not run on linux?
+    # @unittest.expectedFailure


@youknowone what do you want to do about this? Related: python/cpython#49572

I found our __new__ behavior is different from Python 3.14. (not checked incompatibility of 3.13 or changed in 3.14)
Let's see that change affect this change

Lib/test/test_marshal.py

Lib/test/test_threading.py

…lures

ShaharNaveh

Overall it looks great!
Minor nitpicks about some added code that we can delete.

TYSM!!!

Lib/test/test_io.py

fanninpm · 2026-01-13T17:25:57Z

Lib/test/test_tempfile.py


    @os_helper.skip_unless_symlink
-    @unittest.skip('TODO: RUSTPYTHON; No such file or directory "..."')
+    @unittest.skip('TODO: RUSTPYTHON; alters the execution environment (env changed)')


For better tracking:

Suggested change

@unittest.skip('TODO: RUSTPYTHON; alters the execution environment (env changed)')

@unittest.skipIf(

'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ,

'TODO: RUSTPYTHON; alters the execution environment (env changed)'

)

...and when you do this, please add test_tempfile to the ENV_POLLUTING_TESTS_COMMON list in .github/workflows/ci.yaml.

You may end up needing to do something like this:

Suggested change

@unittest.skip('TODO: RUSTPYTHON; alters the execution environment (env changed)')

@unittest.skipIf(

sys.platform == 'linux' and 'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ,

'TODO: RUSTPYTHON; alters the execution environment (env changed)'

)

...or this:

Suggested change

@unittest.skip('TODO: RUSTPYTHON; alters the execution environment (env changed)')

@unittest.skipIf(

sys.platform in ('darwin', 'linux') and 'RUSTPYTHON_SKIP_ENV_POLLUTERS' in os.environ,

'TODO: RUSTPYTHON; alters the execution environment (env changed)'

)

...in which case(s), please make sure test_tempfile is added to the appropriate OS-specific variable(s) in .github/workflows/ci.yaml.

I'm still trying to nail down the test_tempfile.py error to double check, but fyi there may an issue with the ENV_POLLUTING_TESTS_COMMON list.

In the last test run, it seems that test.test_multiprocessing_forkserver.test_threads and test.test_multiprocessing_spawn.test_threads had a similar ENV CHANGED issue, despite the fact that no changes are made to the test_multiprocessing_spawn tests

… ENV_POLLUTING_TESTS_WINDOWS

youknowone

Thank you so much! You seem to find many outdated skips in our test code.

Please make sure unskipped tests are not actually flaky. Running multiple times of them will be helpful.
Sometimes they are flaky on specific platforms. I will run the entire CI multiple times once this is done.

Lib/test/test_ast/test_ast.py

Lib/test/test_zipfile/test_core.py

Lib/test/test_configparser.py

Lib/test/test_descr.py

Lib/test/test_ftplib.py

Lib/test/test_http_cookiejar.py

Lib/test/test_set.py

Lib/test/test_subprocess.py

Lib/test/test_json/test_recursion.py

terryluan12 · 2026-01-14T03:51:48Z

@youknowone It should be ok to rereview/run the CI/CD tests.

Like I said, there seem to be intermittent issues with test.test_multiprocessing_forkserver.test_threads + test.test_multiprocessing_spawn.test_threads but it seems to be unrelated.

terryluan12 force-pushed the downgrade_tests branch from 8e34530 to 5ac99cf Compare January 12, 2026 21:15

Downgraded skips in tests

770c867

terryluan12 force-pushed the downgrade_tests branch from 5ac99cf to 770c867 Compare January 12, 2026 21:17

terryluan12 marked this pull request as draft January 12, 2026 21:32

Fixed failing tests

7e58650

fanninpm reviewed Jan 13, 2026

View reviewed changes

terryluan12 added 3 commits January 12, 2026 20:32

Fixed test_ftplib + test_socket + test_ssl + test_threaded_import fai…

c9da5d2

…lures

Removed comments about which tests are run in which environment

3a9ed9d

Addressed PR comments

43de102

terryluan12 force-pushed the downgrade_tests branch 2 times, most recently from f9f4fb0 to 33cead3 Compare January 13, 2026 04:56

Annotated skips on failing tests

1c62ce3

terryluan12 force-pushed the downgrade_tests branch from 33cead3 to 1c62ce3 Compare January 13, 2026 05:44

ShaharNaveh suggested changes Jan 13, 2026

View reviewed changes

Lib/test/test_io.py Outdated Show resolved Hide resolved

Lib/test/test_io.py Outdated Show resolved Hide resolved

Lib/test/test_io.py Outdated Show resolved Hide resolved

Removed unneeded tests

cce3c36

terryluan12 marked this pull request as ready for review January 13, 2026 17:01

Removed unneeded sys import from test_ftplib

21bca37

fanninpm reviewed Jan 13, 2026

View reviewed changes

Added annotation to test_ftplib

ea760c8

terryluan12 force-pushed the downgrade_tests branch from fa5ef2e to ea760c8 Compare January 13, 2026 23:42

Readded skipIf to test_cleanup_with_symlink_modes with a more general…

1beac54

… ENV_POLLUTING_TESTS_WINDOWS

youknowone requested changes Jan 14, 2026

View reviewed changes

terryluan12 force-pushed the downgrade_tests branch from 5f70d13 to 8ee84ea Compare January 14, 2026 02:03

Addressed PR comments

ed6dc56

terryluan12 force-pushed the downgrade_tests branch from 8ee84ea to ed6dc56 Compare January 14, 2026 02:37

Made changes to minimize diff in PR

80a5f8b

		# TODO: RUSTPYTHON; The `expectedFailure` here is from CPython, so this test actually fails. Not run on linux?
		# @unittest.expectedFailure

Downgraded skips in tests #6716

Are you sure you want to change the base?

Downgraded skips in tests #6716

Uh oh!

Conversation

terryluan12 commented Jan 12, 2026 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Uh oh!

terryluan12 commented Jan 12, 2026

Uh oh!

terryluan12 commented Jan 13, 2026

Uh oh!

fanninpm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanninpm Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

terryluan12 Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

fanninpm Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

youknowone Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

fanninpm Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

youknowone Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ShaharNaveh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fanninpm Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terryluan12 Jan 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

terryluan12 commented Jan 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

terryluan12 commented Jan 12, 2026 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Jan 12, 2026 •

edited

Loading

youknowone Jan 14, 2026 •

edited

Loading

fanninpm Jan 13, 2026 •

edited

Loading

terryluan12 Jan 14, 2026 •

edited

Loading