Skip to content

Defer xfail condition evaluation with xfail_if_raises context manager#2153

Merged
Byron merged 1 commit into
gitpython-developers:mainfrom
elovelan:fix/xfail_side_effects
May 16, 2026
Merged

Defer xfail condition evaluation with xfail_if_raises context manager#2153
Byron merged 1 commit into
gitpython-developers:mainfrom
elovelan:fix/xfail_side_effects

Conversation

@elovelan
Copy link
Copy Markdown
Contributor

@elovelan elovelan commented May 15, 2026

Summary

This PR introduces xfail_if_raises, a context manager that approximates @pytest.mark.xfail(..., raises=...) behavior inside a test body rather than as a decorator.

Problem

@pytest.mark.xfail evaluates its condition argument during test collection (module import time). In test_index_mutation, this condition includes Git().config("core.symlinks") == "true", which:

  1. Spawns an external git config process even when the test is not being run
  2. Can fail or behave unexpectedly during collection if git configuration is unavailable
  3. Relies on in-repo code during test discovery, which may not be appropriate in all contexts

Solution

xfail_if_raises moves the xfail logic into the test body. The side-effectful condition is only evaluated if the test actually raises the specified exception(s). This keeps test collection lightweight and avoids relying on in-repo code as part of module import/test discovery.

This change is applied to test_index_mutation in test/test_index.py.

AI disclosure

This PR description was prepared with assistance from an agent. All code was written by hand.

@elovelan elovelan marked this pull request as draft May 15, 2026 15:52
The @pytest.mark.xfail decorator with raises=... evaluates its condition
during test collection, which can trigger external calls (e.g.,
Git().config()) that have side effects. Introduce xfail_if_raises as a
context manager that delays this evaluation until test execution time,
and apply it to test_index_mutation.
@elovelan elovelan force-pushed the fix/xfail_side_effects branch from 46daafe to 03baced Compare May 15, 2026 20:29
@elovelan elovelan marked this pull request as ready for review May 15, 2026 20:38
@elovelan
Copy link
Copy Markdown
Contributor Author

elovelan commented May 15, 2026

I didn't change anything that should have affected this one failed test on Cygwin. I have a Windows VM now so I'll see if I can repro.

@EliahKagan
Copy link
Copy Markdown
Member

The failed Cygwin test looks like a known flake with submodule ownership on Cygwin CI that I've been working on and hope to have a PR in for soon. To the best of my knowledge, that is independent of the changes here, though I emphasize that I have not reviewed this PR. I've triggered the test to rerun--my guess is that it will pass.

Copy link
Copy Markdown
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

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

Thanks a lot, looks good to me!

Image

Initially I was confused about strict as it's not actually used, but it's Ok and nothing one couldn't figure out and address next time someone gets confused by it.

@Byron Byron merged commit 9c5b57b into gitpython-developers:main May 16, 2026
31 of 32 checks passed
@elovelan
Copy link
Copy Markdown
Contributor Author

Initially I was confused about strict as it's not actually used, but it's Ok and nothing one couldn't figure out and address next time someone gets confused by it.

I debated a YAGNI approach, but I figured as a utility method, I should try to mirror the decorator as closely as I could.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants