Make logger usage optional #3967
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Great start and great PR, looks good! 🤩
I've addressed some nits with the console.log output (we should not do in this case).
Can you eventually directly do a follow up PR where we get rid now of these ugly logger messages in the client integration tests (so: npm run test:integration)? That would be super great! 🤩 We actually wanted to do this in the first place when we added these tests long ago, but weren't able to do so since the logger was not optional yet.
Didn't have such a close look, but I would guess that these chain.config.logger!.silent = true lines might now also be able to replaced? Core thing though is that the logger for most tests is just generally turned off.
| for (const transport of (replServer.context.client as EthereumClient).config.logger | ||
| const logger = (replServer.context.client as EthereumClient).config.logger | ||
| if (logger === undefined) { | ||
| this.displayPrompt() |
There was a problem hiding this comment.
Some nit: I understand the intention, but if the logger is turned off, we should not use the (a) logger at all, not even the console.log() logger and if it's only for informing the user. Have removed and directly pushed.
packages/client/src/logging.ts
Outdated
| args: { [key: string]: any } = { logLevel: 'info' }, | ||
| ): WinstonLogger | undefined { | ||
| if (args.logLevel === 'off') { | ||
| console.log('logger turned off') |
There was a problem hiding this comment.
Also here, will temporarily deactivate the associated test case.
acolytec3
force-pushed
the
make-logger-optional
branch
2 times, most recently
from
cad20ce to
a4e409f
Compare
jochem-brouwer
force-pushed
the
make-logger-optional
branch
from
21c244d to
a88a082
Compare
| } | ||
| } | ||
| await clientRunHelper(cliArgs, onData) | ||
| }, 30000)*/ |
There was a problem hiding this comment.
Note: this is a newly added test, but this is commented out. The reason is that the tests in this file test stuff by by listening to whatever the output (what is being logged 😅 ) is. Since this test expects no output, we thus cannot test this, this way.
As a general comment: the way to test these things (like in this file) by listening to comments (and also listening to hardcoded strings, not strings which we read from some file, is not the way to test it. If we change a logging message, we could break a test here because the test expects a certain string.
I think the way to test the CLI like this, and also how the CLI is set up to be tested, is not good. I will open an issue for this.
There was a problem hiding this comment.
I created this issue: #3983
This change makes the logger optional in the client package by using the optional chaining operator and also introduces a new option for the cli
logLeveloption to be set tooffif the logger is to be turned off: