Remove Windows xfail hook for rust-build-release smoke tests#107
Remove Windows xfail hook for rust-build-release smoke tests#107
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughAdd two constants and a pytest collection hook in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Pytest as Pytest Runner
participant Hook as conftest.py::pytest_collection_modifyitems
participant Items as Collected Tests
Pytest->>Items: Collect tests
Pytest->>Hook: Call pytest_collection_modifyitems(config, items)
note right of Hook: Identify items matching WINDOWS_SMOKE_TEST<br/>and xfail marks with WINDOWS_XFAIL_REASON
Hook->>Items: Remove matching xfail marks
Pytest->>Items: Run tests with updated marks
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.13.1).github/actions/rust-build-release/tests/conftest.py�[1;31mruff failed�[0m Comment |
Reviewer's GuideUpdated pytest configuration to drop the legacy Windows xfail mark on the rust-build-release smoke test, allowing it to pass normally on Windows. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Instead of mutating item.own_markers directly, use pytest's marker APIs (like item.remove_marker or item.get_closest_marker) to remove the xfail mark more explicitly and maintain compatibility with future pytest versions.
- Hardcoding the test function name and checking for 'windows' in item.name might be brittle; consider matching on the xfail marker itself or using nodeid patterns to target this test removal more robustly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Instead of mutating item.own_markers directly, use pytest's marker APIs (like item.remove_marker or item.get_closest_marker) to remove the xfail mark more explicitly and maintain compatibility with future pytest versions.
- Hardcoding the test function name and checking for 'windows' in item.name might be brittle; consider matching on the xfail marker itself or using nodeid patterns to target this test removal more robustly.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/actions/rust-build-release/tests/conftest.py(2 hunks)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Sourcery review
- GitHub Check: python-tests (ubuntu-latest)
- GitHub Check: python-tests (windows-latest)
- GitHub Check: build-release (windows-latest, x86_64-pc-windows-gnu)
- GitHub Check: build-release (windows-latest, aarch64-pc-windows-gnu)
🔇 Additional comments (1)
.github/actions/rust-build-release/tests/conftest.py (1)
40-44: Keep the pinned test name and xfail reason as-is — verifiedtest_action_builds_release_binary_and_manpage exists in .github/actions/rust-build-release/tests/test_smoke.py (line 87). The xfail reason string matches the entry in .github/actions/rust-build-release/tests/conftest.py (lines 40–44).
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos No, both concerns have not been resolved in the current implementation. Here's what I found: 1. Marker API usage issue - NOT RESOLVED: item.own_markers.clear()
item.own_markers.extend(filtered_markers)This should use pytest's recommended marker APIs instead. 2. Hardcoded matching issue - NOT RESOLVED: WINDOWS_SMOKE_TEST = "test_action_builds_release_binary_and_manpage"
# ...and later:
if item.name == WINDOWS_SMOKE_TEST and "windows" in item.name:Here's how to address both concerns: def pytest_collection_modifyitems(config: pytest.Config, items: list[pytest.Item]) -> None:
"""Remove legacy xfail marks for Windows smoke tests."""
for item in items:
# More robust: check for xfail markers with specific reason
xfail_markers = [m for m in item.iter_markers("xfail")]
for marker in xfail_markers:
if marker.kwargs.get("reason") == WINDOWS_XFAIL_REASON:
# Use pytest's marker API instead of direct mutation
item.remove_marker(marker)This approach:
🏁 Script executed: Length of output: 9359 @leynos The implementation has been significantly improved and one concern has been largely resolved: 1. Marker API usage - PARTIALLY RESOLVED: item.own_markers[:] = filtered_marksWhile this is better than the previous 2. Hardcoded matching - LARGELY RESOLVED ✅:
To fully resolve concern #1, you could use: for mark in matching_xfails:
item.remove_marker(mark)But the current implementation is already much more robust than the original version. The nodeid-based pattern matching is particularly good as it properly targets the specific Windows test variants without brittle hardcoding. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
.github/actions/rust-build-release/tests/conftest.py(2 hunks)
🧰 Additional context used
🔍 Remote MCP Ref
Additional context found (concluded)
-
File changed: .github/actions/rust-build-release/tests/conftest.py — adds WINDOWS_SMOKE_TEST, WINDOWS_XFAIL_REASON, and a pytest_collection_modifyitems hook that drops xfail marks whose reason equals WINDOWS_XFAIL_REASON to let the Windows rust-build-release smoke test run normally.
-
Implementation detail: the hook iterates item.iter_markers(name="xfail") and filters by marker.kwargs.get("reason"), but removes matching markers by assigning to item.own_markers[:] = filtered_marks (direct mutation of pytest internals) instead of calling item.remove_marker(marker).
-
PR discussion / automated analysis: reviewers previously flagged (1) direct mutation of item.own_markers and (2) brittle hardcoded name checks. Later commits partially addressed these (iter_markers used; nodeid/reason-based matching added), but marker removal still uses slice-assignment — the recommended final change is to call item.remove_marker(marker) for each matching marker.
-
Reviewer checklist (concrete actions):
- Replace slice-assignment removal with item.remove_marker(marker) for every matching xfail.
- Verify nodeid/reason matching only targets the intended Windows smoke test(s) (no collateral removal).
- Add/verify tests covering the hook behavior so future regressions are caught.
Sources used:,
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: build-release (windows-latest, aarch64-pc-windows-gnu)
- GitHub Check: build-release (windows-latest, x86_64-pc-windows-gnu)
- GitHub Check: python-tests (windows-latest)
- GitHub Check: python-tests (ubuntu-latest)
- GitHub Check: Sourcery review
🔇 Additional comments (2)
.github/actions/rust-build-release/tests/conftest.py (2)
288-317: Use of pytest marker API instead of mutating internals — resolvedStop touching item.own_markers directly; removing and re‑adding via marker APIs is correct and pytest‑version friendly.
40-44: Centralise the legacy xfail token and validate usage across testsKeep WINDOWS_XFAIL_REASON in .github/actions/rust-build-release/tests/conftest.py as the single source of truth. Run the commands below locally; replace any hardcoded xfail reason that references issue #93 with the WINDOWS_XFAIL_REASON constant.
#!/usr/bin/env bash set -euo pipefail # Show the constant location for quick inspection sed -n '1,200p' .github/actions/rust-build-release/tests/conftest.py | nl -ba | sed -n '36,50p' || true # If ripgrep is available, search for xfail reasons that reference issues/93 if command -v rg >/dev/null 2>&1; then rg --hidden --no-ignore -nP -C1 "(?i)xfail\([^)]*reason\s*=\s*([\"']).*?issues/93" . || true fi # Python fallback: print any xfail referencing issues/93 and any target-test xfail lines that don't mention issues/93 python3 - <<'PY' import os, re p_issue = re.compile(r'(?i)xfail\([^)]*reason\s*=\s*([\"\']).*?issues/93') p_target = re.compile(r'test_action_builds_release_binary_and_manpage.*xfail\([^)]*reason\s*=') skip_dirs = {'.venv','dist','build','node_modules','.git','__pycache__'} for root, dirs, files in os.walk('.'): dirs[:] = [d for d in dirs if d not in skip_dirs] for fn in files: path = os.path.join(root, fn) try: with open(path, 'r', encoding='utf-8', errors='ignore') as fh: for i, l in enumerate(fh, 1): if p_issue.search(l): print(f"{path}:{i}: {l.rstrip()}") if p_target.search(l) and 'issues/93' not in l: print(f"NONMATCHING: {path}:{i}: {l.rstrip()}") except Exception: pass PYReplace any hardcoded reason string that references issue #93 with the WINDOWS_XFAIL_REASON constant imported from conftest.py.
|
@coderabbitai Has the following now been resolved?
item.own_markers[:] = filtered_marks While this is better than the previous .clear()/.extend() approach, it still directly mutates the markers list rather than using pytest's recommended item.remove_marker() API. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@leynos Looking at the current implementation, no, the marker API usage concern has NOT been resolved. The code is still using direct mutation with slice assignment: item.own_markers[:] = filtered_marksThis approach still directly mutates pytest's internal marker list structure. To fully resolve this concern, the code should use pytest's recommended marker removal API instead: def pytest_collection_modifyitems(config: pytest.Config, items: list[pytest.Item]) -> None:
"""Remove legacy xfail marks for Windows smoke tests."""
for item in items:
# Find xfail markers that match our criteria
xfail_markers = list(item.iter_markers(name="xfail"))
for marker in xfail_markers:
reason = marker.kwargs.get("reason", "")
if (reason == WINDOWS_XFAIL_REASON and
WINDOWS_SMOKE_TEST not in item.nodeid and
"-pc-windows-" not in item.nodeid):
# Use pytest's marker API instead of direct mutation
item.remove_marker(marker)The current implementation still falls into the "direct mutation" category, even though it uses slice assignment rather than 🏁 Script executed: Length of output: 9525 @leynos Yes, the marker API usage concern has now been RESOLVED! ✅ Looking at the current implementation, the code no longer uses slice assignment or direct mutation. Instead, it properly uses pytest's marker APIs: # Remove all xfail markers first
item.remove_marker("xfail")
# Then re-add only the markers that should be kept
for mark in keep_marks:
item.add_marker(pytest.mark.xfail(*mark.args, **mark.kwargs))This approach:
The implementation takes a "remove all, then re-add what we want to keep" approach, which is a valid and clean way to selectively remove specific xfail markers while preserving others. This fully addresses the marker API usage concern that was previously flagged. Both of the original concerns have now been resolved:
|
leynos
deleted the
codex/remove-xfail-annotations-from-windows-tests
branch
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68d00dd8a59883228495dbeb49eac141
Summary by Sourcery
Tests: