internal/testlog: fix log output from sub-loggers #31539
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I also would like to modify the example unit test that I copied from the issue to be table-driven, but there doesn't appear to be a way to capture the |
fjl
reviewed
Apr 2, 2025
…handler reference as the bufHandler in the newly-instantiated logger instead of copying from parent
…st-logger to take T as an interface to enable log output capture in the unit test.
jwasinger
force-pushed
the
testlog-fix-sublogger-output
branch
from
4b0e46f to
f464eb4
Compare
fjl
approved these changes
Apr 10, 2025
sivaratrisrinivas
pushed a commit
to sivaratrisrinivas/go-ethereum
that referenced
this pull request
Apr 21, 2025
When we instantiate a sub-logger via `go-ethereum/internal/testlog/logger.With`, we copy the reference to the `bufHandler` from the parent logger. However, internally, `go-ethereum/internal/testlog/logger.With` calls `log/slog/Logger.With` which creates a new handler instance (via `internal/bufHandler.WithAttrs`). This PR modifies sub-logger instantiation to use the newly-instantiated handler, instead of copying the reference from the parent instance. The type cast from `slog.Handler` to `*bufHandler` in `internal/testlog/Logger.With` is safe here because a `internal/testlog/Logger` can only be instantiated with a `*bufHandler` as the underlying handler type. Note, that I've also removed a pre-existing method that broke the above assumption. However, this method is not used in our codebase. I'm not sure if the assumption holds for forks of geth (e.g. optimism has modified the testlogger somewhat allowing test loggers to accept arbitrary handler types), but it seems okay to break API compatibility given that this is in the `internal` package. closes ethereum#31533
Rampex1
pushed a commit
to streamingfast/go-ethereum
that referenced
this pull request
May 15, 2025
When we instantiate a sub-logger via `go-ethereum/internal/testlog/logger.With`, we copy the reference to the `bufHandler` from the parent logger. However, internally, `go-ethereum/internal/testlog/logger.With` calls `log/slog/Logger.With` which creates a new handler instance (via `internal/bufHandler.WithAttrs`). This PR modifies sub-logger instantiation to use the newly-instantiated handler, instead of copying the reference from the parent instance. The type cast from `slog.Handler` to `*bufHandler` in `internal/testlog/Logger.With` is safe here because a `internal/testlog/Logger` can only be instantiated with a `*bufHandler` as the underlying handler type. Note, that I've also removed a pre-existing method that broke the above assumption. However, this method is not used in our codebase. I'm not sure if the assumption holds for forks of geth (e.g. optimism has modified the testlogger somewhat allowing test loggers to accept arbitrary handler types), but it seems okay to break API compatibility given that this is in the `internal` package. closes ethereum#31533
jakub-freebit
pushed a commit
to fblch/go-ethereum
that referenced
this pull request
Jul 3, 2025
When we instantiate a sub-logger via `go-ethereum/internal/testlog/logger.With`, we copy the reference to the `bufHandler` from the parent logger. However, internally, `go-ethereum/internal/testlog/logger.With` calls `log/slog/Logger.With` which creates a new handler instance (via `internal/bufHandler.WithAttrs`). This PR modifies sub-logger instantiation to use the newly-instantiated handler, instead of copying the reference from the parent instance. The type cast from `slog.Handler` to `*bufHandler` in `internal/testlog/Logger.With` is safe here because a `internal/testlog/Logger` can only be instantiated with a `*bufHandler` as the underlying handler type. Note, that I've also removed a pre-existing method that broke the above assumption. However, this method is not used in our codebase. I'm not sure if the assumption holds for forks of geth (e.g. optimism has modified the testlogger somewhat allowing test loggers to accept arbitrary handler types), but it seems okay to break API compatibility given that this is in the `internal` package. closes ethereum#31533
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we instantiate a sub-logger via
go-ethereum/internal/testlog/logger.With, we copy the reference to thebufHandlerfrom the parent logger. However, internally,go-ethereum/internal/testlog/logger.Withcallslog/slog/Logger.Withwhich creates a new handler instance (viainternal/bufHandler.WithAttrs).This PR modifies sub-logger instantiation to use the newly-instantiated handler, instead of copying the reference from the parent instance. The type cast from
slog.Handlerto*bufHandlerininternal/testlog/Logger.Withis safe here because ainternal/testlog/Loggercan only be instantiated with a*bufHandleras the underlying handler type.Note, that I've also removed a pre-existing method that broke the above assumption. However, this method is not used in our codebase.
I'm not sure if the assumption holds for forks of geth (e.g. optimism has modified the testlogger somewhat allowing test loggers to accept arbitrary handler types), but it seems okay to break API compatibility given that this is in the
internalpackage.closes #31533