Fix sympy.testing.pytest.raises to behave like pytest.raises

[eric-wieser]

There seems to be a mistaken impression that XFAIL is needed for tests with raises. This should not be true.

This was true...

Code written as assert raises(...) would pass in pytest and fail in sympy, because sympy was not returning an appropriate object from raises.

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

• other
  • assert sympy.testing.pytest.raises(Exception, func) no longer always asserts when pytest is not present

[sympy-bot]

✅

Hi, I am the SymPy bot (v160). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

• other
  • assert sympy.testing.pytest.raises(Exception, func) no longer always asserts when pytest is not present (#20012 by @eric-wieser)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.7.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

~~There seems to be a mistaken impression that `XFAIL` is needed for tests with `raises`. This should not be true.~~

This was true...

Code written as `assert raises(...)` would pass in pytest and fail in sympy, because sympy was not returning an appropriate object from `raises`.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed


#### Other comments


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* other
   * `assert sympy.testing.pytest.raises(Exception, func)` no longer always asserts when pytest is not present
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[eric-wieser]

CI failure looks like a PyPy network issue.

[asmeurer]

Looks like it is working after a restart.

[codecov]

Codecov Report

Merging #20012 into master will decrease coverage by 0.019%.
The diff coverage is 85.714%.

@@              Coverage Diff              @@
##            master    #20012       +/-   ##
=============================================
- Coverage   75.878%   75.858%   -0.020%     
=============================================
  Files          669       669               
  Lines       173564    173613       +49     
  Branches     40969     40971        +2     
=============================================
+ Hits        131697    131701        +4     
- Misses       36135     36177       +42     
- Partials      5732      5735        +3     

[eric-wieser]

Good to merge then?

[oscarbenjamin]

This looks good and should be merged for compatibility between pytest and sympy's test runner. Personally I think that we should just use pytest all the time but that's another issue.

I also think that we shouldn't use assert with raises. There are only a handful of cases but they should all be changed. My preferred form is with raises but I understand that raises/lambda is used for a single liner. Using assert raises just seems like a misunderstanding to me.

[asmeurer]

I agree assert raises might be better if it raised an error (raises could return an object with a __bool__ that gives an error). If we want to be compatible with pytest we should allow it, since pytest apparently doesn't have any issues with assert raises().

I personally like the lambda form better because it avoids this issue

# Test that these all raise ValueError
with raises(ValueError):
    f(...)
    g(...)
    ...

That can't work. Only the first exception would be tested. It's an easy mistake to make, even if you know about it. So I would prefer the lambda form unless the thing being tested can only be written as a statement.

[oscarbenjamin]

Only the first exception would be tested.

I agree that this is a problem. Our contributors are generally not sufficiently experienced to avoid that so it is better to prefer the lambda approach most of the time.

Related to that is the warns context manager which only works as a context manager. You can see examples where it is being used incorrectly:

$ git grep -A 2 'with warns'
...
--
sympy/crypto/tests/test_crypto.py:    with warns(NonInvertibleCipherWarning):
sympy/crypto/tests/test_crypto.py-        assert rsa_public_key(2, 2, 1) == (4, 1)
sympy/crypto/tests/test_crypto.py-        assert rsa_public_key(8, 8, 8) is False
...

[eric-wieser]

Personally I think that we should just use pytest all the time but that's another issue.

I also think that we shouldn't use assert with raises.

Agreed on both points. If you feel strongly about the second, you could make a patch to pytest to make ExceptionInfo.__bool__ emit a warning.

[asmeurer]

FWIW I'm not a huge fan of pytest. But it's unrelated to this PR, so should be discussed elsewhere.

I don't know if there's any way to have a code quality test that checks against the context manager problem. I think it's kind of like the problem of forgetting to write assert on a test, where we just have to keep an eye out in review, and grep the tests every once in a while to fix them.

[oscarbenjamin]

I think that this is just something we should pick up on review. It could be listed in the dev guide somewhere as well with an explanation. For now though we can clean it up. There is also an example with raises:

$ git grep -A 2 'with raises'
...
sympy/matrices/expressions/tests/test_hadamard.py:    with raises(ShapeError):
sympy/matrices/expressions/tests/test_hadamard.py-        hadamard_product(A, C)
sympy/matrices/expressions/tests/test_hadamard.py-        hadamard_product(A, I)
...

I don't see any automated way to pick up on the context manager problem without false positives unless we require only a single statement in the block. You could have something like

with warns(...):
    x = setup1()
    y = setup2()
    assert test_deprecated(x, y) == 1

If the intention is to test that test_deprecated works and also gives a warning then this is a valid way of writing the test. We could however require that all setup be moved out of the with statement like:

x = setup1()
y = setup2()
with warns(...):
    assert test_deprecated(x, y) == 1

That's a more accurate test of the warning but it could lead to less readable tests in some cases.

[eric-wieser]

Putting this in to unblock #19463. While the discussion is useful and worth continuing, I don't think it's relevant to whether this PR should be merged, and #20012 (comment) seems to be in favor of merging

[asmeurer]

I started https://github.com/sympy/sympy/wiki/Writing-tests, with a gotchas section that has these things. I also added some other basic info on writing tests, but more could be added.