Mark Windows-specific test failures as expected failures by samchakra0204 · Pull Request #7737 · RustPython/RustPython

samchakra0204 · 2026-04-29T14:13:42Z

Added @unittest.expectedFailure + TODO: RUSTPYTHON to:
- test_pathlib: surrogate/Windows-path tests
- test_posixpath: invalid path tests (islink, ismount, realpath)
Other previously skipped tests (rlcompleter, venv) were already marked
No CI skip list existed in this fork, so no YAML change needed Closes Handle windows test proper way #3960

Summary by CodeRabbit

Chores
- Standardized CI test matrix across macOS, Ubuntu, and Windows.
- Cleared platform-specific environment-sensitive test lists for consistency.
- Normalized extra test arguments to empty values.
- Retained existing platform skip lists and timeout settings.

- Added @unittest.expectedFailure + TODO: RUSTPYTHON to: - test_pathlib: surrogate/Windows-path tests - test_posixpath: invalid path tests (islink, ismount, realpath) - Other previously skipped tests (rlcompleter, venv) were already marked - No CI skip list existed in this fork, so no YAML change needed Closes RustPython#3960

coderabbitai · 2026-04-29T14:13:52Z

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

📝 Walkthrough

Walkthrough

The CI snippets_cpython job matrix now clears env_polluting_tests for macOS, Ubuntu, and Windows entries; macOS/Ubuntu also set skips: [] while Windows keeps its existing skips block and timeout, and preserves extra_test_args values.

Changes

CI matrix edits

Layer / File(s)	Summary
macOS & Ubuntu matrix entries `.github/workflows/ci.yaml`	Set `env_polluting_tests: []` and `skips: []` for the macOS and Ubuntu `snippets_cpython` matrix entries; keep existing `extra_test_args` and `timeout` values.
Windows matrix entry `.github/workflows/ci.yaml`	Set `env_polluting_tests: []` for the `windows-2025` `snippets_cpython` matrix entry; keep `extra_test_args: []`, preserve the existing Windows `skips` exclusions block and `timeout`.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

fanninpm

Poem

🐰 I nudged the YAML, made it neat,
Brackets emptied, tidy seat,
Lists now quiet, runners wake,
Small hops tidy CI's cake,
Hop on — quick checks never late.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly describes the main change: marking Windows-specific test failures as expected failures, which aligns with removing CI skips and adding test markers.
Linked Issues check	✅ Passed	The PR addresses the objectives in `#3960` by removing CI-level skips for test_pathlib and test_posixpath from ci.yaml and adding `@unittest.expectedFailure` markers to Windows-specific failing tests.
Out of Scope Changes check	✅ Passed	Changes to ci.yaml are limited to clearing env_polluting_tests for relevant test jobs, which is directly scoped to the PR's objective of removing CI-level skips for Windows test handling.
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.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

github-actions · 2026-04-29T14:14:10Z

📦 Library Dependencies

The following Lib/ modules were modified. Here are their dependencies:

[ ] lib: cpython/Lib/venv
[ ] test: cpython/Lib/test/test_venv.py (TODO: 4)

dependencies:

venv (native: _winapi, sys)
- sysconfig (native: _sysconfig, _winapi, importlib.machinery, importlib.util, os.path, sys)
- argparse, logging, os, shlex, shutil, subprocess, types

dependent tests: (1 tests)

venv: test_venv

Legend:

[+] path exists in CPython
[x] up-to-date, [ ] outdated

ShaharNaveh

Tysm for the patch!

Can you pleas remove the relevant tests from:

RustPython/.github/workflows/ci.yaml

Lines 245 to 249 in 6b67067

    
           skips: 
        
             - test_rlcompleter 
        
             - test_pathlib # panic by surrogate chars 
        
             - test_posixpath # OSError: (22, 'The filename, directory name, or volume label syntax is incorrect. (os error 123)') 
        
             - test_venv # couple of failing tests

So the CI will test them?

- Removed skips for test_rlcompleter, test_pathlib, test_posixpath, test_venv - CI will now run these tests and show expected failures - Addresses reviewer feedback

samchakra0204 · 2026-04-29T14:37:49Z

@ShaharNaveh Thanks for the catch! I’ve removed the skips: block for test_rlcompleter, test_pathlib, test_posixpath, and test_venv from .github/workflows/ci.yaml. The Windows CI will now run those modules, and the @unittest.expectedFailure markers should show them as expected failures instead of hiding them. Ready for another look when you have a moment.

fanninpm · 2026-04-29T14:38:11Z

You may need to use RustPython's custom @unittest.expectedFailureIfWindows("TODO: RUSTPYTHON, ...") decorator if it fails only on windows.

ShaharNaveh

lgtm if CI passes

fanninpm · 2026-04-29T15:04:30Z

if CI passes

About that...

ShaharNaveh · 2026-04-29T15:21:09Z

        self.assertEqual(st, p.lstat())

+    # TODO: RUSTPYTHON
+    @unittest.expectedFailure


Suggested change

@unittest.expectedFailure

@unittest.expectedFailureIfWindows("TODO: RUSTPYTHON")

The same for the rest of the patches, and then it should be good

I had to update your comment because that decorator expects another argument.

RustPython/Lib/unittest/__init__.py

Lines 99 to 112 in 6b67067

# XXX: RUSTPYTHON

# This is very useful to reduce platform difference boilerplates in tests.

def expectedFailureIf(condition, reason):

assert reason.startswith("TODO: RUSTPYTHON")

if condition:

return expectedFailure

else:

return lambda x: x

# XXX: RUSTPYTHON

# Even more useful because most of them are windows only.

def expectedFailureIfWindows(reason):

import sys

return expectedFailureIf(sys.platform == 'win32', reason)

ShaharNaveh · 2026-05-07T14:38:21Z

-            - test_rlcompleter
-            - test_pathlib # panic by surrogate chars
-            - test_posixpath # OSError: (22, 'The filename, directory name, or volume label syntax is incorrect. (os error 123)')
-            - test_venv # couple of failing tests


@samchakra0204 you can leave test_venv here, and fix it in a separate PR if you want to:)

ShaharNaveh · 2026-05-08T06:10:18Z

+            else:
+                self.exe = 'python.exe'
+        else:
+            self.exe = os.path.split(executable)[-1]


Please revert this part @samchakra0204

samchakra0204 · 2026-05-09T04:13:25Z

@ShaharNaveh Hi, I’ve applied the requested revert in Lib/test/test_venv.py and pushed the latest update. Could you please take another look and verify the CI/CD pipeline test cases for this PR, especially the Windows test_venv / test_pathlib / test_posixpath coverage?

ShaharNaveh

ty:)

- Add @unittest.expectedFailureIfWindows decorators to venv tests that execute the venv python.exe - Wrap subprocess calls in try/except to convert FileNotFoundError to AssertionError for expectedFailure to work

ShaharNaveh · 2026-05-09T08:31:06Z

+                    out, err = check_output(cmd, encoding='utf-8')
+                    self.assertEqual(out.strip(), expected, err)
+                except Exception as e:
+                    self.fail(f"venv executable failed: {e}")


We want to minimize our manual changes for upstream code.

If you want to can take the file from #7798 and apply your changes to it.

ShaharNaveh · 2026-05-09T08:33:26Z

-            - test_rlcompleter
-            - test_pathlib # panic by surrogate chars
-            - test_posixpath # OSError: (22, 'The filename, directory name, or volume label syntax is incorrect. (os error 123)')
-            - test_venv # couple of failing tests


@samchakra0204 If okay, can we have 4 different PRs for each removed skipped test (with the necessary test markers). this would allow us to iterate more rapidly, and get some of the code to be merged quicker (by not being blocked by another unrelated change)

