Skip to content
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

testing/slogtest: add check for empty group [freeze exception] #61227

Closed
jba opened this issue Jul 7, 2023 · 2 comments
Closed

testing/slogtest: add check for empty group [freeze exception] #61227

jba opened this issue Jul 7, 2023 · 2 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@jba
Copy link
Contributor

jba commented Jul 7, 2023

https://go.dev/issue/61067 identifies a bug in log/slog. CL https://go.dev/cl/508436 fixes it and so does not require a freeze exception.

However, any user-written handler could exhibit the same bug. The testing/slogtest package is designed to catch errors like this in Handlers. It should be updated to check for the problem. https://go.dev/cl/508438 does that.

Since this isn't technically a bug fix, I'm applying for an exception.

/cc @golang/release

@seankhliao seankhliao added this to the Go1.21 milestone Jul 7, 2023
@seankhliao seankhliao added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Jul 7, 2023
@heschi
Copy link
Contributor

heschi commented Jul 7, 2023

Approved.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/508438 mentions this issue: testing/slogtest: check for no group with empty record

@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Jul 12, 2023
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
As golang#61067 pointed out, slog did not properly handle empty groups.
https://go.dev/cl/508436 dealt with most cases inside slog itself,
but handlers must still do a check on their own. Namely, a handler
must not output a group created by WithGroup unless the Record
has attributes.

This change adds a test to slogtest to check that case.

Fixes golang#61227.

Change-Id: Ibc065b6e5f6e199a41bce8332ea8c7f9d8373392
Reviewed-on: https://go-review.googlesource.com/c/go/+/508438
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
As golang#61067 pointed out, slog did not properly handle empty groups.
https://go.dev/cl/508436 dealt with most cases inside slog itself,
but handlers must still do a check on their own. Namely, a handler
must not output a group created by WithGroup unless the Record
has attributes.

This change adds a test to slogtest to check that case.

Fixes golang#61227.

Change-Id: Ibc065b6e5f6e199a41bce8332ea8c7f9d8373392
Reviewed-on: https://go-review.googlesource.com/c/go/+/508438
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
eric pushed a commit to fancybits/go that referenced this issue Sep 7, 2023
As golang#61067 pointed out, slog did not properly handle empty groups.
https://go.dev/cl/508436 dealt with most cases inside slog itself,
but handlers must still do a check on their own. Namely, a handler
must not output a group created by WithGroup unless the Record
has attributes.

This change adds a test to slogtest to check that case.

Fixes golang#61227.

Change-Id: Ibc065b6e5f6e199a41bce8332ea8c7f9d8373392
Reviewed-on: https://go-review.googlesource.com/c/go/+/508438
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Jonathan Amsterdam <jba@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants