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
Use assert ... is True in XFAIL tests #15817
Conversation
✅ Hi, I am the SymPy bot (v135). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.
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.
|
sympy/utilities/tests/test_wester.py
Outdated
@@ -735,7 +735,7 @@ def test_J16(): | |||
|
|||
@XFAIL | |||
def test_J17(): | |||
assert deltaintegrate(f((x + 2)/5)*DiracDelta((x - 2)/3) - g(x)*diff(DiracDelta(x - 1), x), (x, 0, 3)) | |||
raise NotImplementedError("deltaintegrate(f((x + 2)/5)*DiracDelta((x - 2)/3) - g(x)*diff(DiracDelta(x - 1), x), (x, 0, 3))") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
raise NotImplementedError("deltaintegrate(f((x + 2)/5)*DiracDelta((x - 2)/3) - g(x)*diff(DiracDelta(x - 1), x), (x, 0, 3))") | |
assert deltaintegrate(f((x + 2)/5)*DiracDelta((x - 2)/3) - g(x)*diff(DiracDelta(x - 1), x), (x, 0, 3)) is not None |
(or better, assert that it equals the correct answer).
XFAIL tests should ideally xpass once they start working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you're saying makes sense and I fully agree. I think that an xfail test should not only xpass when working but should actually be a reasonable non-xfail test of the correct behaviour.
Happy to change it but if you look at the surrounding code though you'll see why I did it like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually don't understand this test. Why is it testing the internal deltaintegrate
function? For whatever reason, it doesn't work, even if you split up the integral, but integrate
itself works just fine:
>>> integrate(f((x + 2)/5)*DiracDelta((x - 2)/3) - g(x)*diff(DiracDelta(x - 1), x), (x, 0, 3))
3*f(4/5) + Subs(Derivative(g(x), x), x, 1)
The Wester tests should be testing the public APIs.
So I actually think we should change this test to a passing one that tests the above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
raise NotImplementedError("deltaintegrate(f((x + 2)/5)*DiracDelta((x - 2)/3) - g(x)*diff(DiracDelta(x - 1), x), (x, 0, 3))") | |
assert integrate(f((x + 2)/5)*DiracDelta((x - 2)/3) - g(x)*diff(DiracDelta(x - 1), x), (x, 0, 3)) == 3*f(S(4)/5) + Subs(Derivative(g(x), x), x, 1) |
I have just the one comment. I don't really understand why pytest has an issue with |
I don't agree with pytest's issue with assert None. It seems unnecessary but at the same time I can see the argument that it is better to be explicit in tests and write |
This silences a warning about asserting None when running tests under pytest.
2f90875
to
2fade23
Compare
Brief description of what is fixed or changed
Changes failing asserts in xfail tests to use
assert ... is True
rather than justassert ...
. Otherwise the result isassert None
which triggers a warning under pytestOther comments
I raised this as an issue with
pytest
: pytest-dev/pytest#4639.Release Notes
NO ENTRY