GeezerVison

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

GeezerVison is showing github.com — Downgraded skips in tests by terryluan12 · Pull Request #6716 · RustPython/RustPython

Conversation

terryluan12 commented Jan 12, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

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.}

Sorry, something went wrong.

Uh oh!

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

All reactions

coderabbitai Bot commented Jan 12, 2026 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (27)

Lib/test/test_ast/test_ast.py is excluded by !Lib/**
Lib/test/test_bdb.py is excluded by !Lib/**
Lib/test/test_bytes.py is excluded by !Lib/**
Lib/test/test_class.py is excluded by !Lib/**
Lib/test/test_compile.py is excluded by !Lib/**
Lib/test/test_configparser.py is excluded by !Lib/**
Lib/test/test_context.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_enum.py is excluded by !Lib/**
Lib/test/test_future_stmt/test_future.py is excluded by !Lib/**
Lib/test/test_http_cookiejar.py is excluded by !Lib/**
Lib/test/test_importlib/test_threaded_import.py is excluded by !Lib/**
Lib/test/test_io.py is excluded by !Lib/**
Lib/test/test_itertools.py is excluded by !Lib/**
Lib/test/test_logging.py is excluded by !Lib/**
Lib/test/test_marshal.py is excluded by !Lib/**
Lib/test/test_signal.py is excluded by !Lib/**
Lib/test/test_socket.py is excluded by !Lib/**
Lib/test/test_sort.py is excluded by !Lib/**
Lib/test/test_sqlite3/test_dbapi.py is excluded by !Lib/**
Lib/test/test_str.py is excluded by !Lib/**
Lib/test/test_tarfile.py is excluded by !Lib/**
Lib/test/test_thread.py is excluded by !Lib/**
Lib/test/test_threading.py is excluded by !Lib/**
Lib/test/test_time.py is excluded by !Lib/**
Lib/test/test_timeout.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.}

All reactions

Sorry, something went wrong.

Uh oh!

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

terryluan12 force-pushed the downgrade_tests branch 2 times, most recently from 5ac99cf to 770c867 Compare

January 12, 2026 21:17

terryluan12 commented Jan 12, 2026

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

terryluan12 marked this pull request as draft

January 12, 2026 21:32

terryluan12 commented Jan 13, 2026

Copy link

Copy Markdown

Contributor Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

fanninpm reviewed

Jan 13, 2026

View reviewed changes

fanninpm left a comment

Copy link

Copy Markdown

Contributor

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.

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?

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_sqlite3/test_dbapi.py Resolved

Uh oh!

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

Comment thread

Lib/test/test_sqlite3/test_dbapi.py

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

		@unittest.skip('TODO: RUSTPYTHON')

fanninpm Jan 13, 2026

Copy link

Copy Markdown

Contributor

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.

Any particular reason for the skip?

Sorry, something went wrong.

Uh oh!

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

All reactions

terryluan12 Jan 13, 2026

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.

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.

Sorry, something went wrong.

Uh oh!

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

youknowone reacted with thumbs up emoji

All reactions

👍 1 reaction

fanninpm Jan 13, 2026

Copy link

Copy Markdown

Contributor

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

youknowone Jan 13, 2026

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

Comment thread

Lib/test/test_descr.py Outdated

Comment on lines +1837 to +1838

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

fanninpm Jan 13, 2026

Copy link

Copy Markdown

Contributor

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

youknowone Jan 14, 2026 •

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.

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

Sorry, something went wrong.

Uh oh!

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

terryluan12 reacted with thumbs up emoji

All reactions

👍 1 reaction

Comment thread

Lib/test/test_marshal.py Resolved

Uh oh!

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

Comment thread

Lib/test/test_threading.py Resolved

Uh oh!

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

terryluan12 force-pushed the downgrade_tests branch 3 times, most recently from 33cead3 to 1c62ce3 Compare

January 13, 2026 05:44

ShaharNaveh requested changes

Jan 13, 2026

View reviewed changes

ShaharNaveh left a comment

Copy link

Copy Markdown

Contributor

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.

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

TYSM!!!

Sorry, something went wrong.

Uh oh!

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

terryluan12 reacted with heart emoji

All reactions

❤️ 1 reaction

Comment thread

Lib/test/test_io.py Resolved

Uh oh!

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

Comment thread

Lib/test/test_io.py Resolved

Uh oh!

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

Comment thread

Lib/test/test_io.py Resolved

Uh oh!

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

terryluan12 marked this pull request as ready for review

January 13, 2026 17:01

fanninpm reviewed

Jan 13, 2026

View reviewed changes

Comment thread

Lib/test/test_tempfile.py Outdated


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

fanninpm Jan 13, 2026 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Contributor

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.

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.

Sorry, something went wrong.

Uh oh!

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

All reactions

terryluan12 Jan 14, 2026 •

edited

Loading

Uh oh!

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

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.

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

Sorry, something went wrong.

Uh oh!

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

All reactions

terryluan12 force-pushed the downgrade_tests branch from fa5ef2e to ea760c8 Compare

January 13, 2026 23:42

youknowone requested changes

Jan 14, 2026

View reviewed changes

youknowone 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.

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.

Sorry, something went wrong.

Uh oh!

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

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

youknowone force-pushed the downgrade_tests branch from 80a5f8b to 559d35d Compare

January 14, 2026 11:51

youknowone commented Jan 14, 2026

Copy link

Copy Markdown

Member

You’re right. those are making it hard to see the actual results of this test. I’ll fix that and run it again.

All reactions

Sorry, something went wrong.

Uh oh!

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

youknowone force-pushed the downgrade_tests branch from 559d35d to 8a5c49f Compare

January 14, 2026 23:38

terryluan12 mentioned this pull request

Jan 15, 2026

Update asyncio library #6601

Merged

terryluan12 added 8 commits

January 15, 2026 14:30


          Downgraded skips in tests

4e658db


          Fixed failing tests

59b0be2


          Fixed test_ftplib + test_socket + test_ssl + test_threaded_import fai…

6c3ee49

…lures


          Removed comments about which tests are run in which environment

55665b7


          Addressed PR comments

4c1dd38


          Annotated skips on failing tests

87fa39d


          Removed unneeded tests

e0fa426


          Removed unneeded sys import from test_ftplib

46677e7

terryluan12 added 4 commits

January 15, 2026 14:30


          Added annotation to test_ftplib

96daa90


          Readded skipIf to test_cleanup_with_symlink_modes with a more general…

4f86cbf

… ENV_POLLUTING_TESTS_WINDOWS


          Addressed PR comments

d5035f4


          Made changes to minimize diff in PR

6a3932d

youknowone force-pushed the downgrade_tests branch from 8a5c49f to 6a3932d Compare

January 15, 2026 05:30

youknowone commented Jan 15, 2026 •

edited

Loading

Uh oh!

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

Copy link

Copy Markdown

Member

the failure looks like unrelated

ERROR: test_saturation (test.test_concurrent_futures.test_process_pool.ProcessPoolForkProcessPoolExecutorTest.test_saturation)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/runner/work/RustPython/RustPython/crates/pylib/Lib/test/test_concurrent_futures/test_process_pool.py", line 114, in test_saturation
    executor.submit(sem.acquire)
                    ^^^
  File "/Users/runner/work/RustPython/RustPython/crates/pylib/Lib/concurrent/futures/process.py", line 791, in submit
    raise BrokenProcessPool(self._broken)
concurrent.futures.process.BrokenProcessPool: A child process terminated abruptly, the process pool is not usable anymore



2 tests failed:
    test.test_concurrent_futures.test_process_pool
    test.test_concurrent_futures.test_shutdown

All reactions

Sorry, something went wrong.

Uh oh!

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

youknowone self-requested a review

January 15, 2026 10:54

youknowone reviewed

Jan 15, 2026

View reviewed changes

Comment thread

Lib/test/test_ftplib.py Resolved

Uh oh!

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


          Apply suggestion from @youknowone

6b57027

youknowone approved these changes

Jan 15, 2026

View reviewed changes

youknowone 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.

@terryluan12 Thank you so much! I wish your awesome script to be integrated to our tools soon. Unskipping tests is not a 5 seconds task, you made it in great way. Thanks!

No issue for 6 runs. It could be not lucky enough though

Sorry, something went wrong.

Uh oh!

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

ShaharNaveh reacted with thumbs up emoji

All reactions

👍 1 reaction

youknowone merged commit 609dbb1 into RustPython:main

Jan 15, 2026

13 checks passed

Uh oh!

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

Reviewers

fanninpm

fanninpm left review comments

ShaharNaveh

ShaharNaveh requested changes

youknowone

youknowone approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

4 participants

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.

GeezerVison

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!

youknowone commented Jan 14, 2026

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

youknowone left a comment

Choose a reason for hiding this comment

terryluan12 commented Jan 12, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

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

coderabbitai Bot commented Jan 12, 2026 •

edited

Loading

Uh oh!

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

youknowone Jan 14, 2026 •

edited

Loading

Uh oh!

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

fanninpm Jan 13, 2026 •

edited

Loading

Uh oh!

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

terryluan12 Jan 14, 2026 •

edited

Loading

Uh oh!

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

youknowone commented Jan 15, 2026 •

edited

Loading

Uh oh!

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