Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

log4js.configure({}) resets all loggers' levels to TRACE #70

Merged

Conversation

mab-work
Copy link
Contributor

@mab-work mab-work commented May 2, 2012

This is seems inconsistent, given that log4js.configure({levels:{}}) preserves any previously set levels.
https://gist.github.com/2577583

This fixes that, by ensuring the setLevel(undefined || null) does not over-write a previously set level with TRACE.

Since the only time setLevel() is called within log4js.js with no parameters is when configure is called without the configuration object containing a 'levels' property, this seems like a reasonable and simple fix.

… to TRACE, even if they were previously set to something else
@nomiddlename
Copy link
Collaborator

Would you mind adding some tests to cover this, please?

…ls' property is passed in the configuration
@mab-work
Copy link
Contributor Author

mab-work commented May 8, 2012

Done

nomiddlename pushed a commit that referenced this pull request May 8, 2012
log4js.configure({}) resets all loggers' levels to TRACE
@nomiddlename nomiddlename merged commit 2169376 into log4js-node:master May 8, 2012
@nomiddlename
Copy link
Collaborator

Thank you very much.

@nomiddlename
Copy link
Collaborator

Tests fail for me in node 0.6.15, example below:

Setting up loggers with initial levels, then setting a configuration which has null levels, and checking the logger whose level was set to Warn with isLevelEnabled(Debug)
✗ should return false
TypeError: Object Error: Problem reading log4js config { levels: null }. Error was "Cannot call method 'toString' of undefined" (TypeError: Cannot call method 'toString' of undefined
at Object.toLevel (/Users/garethjones/log4js-node/lib/levels.js:29:23)
at Logger.setLevel (/Users/garethjones/log4js-node/lib/log4js.js:204:25)
at configureLevels (/Users/garethjones/log4js-node/lib/log4js.js:164:28)
at configureOnceOff (/Users/garethjones/log4js-node/lib/log4js.js:281:13)
at Object.configure (/Users/garethjones/log4js-node/lib/log4js.js:357:5)
at Object. (/Users/garethjones/log4js-node/test/test-configureNoLevels.js:56:16)
at run (/Users/garethjones/log4js-node/node_modules/vows/lib/vows/suite.js:134:35)
at EventEmitter. (/Users/garethjones/log4js-node/node_modules/vows/lib/vows/suite.js:233:40)
at EventEmitter. (events.js:88:20)
at EventEmitter.emit (/Users/garethjones/log4js-node/node_modules/vows/lib/vows.js:236:24)) has no method 'getLogger'
at Object. (/Users/garethjones/log4js-node/test/test-configureNoLevels.js:107:35)
at runTest (/Users/garethjones/log4js-node/node_modules/vows/lib/vows.js:132:26)
at EventEmitter. (/Users/garethjones/log4js-node/node_modules/vows/lib/vows.js:78:9)
at EventEmitter. (events.js:88:20)
at EventEmitter.emit (/Users/garethjones/log4js-node/node_modules/vows/lib/vows.js:236:24)
at Array.19 (/Users/garethjones/log4js-node/node_modules/vows/lib/vows/suite.js:168:45)
at EventEmitter._tickCallback (node.js:192:40)

  ✗ finally checking for comparison mismatch with log4js 
  TypeError: Cannot call method 'toLevel' of undefined 
  at Object.<anonymous> (/Users/garethjones/log4js-node/test/test-configureNoLevels.js:113:38) 
  at runTest (/Users/garethjones/log4js-node/node_modules/vows/lib/vows.js:132:26) 
  at EventEmitter.<anonymous> (/Users/garethjones/log4js-node/node_modules/vows/lib/vows.js:78:9) 
  at EventEmitter.<anonymous> (events.js:88:20) 
  at EventEmitter.emit (/Users/garethjones/log4js-node/node_modules/vows/lib/vows.js:236:24) 
  at Array.19 (/Users/garethjones/log4js-node/node_modules/vows/lib/vows/suite.js:168:45) 
  at EventEmitter._tickCallback (node.js:192:40) 

@nomiddlename
Copy link
Collaborator

Never mind - I fixed it :)

@mab-work
Copy link
Contributor Author

mab-work commented May 9, 2012

What was the problem? I had some similar issues originally, which disappeared after I hosed everything and went to a clean node v0.6.7, vows 0.6.2, and HEAD of log4js-node/master

@nomiddlename
Copy link
Collaborator

Here's the commit: 613a077

@mab-work
Copy link
Contributor Author

mab-work commented May 9, 2012

OK, since that's the only place setLevel() is called with no params, the previous change to line 204 of log4js.js probably isn't needed now. Still, if it works it works ;)

@nomiddlename
Copy link
Collaborator

It's still failing on travis, but I think that's an npm problem for me to investigate. http://travis-ci.org/#!/nomiddlename/log4js-node

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants