Skip to content
This repository has been archived by the owner on Nov 8, 2024. It is now read-only.

refactor: upgrades "winston" package to v3 #1293

Closed
wants to merge 1 commit into from
Closed

refactor: upgrades "winston" package to v3 #1293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions lib/logger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,19 @@
const winston = require('winston');
const { createLogger, transports, format } = require('winston');

module.exports = new (winston.Logger)({
module.exports = new createLogger({
transports: [
new (winston.transports.Console)({ colorize: true }),
new transports.Console({ colorize: true }),
],
levels: {
debug: 2,
warn: 1,
error: 0,
},
colors: {
debug: 'cyan',
warn: 'yellow',
error: 'red',
},
format: format.colorize({
colors: {
debug: 'cyan',
warn: 'yellow',
error: 'red',
},
}),
});
34 changes: 18 additions & 16 deletions lib/reporters/reporterOutputLogger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
const winston = require('winston');
const { createLogger, transports, format } = require('winston');

module.exports = new (winston.Logger)({
module.exports = new createLogger({
transports: [
new (winston.transports.Console)({ colorize: true, level: 'info' }),
new transports.Console({ colorize: true, level: 'info' }),
],
levels: {
info: 10,
Expand All @@ -17,17 +17,19 @@ module.exports = new (winston.Logger)({
skip: 1,
error: 0,
},
colors: {
info: 'blue',
test: 'yellow',
pass: 'green',
fail: 'red',
complete: 'green',
actual: 'red',
expected: 'red',
hook: 'green',
request: 'green',
skip: 'yellow',
error: 'red',
},
format: format.colorize({
colors: {
info: 'blue',
test: 'yellow',
pass: 'green',
fail: 'red',
complete: 'green',
actual: 'red',
expected: 'red',
hook: 'green',
request: 'green',
skip: 'yellow',
error: 'red',
},
}),
});
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
"untildify": "3.0.3",
"uuid": "3.3.2",
"which": "1.3.1",
"winston": "2.4.0"
"winston": "3.2.1"
},
"devDependencies": {
"@commitlint/cli": "7.5.2",
Expand Down
12 changes: 8 additions & 4 deletions test/unit/reporters/CLIReporter-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,17 @@ describe('CLIReporter', () => {
let test = {};

before(() => {
loggerStub.transports.console.silent = true;
reporterOutputLoggerStub.transports.console.silent = true;
[loggerStub, reporterOutputLoggerStub].forEach((logger) => {
logger.configure({ silent: true });
Copy link
Contributor Author

@kettanaito kettanaito Mar 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm particularly insterested in how to verify this silent toggling.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

})
});

after(() => {
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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

[loggerStub, reporterOutputLoggerStub].forEach((logger) => {
logger.configure({ silent: false });
})
});

describe('when starting', () => {
Expand Down