cmd/geth, internal/debug: get rid of by-default log config #28801
Conversation
| @@ -51,9 +50,6 @@ func (c customQuotedStringer) String() string { | |||
| // logTest is an entry point which spits out some logs. This is used by testing | |||
| // to verify expected outputs | |||
| func logTest(ctx *cli.Context) error { | |||
| // clear field padding map | |||
| debug.ResetLogging() | |||
There was a problem hiding this comment.
We had to add this to have the log tests deterministic. Curious if it works without it
There was a problem hiding this comment.
That's why I was asking how this thing is used? The only was I saw was running geth logtest, in which case there's nothing logging that would break it.
There was a problem hiding this comment.
There's no documentation on how to use this command, so I have no idea if I broke something or not.
There was a problem hiding this comment.
The tests will tell you if you did
There was a problem hiding this comment.
go test ./cmd/geth -tags integrationtests -run TestLogging
There was a problem hiding this comment.
CI seems happy about it though :)
|
It's a bit strange to have a geth command that tests some log output. We could also just have this as a unit test or something. |
We don't, it is a command which spits out a predefined set of logs.
We do ( |
|
Yeah I get it, but why is this a cmd/geth test and not a package log test. |
|
Because it's testing the geth logging, it is not log-package specific. |
|
Ahh OK! |
|
The flags to geth will directly translate into Looking at this test, I just really think this test should always run, not just manually. It's not great to have a larget bit of code that doesn't compile/run by default in CI. |
|
Yes, but not without loss of test-coverage. So if we screw up geth logging output, or ability spit log output to file, we won't notice it. I added these tests which test the binary in order to ensure that we detect if we bork geth. I think it's a LOT better than a package-level test.
We run it on CI |
Initializing a default logger on package init is bad practice, because anything referencing the package will have this side effect. This specifically came up with the new simulated backend, which uses node/api, which pulls in the debug namespace, initing the logger. That's bad, as everything gets very verbose this way.
This PR gets rid of most of the initialization. It just leaves in initing the glogger value, so if someone uses the api handler without calling Setup, they don't end up with nil exceptions.