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.

Sign up for GitHub

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

Jump to bottom

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
Go1.21

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

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
@jba @bradfitz
testing/slogtest: check for no group with empty record
ca49c1a 
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
@jba @eric
testing/slogtest: check for no group with empty record
ad14e20 
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
@jba @eric
testing/slogtest: check for no group with empty record
9f38073 
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
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
5 participants
@dmitshur @gopherbot @seankhliao @jba @heschi