- test_defaults_with_str_path, test_defaults_with_pathlike, test_upgrade, test_zippath_from_non_installed_posix - Use @unittest.expectedFailureIfWindows with TODO: RUSTPYTHON comment

samchakra0204 · 2026-05-18T04:19:12Z

@ShaharNaveh Hi, I’ve resolved the latest requested changes and removed the stale Windows expected failures in test_venv. Could you please take another look when you have time, especially the pending Windows CI cases (test_pathlib, test_posixpath, test_venv, and workflow coverage)? Thanks!

ShaharNaveh · 2026-05-18T05:42:04Z

@ShaharNaveh Hi, I’ve resolved the latest requested changes and removed the stale Windows expected failures in test_venv. Could you please take another look when you have time, especially the pending Windows CI cases (test_pathlib, test_posixpath, test_venv, and workflow coverage)? Thanks!

@samchakra0204 can you please resolve the merge conflicts? overall this LGTM

ShaharNaveh · 2026-05-18T07:08:47Z

-            - test_pathlib # panic by surrogate chars
-            - test_posixpath # OSError: (22, 'The filename, directory name, or volume label syntax is incorrect. (os error 123)')
-            - test_venv # couple of failing tests
+          env_polluting_tests: []


Suggested change

env_polluting_tests: []

env_polluting_tests:

- test_set

ShaharNaveh · 2026-05-18T07:14:20Z

        self.run_with_capture(venv.create, self.env_dir)
        self._check_output_of_default_create()

-    @unittest.expectedFailure  # TODO: RUSTPYTHON


Those probably needs to be:

@unittest.expectedFailureIf(sys.platform != "win32", "TODO: RUSTPYTHON")

…ts and fix expectedFailureIf decorator

samchakra0204 · 2026-05-18T07:37:33Z

@ShaharNaveh Thanks! I’ve applied the latest reviewer suggestions, resolved the merge conflicts with upstream/main, and pushed the updated changes (b9096ac). The PR should now be ready for another review once the pending workflow approvals and CI checks complete.

…d posixpath tests

ShaharNaveh · 2026-05-18T08:02:06Z

        fn = self.get_env_file(*args)
        self.assertTrue(os.path.isdir(fn))

-    @unittest.expectedFailure  # TODO: RUSTPYTHON


This one would probably needs to be adjusted as well

@samchakra0204 please take a look at this comment and the ones below

ShaharNaveh · 2026-05-18T08:02:14Z

        self.run_with_capture(venv.create, self.env_dir)
        self._check_output_of_default_create()

-    @unittest.expectedFailure  # TODO: RUSTPYTHON


ShaharNaveh · 2026-05-18T08:02:22Z

            self.assertRaises((ValueError, OSError), venv.create, self.env_dir)
            self.clear_directory(self.env_dir)

-    @unittest.expectedFailure  # TODO: RUSTPYTHON


samchakra0204 · 2026-05-18T09:35:46Z

@ShaharNaveh Hi! I've fixed the failing CI checks — the decorator was mistakenly placed before test_doctest_issue4197 with incorrect indentation. I've moved it to the correct location above test_doctest_main_issue4197 with proper indentation.

Could you please take another look when you have a moment? Thanks!

ShaharNaveh

lgtm, once CI passes:)

ShaharNaveh · 2026-05-18T10:13:07Z

@samchakra0204 please resolve the CI failures. After that we can merge:)

Removed env_polluting_tests from macOS, Ubuntu, and Windows configurations.

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In @.github/workflows/ci.yaml:
- Around line 302-305: The Windows matrix include for "windows-2025" is missing
the "skips" key which is read unconditionally later; add a "skips: []" entry to
the windows-2025 include object so its schema matches the macOS/Ubuntu entries
and avoids undefined access to matrix.skips.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: a9b66f1c-b1bb-40ff-8a64-2d781f16d39c

