◐ Shell
reader mode source ↗
Skip to content

Downgraded skips in tests#6716

Merged
youknowone merged 13 commits into
RustPython:mainfrom
terryluan12:downgrade_tests
Jan 15, 2026
Merged

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

Conversation

@terryluan12

@terryluan12 terryluan12 commented Jan 12, 2026 •
edited by coderabbitai Bot
Loading

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.

@coderabbitai

coderabbitai Bot commented Jan 12, 2026 •
edited
Loading

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.

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

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

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

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)

fanninpm
fanninpm reviewed Jan 13, 2026
View reviewed changes

@fanninpm fanninpm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

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?

@terryluan12 terryluan12 force-pushed the downgrade_tests branch 3 times, most recently from 33cead3 to 1c62ce3 Compare January 13, 2026 05:44
ShaharNaveh
ShaharNaveh requested changes Jan 13, 2026
View reviewed changes

@ShaharNaveh ShaharNaveh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

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

TYSM!!!

@terryluan12 terryluan12 marked this pull request as ready for review January 13, 2026 17:01
youknowone
youknowone requested changes Jan 14, 2026
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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.

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

terryluan12 commented Jan 14, 2026

Copy link
Copy Markdown
Contributor Author

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

@youknowone

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.

@terryluan12 terryluan12 mentioned this pull request Jan 15, 2026
Merged
@youknowone

youknowone commented Jan 15, 2026 •
edited
Loading

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

@youknowone youknowone self-requested a review January 15, 2026 10:54
youknowone
youknowone approved these changes Jan 15, 2026
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

@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

Hide details View details @youknowone youknowone merged commit 609dbb1 into RustPython:main Jan 15, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fanninpm fanninpm

@ShaharNaveh ShaharNaveh ShaharNaveh requested changes

@youknowone youknowone

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@terryluan12 @youknowone @fanninpm @ShaharNaveh