Skip to content

Conversation

@mje-nz
Copy link
Contributor

@mje-nz mje-nz commented Dec 12, 2013

At the moment, if you set the log level to TRACE, loadPersistedLevel() will set the log level back to WARN (since self.levels[storedLevel] will be 0 which is falsey). This fixes that in at least Chrome, I'm not sure where else to test it.

@pimterry
Copy link
Owner

Fix looks fine itself, but the tests don't pass; could you take a look at that? If you've got node + npm installed, you should be able to go:

npm install
grunt test

and have it run the lot of them headlessly (which should then fail, as in travis). If you run grunt jasmine:src:build I think it'll then build you an actual HTML file as _SpecRunner.html in the root directory, which you can open in a browser of your choice to run them interactively there.

It'd be good to also add a new test to cover this case directly too.

@mje-nz
Copy link
Contributor Author

mje-nz commented Dec 12, 2013

Ah, silly me! This fixes the failing tests and adds new tests covering my change.

One thing that's a bit unhelpful is that when an error is thrown while loading the persisted log level, it ends up going through the requirejs error handler, which produces an incredibly unhelpful error message and stack trace:

>> Error: undefined: undefined at
>> _SpecRunner.html:21 
>> .grunt/grunt-contrib-jasmine/require.js:12 v
>> .grunt/grunt-contrib-jasmine/require.js:19 
>> .grunt/grunt-contrib-jasmine/require.js:23 
>> .grunt/grunt-contrib-jasmine/require.js:17 
>> .grunt/grunt-contrib-jasmine/require.js:14 D
>> .grunt/grunt-contrib-jasmine/require.js:28 
>> .grunt/grunt-contrib-jasmine/require.js:29 

pimterry added a commit that referenced this pull request Dec 13, 2013
Fix bug where log level is reset to default if it's stored as TRACE
@pimterry pimterry merged commit 150af9e into pimterry:master Dec 13, 2013
@pimterry
Copy link
Owner

Good stuff, merged, thanks!

Yeah, that's pretty bad as errors go. Looks like that's from whichever Jasmine/RequireJS template I'm using, I forget now, but I might take a look at fixing that over there pretty soon.

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.

2 participants