Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minor change in pytest xfail usage in tests #191

Merged
merged 1 commit into from
May 31, 2022
Merged

Conversation

ap--
Copy link
Contributor

@ap-- ap-- commented May 31, 2022

Hi @mdshw5,

This PR is very much optional. I noticed that you were using pytest.mark.xfail to mark tests that raise specific Exceptions. This might be my OCD talking, but it's not the intended usage for xfail, which should be used to mark tests that either fail due to missing implementations or because they are expected to fail due to other reasons: https://docs.pytest.org/en/6.2.x/skipping.html

If the code should raise an Exception due to wrong input or other mistakes it's better to use pytest.raises.

In your test-suite it seemed to me that only two tests qualified for being marked as xfail.
Again, this is a very minor distinction. Feel free to close the PR if you prefer using xfail.

Cheers,
Andreas 😃

@mdshw5
Copy link
Owner

mdshw5 commented May 31, 2022

Thanks for sorting this out. I was confused about expected failures vs expected exceptions when I made the transition to pytest. I'll gladly merge this PR.

@mdshw5 mdshw5 merged commit 6b7d4c5 into mdshw5:master May 31, 2022
@ap-- ap-- deleted the pytest-xfail branch May 31, 2022 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants