Skip to content

Doc xfail #11501

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

Closed
wants to merge 4 commits into from
Closed

Doc xfail #11501

wants to merge 4 commits into from

Conversation

TanyaAgarwal28
Copy link
Contributor

Description:

This pull request addresses discrepancies in the documentation for pytest.mark.xfail() and aligns it with the actual behavior in the code. The key points of this PR include:

  • Clarified the behavior of the reason argument, specifically addressing the use of None.
  • Updated the expected type for the raises argument to match the code, allowing for None.
  • Highlighted the dependency of the strict argument on the xfail_strict configuration.

Fixes:

This PR resolves the issue #10094.

Documentation:

  • Modified the documentation for pytest.mark.xfail() to accurately represent its behavior based on the code.

Tests:

  • Added test cases to ensure that the documented behavior aligns with the actual code behavior.

Changelog:

  • Created a new changelog file: changelog/10094.improvement.rst to document the improvement made in this PR.

Author:
Tanya Agarwal

Comment on lines +1 to +4
The documentation for pytest.mark.xfail():
reason=None: If no conditions are passed, reason’s default is "" (code). Adding @pytest.mark.xfail(reason=None, strict=True) to a passing test results in “TypeError: can only concatenate str (not "NoneType") to str”. If a condition is passed, passing no reason behaves like passing None to it (code). I’m unsure how this subtlety could be captured in the function signature. pytest.mark.skipif() has the same issue. pytest.mark.skip()’s default value for reason is in fact "unconditional skip" (code).
raises: The type (according to the type annotations) is Union[Type[BaseException], Tuple[Type[BaseException], ...]] instead of Type[Exception]. Neither captures the fact that None can be passed to it (which is also the documented default value).
strict=False: The default value of strict is actually determined by the xfail_strict config (code).
Copy link
Member

Choose a reason for hiding this comment

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

The changelog should be written in a language targeting end users, not necessarily duplicating what was done in detail, so I suggest we go with:

Suggested change
The documentation for pytest.mark.xfail():
reason=None: If no conditions are passed, reason’s default is "" (code). Adding @pytest.mark.xfail(reason=None, strict=True) to a passing test results in “TypeError: can only concatenate str (not "NoneType") to str”. If a condition is passed, passing no reason behaves like passing None to it (code). I’m unsure how this subtlety could be captured in the function signature. pytest.mark.skipif() has the same issue. pytest.mark.skip()’s default value for reason is in fact "unconditional skip" (code).
raises: The type (according to the type annotations) is Union[Type[BaseException], Tuple[Type[BaseException], ...]] instead of Type[Exception]. Neither captures the fact that None can be passed to it (which is also the documented default value).
strict=False: The default value of strict is actually determined by the xfail_strict config (code).
Improved/fixed the documentation and signature for :func:`pytest.mark.xfail <pytest.mark.xfail>`.

unexpectedly passes then it will **fail** the test suite. This is particularly useful to mark functions
that are always failing and there should be a clear indication if they unexpectedly start to pass (for example
a new release of a library fixes a known bug).
* The value of `strict` is determined by the `xfail_strict` configuration.
Copy link
Member

Choose a reason for hiding this comment

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

This is duplicated from line 262:

Suggested change
* The value of `strict` is determined by the `xfail_strict` configuration.


Defaults to :confval:`xfail_strict`, which is ``False`` by default.

.. py:function:: pytest.mark.xfail(condition=None, *, reason=None, raises=None, run=True, strict=xfail_strict)
Copy link
Member

Choose a reason for hiding this comment

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

This is duplicated from line 240 and should be removed.

Suggested change
.. py:function:: pytest.mark.xfail(condition=None, *, reason=None, raises=None, run=True, strict=xfail_strict)

Comment on lines +249 to +251
If no conditions are passed, the default value of `reason` is an empty string ("").
However, please note that you should avoid passing `None` as the `reason`, as this can result in a TypeError.
If a condition is passed and no `reason` is specified, it behaves as if you passed `None` as the `reason`.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should be documenting this at all, we should just change the reason=None in line 240 to reason="" to reflect reality: users are not passing reason=None currently already as that would result in an error.

Suggested change
If no conditions are passed, the default value of `reason` is an empty string ("").
However, please note that you should avoid passing `None` as the `reason`, as this can result in a TypeError.
If a condition is passed and no `reason` is specified, it behaves as if you passed `None` as the `reason`.

@Zac-HD
Copy link
Member

Zac-HD commented Feb 23, 2024

Closing this because @TanyaAgarwal28 hasn't been seen on GitHub for several months. Happy to reopen if it updates!

@Zac-HD Zac-HD closed this Feb 23, 2024
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.

3 participants