-
Notifications
You must be signed in to change notification settings - Fork 514
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
fix(django): Add sync_capable
to SentryWrappingMiddleware
#3510
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #3510 +/- ##
==========================================
- Coverage 84.26% 84.26% -0.01%
==========================================
Files 136 136
Lines 13894 13895 +1
Branches 2938 2938
==========================================
Hits 11708 11708
Misses 1458 1458
- Partials 728 729 +1
|
64f8f12
to
11698e4
Compare
@sentrivana or @antonpirker, do you think we need a test for this? I am not sure how exactly we would test this change |
11698e4
to
1f19214
Compare
Nice!
Do we already have any middleware tests? It'd be great to have a test case that sets up a middleware, then inits the SDK, and then checks whether the wrapped middleware has all the expected attributes, including If there's already a similar test case, we can just add this to it/repurpose that. If we don't have this, that's a blind spot we should fix. But if it's too much effort, you can just create a ticket and eventually we'll get to it during maintenance. |
@sentrivana @antonpirker, I added a test. Would appreciate if one of you could check that it looks reasonable |
Apparently, not having the
sync_capable
on theSentryWrappingMiddleware
sometimes breaks things when we have an async-only middleware, such as ServeStatic.Fixes #3506