-
-
Notifications
You must be signed in to change notification settings - Fork 362
Make assert_all_requests_are_fired always assert on exception
#782
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
base: master
Are you sure you want to change the base?
Conversation
When set to True, assertions about unfired requests will be raised even when an exception occurs in the context manager. This provides valuable debugging context about which mocked requests were or weren't called when debugging test failures. By default (assert_on_exception=False), the assertion is suppressed to avoid masking the original exception. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
assert_on_exception parameter to add context to test failures
| raise ValueError("Main error") | ||
|
|
||
| # The AssertionError should mention the unfired request | ||
| assert "not-called.com" in str(assert_exc_info.value) |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization
| m.add(responses.GET, "http://not-called.com", body=b"test") | ||
| requests.get("http://example.com") | ||
|
|
||
| assert "not-called.com" in str(assert_exc_info2.value) |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization
| test_with_assert_on_exception() | ||
|
|
||
| # The AssertionError should mention the unfired request | ||
| assert "not-called.com" in str(assert_exc_info.value) |
Check failure
Code scanning / CodeQL
Incomplete URL substring sanitization
responses/tests/test_responses.py
Outdated
| with pytest.raises(ValueError) as value_exc_info: | ||
| with responses.RequestsMock( | ||
| assert_all_requests_are_fired=True, assert_on_exception=False | ||
| ) as m: | ||
| m.add(responses.GET, "http://example.com", body=b"test") | ||
| m.add(responses.GET, "http://not-called.com", body=b"test") | ||
| requests.get("http://example.com") | ||
| raise ValueError("Main error") |
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.
It is arguable that this behaviour is a bug. And that assert_on_exception doesn't need to exist.
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.
The problem with changing this behaviour without an opt-in feature flag is that it'd be a breaking change, as the tests demonstrate (the exception leaving the RequestsMock block would change).
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 disagree that the exception type changing because the library has become more strict/correct constitutes a breaking change. Following that line of thinking would imply that no additional exception types could be added as they could 'break' compatibility.
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.
It's your library, happy to make this a flagless change if you prefer 🙂 I'll go ahead and update the PR accordingly.
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.
Done.
The @responses.activate decorator now accepts an assert_on_exception
parameter, providing a convenient way to enable assertion checking
even when exceptions occur:
@responses.activate(
assert_all_requests_are_fired=True,
assert_on_exception=True
)
def test_my_api():
...
This is consistent with the existing decorator support for
assert_all_requests_are_fired and registry parameters.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Document the new assert_on_exception parameter in version 0.26.0. This is a minor version bump (not patch) because we're adding new functionality to the public API, even though it's fully backward compatible with existing code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
7a49723 to
c27fb47
Compare
…ed=True This is a breaking change that modifies the behavior of assert_all_requests_are_fired to always raise assertions about unfired requests even when an exception occurs in the context manager or decorated function. Previously, when an exception occurred, assertions about unfired requests were suppressed to avoid masking the original exception. However, this behavior hid valuable debugging context about which mocked requests were or weren't called. The new behavior always raises assertions (when assert_all_requests_are_fired=True), with the original exception chained as context. This provides developers with complete information about both the original failure and the state of mocked requests. Changes: - Updated __exit__ to always pass allow_assert=True to stop() - Removed conditional logic that suppressed assertions on exception - Updated tests to verify assertions are raised during exceptions - Updated documentation to reflect new behavior 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
assert_on_exception parameter to add context to test failuresassert_on_exception and make assert_all_requests_are_fired always assert on exception
assert_on_exception and make assert_all_requests_are_fired always assert on exceptionassert_all_requests_are_fired always assert on exception
What type of PR is this? (check all applicable)
Description
This PR modifies the behavior of
assert_all_requests_are_firedto always raise assertions about unfired requests, even when an exception occurs in the context manager or decorated function. This is a breaking change that improves debugging capabilities by providing complete context about both the original failure and the state of mocked requests.Background
Previously, when a test failed within the scope of
RequestsMock(e.g. an exception was raised or an assertion failed),responseswould skip asserting that all requests were fired to avoid masking the original exception. This could hide useful debugging information, as the test failure may have been a side effect of an expected request not being called (e.g. the mocked URL was wrong).An earlier iteration of this PR introduced an
assert_on_exceptionparameter to make this behavior configurable. However, after further consideration, the new behavior is so valuable for debugging that it should be the default and only behavior.Key Changes
assert_all_requests_are_fired=True, assertions about unfired requests are now always raised, even when an exception occursassert_on_exceptionparameter - the improved behavior is now automaticException Handling Example
Before (old behavior):
After (new behavior):
Benefits
Important
This is a breaking change. Code that previously relied on unfired request assertions being suppressed during exceptions will now see those assertions raised. However, this change improves debugging capabilities significantly.
Related Issues
PR checklist
Before submitting this pull request, I have done the following:
toxandpre-commitchecks locallyAdded/updated tests?
Test Coverage: Comprehensive tests updated to verify: