test: fix dead assert in retry streaming tests causing coverage issues - #17723
Conversation
The assert statement was incorrectly indented inside the pytest.raises context manager, causing it to never execute. Dedented the assert to ensure exception messages are actually verified, which also resolves coverage gaps.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request corrects a pytest anti-pattern by moving the exception message matching out of the with pytest.raises context block in both async and sync streaming retry tests. The reviewer provided helpful feedback pointing out that the assert keyword is redundant when using exc_info.match(), and suggested a cleaner, more idiomatic approach of passing the match argument directly to pytest.raises.
| with pytest.raises(TypeError) as exc_info: | ||
| await generator.asend("cannot send to fresh generator") | ||
| assert exc_info.match("can't send non-None value") | ||
| assert exc_info.match("can't send non-None value") |
There was a problem hiding this comment.
The assert keyword is redundant here because exc_info.match() already performs the assertion internally and raises a failure if the pattern does not match.
Additionally, you can simplify this entire block by passing the match parameter directly to pytest.raises, which is the idiomatic way to assert both the exception type and message in pytest:
with pytest.raises(TypeError, match="can't send non-None value"):
await generator.asend("cannot send to fresh generator")| assert exc_info.match("can't send non-None value") | |
| exc_info.match("can't send non-None value") |
| # calling first send with non-None input should raise a TypeError | ||
| result.send("can not send to fresh generator") | ||
| assert exc_info.match("can't send non-None value") | ||
| assert exc_info.match("can't send non-None value") |
There was a problem hiding this comment.
The assert keyword is redundant here because exc_info.match() already performs the assertion internally and raises a failure if the pattern does not match.
Additionally, you can simplify this entire block by passing the match parameter directly to pytest.raises, which is the idiomatic way to assert both the exception type and message in pytest:
with pytest.raises(TypeError, match="can't send non-None value"):
# calling first send with non-None input should raise a TypeError
result.send("can not send to fresh generator")| assert exc_info.match("can't send non-None value") | |
| exc_info.match("can't send non-None value") |
#17723) ## Problem In `test_retry_streaming.py` and `test_retry_streaming_async.py`, an assert statement was indented inside the `pytest.raises` context manager block. In Python testing with `pytest`, any code in a pytest.raises block that follows line that raises the expected exception is NOT executed. This meant that the assert statement intended to verify the exception message was dead code and never ran. ## Solution The assert statement has been dedented (moved outside and after the `pytest.raises` block) to ensure it executes after the exception is caught. This allows the test to actually verify the exception message as originally intended. ## Notes to Reviewers This fix ensures that the tests are actually checking that the exception contains the message "can't send non-None value". It also helps with code coverage metrics by removing dead code.
2 participants
Problem
In
test_retry_streaming.pyandtest_retry_streaming_async.py, an assert statement was indented inside thepytest.raisescontext manager block. In Python testing withpytest, any code in a pytest.raises block that follows line that raises the expected exception is NOT executed.This meant that the assert statement intended to verify the exception message was dead code and never ran.
Solution
The assert statement has been dedented (moved outside and after the
pytest.raisesblock) to ensure it executes after the exception is caught. This allows the test to actually verify the exception message as originally intended.Notes to Reviewers
This fix ensures that the tests are actually checking that the exception contains the message "can't send non-None value". It also helps with code coverage metrics by removing dead code.