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
- Updated CI workflow configuration for test matrix handling on Windows platform.

- 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

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)

Lib/test/test_venv.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.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 8fa79d49-de08-483e-8a85-182949210e3c

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

Use the checkbox below for a quick retry:

🔍 Trigger review

📝 Walkthrough

Walkthrough

The CI workflow's snippets_cpython job Windows matrix entry is modified to explicitly set extra_test_args as an empty list with a comment indicating -u all remains disabled on Windows. Inline test exclusion entries are removed from the configuration.

Changes

Windows Test Configuration

Layer / File(s)	Summary
Windows Matrix Configuration `.github/workflows/ci.yaml`	Windows matrix entry now explicitly sets `extra_test_args` to empty list with TODO comment. Skipped test exclusion entries (`test_rlcompleter`, `test_pathlib`, `test_posixpath`, `test_venv`) are removed from workflow configuration.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Suggested reviewers

fanninpm
youknowone

Poem

🐰 Windows tests now cleanly configured,
Extra args removed, exclusions migrated,
The CI workflow runs more straight,
Matrix entries aligned and light,
A tidier path forward in sight! 🎯

🚥 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 accurately describes the main change—adding `@unittest.expectedFailure` markers for Windows-specific test failures—which aligns with the PR's primary objective.
Linked Issues check	✅ Passed	The PR addresses issue `#3960` by removing CI skips and marking Windows-specific failures; changes to test_pathlib and test_posixpath align with linked issue requirements and commit progress.
Out of Scope Changes check	✅ Passed	The CI YAML modification (removing Windows matrix entries and setting extra_test_args) directly supports the linked issue objective of removing CI skips for Windows tests.
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

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

Generate code and open pull requests
Plan features and break down work
Investigate incidents and troubleshoot customer tickets together
Automate recurring tasks and respond to alerts with triggers
Summarize progress and report instantly

Built for teams:

Shared memory across your entire org—no repeating context
Per-thread sandboxes to safely plan and execute work
Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started

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

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

	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!

Review skipped

Walkthrough

Changes

Estimated code review effort

Suggested reviewers

Poem

Review ran into problems

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!

fanninpm Apr 29, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ShaharNaveh May 7, 2026

Choose a reason for hiding this comment

Uh oh!

ShaharNaveh May 8, 2026

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!

ShaharNaveh May 9, 2026

Choose a reason for hiding this comment

Uh oh!

ShaharNaveh May 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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