Downgraded skips in tests by terryluan12 · Pull Request #6716 · RustPython/RustPython · GitHub

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.

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.

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

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

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?

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

@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

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

TYSM!!!

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

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.

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

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

…lures

… ENV_POLLUTING_TESTS_WINDOWS

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

@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

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

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

fanninpm reviewed Jan 13, 2026

View reviewed changes

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

Comment thread Lib/test/test_io.py Outdated

Comment thread Lib/test/test_io.py Outdated

Comment thread Lib/test/test_io.py Outdated

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

fanninpm reviewed Jan 13, 2026

View reviewed changes

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

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

youknowone force-pushed the downgrade_tests branch from 80a5f8b to 559d35d Compare January 14, 2026 11:51

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 self-requested a review January 15, 2026 10:54

youknowone reviewed Jan 15, 2026

View reviewed changes

Comment thread Lib/test/test_ftplib.py Outdated

Apply suggestion from @youknowone

6b57027

youknowone approved these changes Jan 15, 2026

View reviewed changes

youknowone merged commit 609dbb1 into RustPython:main Jan 15, 2026
13 checks passed

                   def setUp(self):
                       self.cx = sqlite.Connection.__new__(sqlite.Connection)
+                  @unittest.skip('TODO: RUSTPYTHON')

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

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

Downgraded skips in tests by terryluan12 · Pull Request #6716 · RustPython/RustPython · GitHub

Uh oh!

Downgraded skips in tests - #6716

Downgraded skips in tests#6716youknowone merged 13 commits intoRustPython:mainRustPython/RustPython:mainfrom terryluan12:downgrade_teststerryluan12/RustPython:downgrade_testsCopy head branch name to clipboard

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!

Downgraded skips in tests#6716
youknowone merged 13 commits into
RustPython:mainfrom
terryluan12:downgrade_tests

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

youknowone commented Jan 15, 2026 •
edited
Loading