-
Notifications
You must be signed in to change notification settings - Fork 4
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
ref: add postChunk integration test #480
Conversation
Check necessary since sentry.GetHubFromContext can return a nil value
@@ -29,7 +29,9 @@ func (env *environment) postChunk(w http.ResponseWriter, r *http.Request) { | |||
body, err := io.ReadAll(r.Body) | |||
s.Finish() | |||
if err != nil { | |||
hub.CaptureException(err) | |||
if hub != nil { |
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 suppose you're doing this to not initialize Sentry in tests? Can we instead initialize Sentry in tests and just drop every event?
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.
@phacops if we want to leave things as they are (without checking the hub
) it's even easier than initializing sentry
and then dropping the events. We can just inject a hub
in the request context (in the test) and it'll work just fine. Like this:
ctx := req.Context()
hub := sentry.NewHub(&sentry.Client{}, sentry.NewScope())
ctx = context.WithValue(ctx, sentry.HubContextKey, hub)
req = req.WithContext(ctx)
The reason why I've added the check is that I've noticed that GetHubFromContext
can return a nil
.
Now, since we're using (and wrapping our router with) the sentryhttp
integration that should never happen as we should always get a hub in the request, but I think it's still good practice to make a nil check.
#skip-changelog