refactor: upgrades "winston" package to v3 #1293
Conversation
|
Hi, @honzajavorek. Could you please assist me in verifying these changes retain the previous behavior of both internal and external (repoter) loggers? I've used |
| loggerStub.transports.console.silent = true; | ||
| reporterOutputLoggerStub.transports.console.silent = true; | ||
| [loggerStub, reporterOutputLoggerStub].forEach((logger) => { | ||
| logger.configure({ silent: true }); |
There was a problem hiding this comment.
I'm particularly insterested in how to verify this silent toggling.
There was a problem hiding this comment.
Short: I think it is in the test only to prevent the logger to print to the terminal during the tests. I wouldn't be bothered about it in this phase. If the --loglevel=silent option works correctly for Dredd (important), feel free to omit the silencing and leave the logging chatty in tests (nice to have).
Long: What I'm trying to achieve is to use winston for Dredd's "application logging" (roughly equal to stderr), and to replace winston completely for the reporters and any other "application output" (roughly equal to stdout). Also, I'd like the logger or the "output rendering component" (not in place yet) to be testable in better ways than it is now. Currently, magic with proxyquire() and sinon.stub()-s is necessary every time the tests want to verify what was displayed to the user.
The refactoring is hairy and complex, and especially gradually updating the tests to be able to test the same stuff even after changes to the logging infrastructure is tedious. For that reason, I consider having the logger silent during the tests as something nice to have at this moment, and I omitted it many times in my previous PRs. The result is that the output of npm test is a bit messy now, but I count with the fact this gets figured out once we have a better way to test the logger (like dependency injection).
|
Thanks! I'll take a look tomorrow 👍 |
There was a problem hiding this comment.
I made two comments. Let me know if that answers your questions, I'm happy to help.
On a related note, with @dariagrudzien we recently worked on upgrading Winston in one private library we have in @apiaryio and it seems the v3 doesn't emit the logging event anymore when the logger logs something. It is a shame, as that was one of the nicer ways how to inspect the logger in tests without any sinon-ing around. I'm afraid it is used in Dredd's tests as well, so watch out for that during the upgrade.
| loggerStub.transports.console.silent = true; | ||
| reporterOutputLoggerStub.transports.console.silent = true; | ||
| [loggerStub, reporterOutputLoggerStub].forEach((logger) => { | ||
| logger.configure({ silent: true }); |
There was a problem hiding this comment.
Short: I think it is in the test only to prevent the logger to print to the terminal during the tests. I wouldn't be bothered about it in this phase. If the --loglevel=silent option works correctly for Dredd (important), feel free to omit the silencing and leave the logging chatty in tests (nice to have).
Long: What I'm trying to achieve is to use winston for Dredd's "application logging" (roughly equal to stderr), and to replace winston completely for the reporters and any other "application output" (roughly equal to stdout). Also, I'd like the logger or the "output rendering component" (not in place yet) to be testable in better ways than it is now. Currently, magic with proxyquire() and sinon.stub()-s is necessary every time the tests want to verify what was displayed to the user.
The refactoring is hairy and complex, and especially gradually updating the tests to be able to test the same stuff even after changes to the logging infrastructure is tedious. For that reason, I consider having the logger silent during the tests as something nice to have at this moment, and I omitted it many times in my previous PRs. The result is that the output of npm test is a bit messy now, but I count with the fact this gets figured out once we have a better way to test the logger (like dependency injection).
| loggerStub.transports.console.silent = false; | ||
| reporterOutputLoggerStub.transports.console.silent = false; | ||
| // Is this really a good idea to mutate logger instances | ||
| // vs creating logger instances for tests using the same factory function? |
There was a problem hiding this comment.
Is this really a good idea to mutate logger instances
Nope 😄 But as per my previous comment, currently the logging isn't done in a way it would be easy to test. It is planned, but if you want to keep this PR scoped to Winston upgrade, you should restrain yourself from trying to solve this problem as well. We can (should!) do it as one of the next steps.
We can limit the mutations though by ommitting silencing of the logging in tests, which is not for the purpose of the test itself, but only for the purpose of not having the npm test output cluttered (like in this case).
There was a problem hiding this comment.
Thanks for clarifying. I will remove this mutation and see the test output.
Let's keep this pull request scoped to prevent the complexity of the changes. We can refactor the usage of logging later, I'd be glad to.
|
Thanks for the support, Honzo. I don't have the connection at the moment, so we can continue on this upgrade once I'm in the office. Hope that's fine. |
|
Closed in favor of #1303. |
🚀 Why this change?
Upgrades
winstonpackage to version 3. Follows the migration guides to adjust existing usage for breaking API changes of thewinstonpackage.winston.createLoggerinstead ofwinston.Loggercolorsfield from the logger configformatfield withformat.colorize()for logs' colors (retains the previously set colors mapping)📝 Related issues and Pull Requests
✅ What didn't I forget?
npm run lint