📥 Commits

Reviewing files that changed from the base of the PR and between b9096ac and ddcce88.

📒 Files selected for processing (1)

.github/workflows/ci.yaml

Added skipped tests for Windows 2025 environment.

- Remove stale Windows test skips from CI (test_rlcompleter, test_pathlib, test_posixpath, test_venv) - Add test_set to env_polluting_tests for Windows CI - Remove contradictory @unittest.expectedFailureIf decorator from test_zippath_from_non_installed_posix (POSIX-specific test should only skip on Windows)

ShaharNaveh · 2026-05-18T12:30:02Z

            - '-u all'
-          env_polluting_tests:
-            - test_set
+          env_polluting_tests: []


@samchakra0204 what is the motivation for this change? the CI issues were related to test_venv

Good point. The CI failure I was targeting was test_venv, so changing env_polluting_tests / unskipping unrelated tests was too broad. I’ll keep test_set as an env-polluting test and scope the CI change to only removing test_venv from the Windows skips.

ShaharNaveh · 2026-05-18T13:16:20Z

+        sys.implementation.name == 'rustpython',
+        "TODO: RUSTPYTHON - doctest captures bare string expressions differently"
+    )
    def test_doctest_main_issue4197(self):


what does test_zipimport_support has to do with anything? could you please elaborate?

samchakra0204 mentioned this pull request Apr 29, 2026

Handle windows test proper way #3960

Open

11 tasks

ShaharNaveh requested changes Apr 29, 2026

View reviewed changes

Remove Windows test skips from CI to enable expected failure testing

23a47a3

- Removed skips for test_rlcompleter, test_pathlib, test_posixpath, test_venv - CI will now run these tests and show expected failures - Addresses reviewer feedback

ShaharNaveh approved these changes Apr 29, 2026

View reviewed changes

ShaharNaveh reviewed Apr 29, 2026

View reviewed changes

samchakra0204 added 2 commits May 7, 2026 16:24

Use expectedFailureIfWindows to avoid unexpected passes on non-Windows

cbf6fb7

Normalize Windows venv executable name in test_venv

005fca1

ShaharNaveh requested changes May 7, 2026

View reviewed changes

Comment thread Lib/test/test_venv.py

ShaharNaveh reviewed May 7, 2026

View reviewed changes

Use expectedFailureIfWindows for venv tests failing only on Windows

f3c3454

ShaharNaveh mentioned this pull request May 7, 2026

Update test_venv.py to 3.14.4 #7798

Merged

samchakra0204 requested a review from ShaharNaveh May 8, 2026 03:52

ShaharNaveh requested changes May 8, 2026

View reviewed changes

Revert Windows-specific venv executable normalization in test_venv

d31cda8

ShaharNaveh approved these changes May 9, 2026

View reviewed changes

Mark venv tests as expected failures on Windows and catch exceptions

20a3067

- Add @unittest.expectedFailureIfWindows decorators to venv tests that execute the venv python.exe - Wrap subprocess calls in try/except to convert FileNotFoundError to AssertionError for expectedFailure to work

ShaharNaveh requested changes May 9, 2026

View reviewed changes

ShaharNaveh reviewed May 9, 2026

View reviewed changes

Minimize venv patch to Windows expected failures only

29e542b

samchakra0204 requested a review from ShaharNaveh May 9, 2026 12:07

Mark 4 failing venv tests as expected failure on Windows

b1e1f44

- test_defaults_with_str_path, test_defaults_with_pathlike, test_upgrade, test_zippath_from_non_installed_posix - Use @unittest.expectedFailureIfWindows with TODO: RUSTPYTHON comment

youknowone enabled auto-merge (squash) May 9, 2026 15:33

Fix mangled UTF-8 in venv Windows expected failure markers

16897fa

auto-merge was automatically disabled May 18, 2026 04:07
Head branch was pushed to by a user without write access

Remove stale Windows expected failures in test_venv

56c17f0

ShaharNaveh approved these changes May 18, 2026

View reviewed changes

Merge remote-tracking branch 'upstream/main'

108e4c4

ShaharNaveh reviewed May 18, 2026

View reviewed changes

Apply reviewer suggestions: add test_set to Windows env_polluting_tes…

b9096ac

…ts and fix expectedFailureIf decorator

Remove stale Windows expectedFailureIfWindows markers from pathlib an…

030bee3

…d posixpath tests

ShaharNaveh reviewed May 18, 2026

View reviewed changes

samchakra0204 requested a review from ShaharNaveh May 18, 2026 09:47

ShaharNaveh approved these changes May 18, 2026

View reviewed changes

Clear env_polluting_tests for CI workflows

ddcce88

Removed env_polluting_tests from macOS, Ubuntu, and Windows configurations.

coderabbitai Bot reviewed May 18, 2026

View reviewed changes

Comment thread .github/workflows/ci.yaml

samchakra0204 and others added 2 commits May 18, 2026 17:41

Skip specific tests in Windows 2025 CI workflow

7f2a95e

Added skipped tests for Windows 2025 environment.

samchakra0204 requested a review from ShaharNaveh May 18, 2026 12:26

ShaharNaveh requested changes May 18, 2026

View reviewed changes

Fix snippets CI failures

479e21a

samchakra0204 requested a review from ShaharNaveh May 18, 2026 13:05

ShaharNaveh requested changes May 18, 2026

View reviewed changes

Scope venv expected failures

a66133f

ShaharNaveh approved these changes May 18, 2026

View reviewed changes

Update test_venv.py

bd3cbcc

samchakra0204 requested a review from ShaharNaveh May 19, 2026 02:58

	skips:
	- test_rlcompleter
	- test_pathlib # panic by surrogate chars
	- test_posixpath # OSError: (22, 'The filename, directory name, or volume label syntax is incorrect. (os error 123)')
	- test_venv # couple of failing tests

	@unittest.expectedFailure
	@unittest.expectedFailureIfWindows("TODO: RUSTPYTHON")

	# XXX: RUSTPYTHON
	# This is very useful to reduce platform difference boilerplates in tests.
	def expectedFailureIf(condition, reason):
	assert reason.startswith("TODO: RUSTPYTHON")
	if condition:
	return expectedFailure
	else:
	return lambda x: x

	# XXX: RUSTPYTHON
	# Even more useful because most of them are windows only.
	def expectedFailureIfWindows(reason):
	import sys
	return expectedFailureIf(sys.platform == 'win32', reason)

Conversation

samchakra0204 commented Apr 29, 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 Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Uh oh!

github-actions Bot commented Apr 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

📦 Library Dependencies

Uh oh!

ShaharNaveh left a comment

Choose a reason for hiding this comment

Uh oh!

samchakra0204 commented Apr 29, 2026

Uh oh!

fanninpm commented Apr 29, 2026

Uh oh!

ShaharNaveh left a comment

Choose a reason for hiding this comment

Uh oh!

fanninpm commented Apr 29, 2026

Uh oh!

ShaharNaveh Apr 29, 2026 • edited by fanninpm Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samchakra0204 commented May 9, 2026

Uh oh!

ShaharNaveh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samchakra0204 commented May 18, 2026

Uh oh!

ShaharNaveh commented May 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samchakra0204 commented May 18, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samchakra0204 commented May 18, 2026

Uh oh!

ShaharNaveh left a comment

Choose a reason for hiding this comment

Uh oh!

ShaharNaveh commented May 18, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

samchakra0204 commented Apr 29, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Apr 29, 2026 •

edited

Loading

github-actions Bot commented Apr 29, 2026 •

edited

Loading

ShaharNaveh Apr 29, 2026 •

edited by fanninpm

Loading