Updated dependencies. Moved mocha options to package.json #75
Conversation
There was a problem hiding this comment.
Thanks for the PR @ravihara,
Looks good. I'll merge tonight, probably with a couple of minor changes as discussed in the comments
| options: { | ||
| heartbeat: 50, | ||
| }, | ||
| }, | ||
| namespace: true, |
There was a problem hiding this comment.
Not sure why this caused tests to fail. Rascal defaults the heartbeat to 10 which is quite a long time in seconds. However it could be that some of the load or flow control tests are filling the event loop, meaning that the broker doesn't get a response. If that's the problem then this was a really good find. Wouldn't even has crossed my mind.
There was a problem hiding this comment.
Yep! That could be the most obvious reason. It was failing randomly at various tests.. sometimes in Publications sometimes in Subscriptions.
| const size = _.get(options, 'size') || 1000; | ||
| const cache = new LRUCache({ | ||
| max: size, | ||
| }); |
There was a problem hiding this comment.
I'd change these back to vars, although I agree const would be better. The rascal code base is still ES5, even though it relies on modules that use const so there's no problem changing these. I'd rather keep the code base consistent for now and update the whole codebase at some point.
|
|
||
| return { | ||
| incrementAndGet: function(key, next) { | ||
| incrementAndGet: function (key, next) { |
There was a problem hiding this comment.
The in-house style is no space between in function (. I'll update the linter to fix this automatically from now on.
| @@ -1,6 +1,6 @@ | |||
| { | |||
| "name": "rascal", | |||
| "version": "7.0.0", | |||
| "version": "7.0.1", | |||
There was a problem hiding this comment.
I'll bump this using npm version [patch|minor|major] so I get a tag. Not sure whether this is a patch though, since some of the deps have major version bumps. Will need to investigate a bit more closely.
There was a problem hiding this comment.
As there was no major changes to features or new feature additions, I thought of bumping up only the patch part. Using an automated version bumper sound better too.
There was a problem hiding this comment.
One of the bumps caused nodee v6 tests to fail (due to use of async). I've removed support for this but will bump to rascal 8.0.0
There was a problem hiding this comment.
Right. I did observe this from circle.ci logs. I guess, it should be fine to remove Node 6.x support at this time :-).
| "mocha": { | ||
| "timeout": 5000, | ||
| "checkLeaks": true, | ||
| "bail": true |
There was a problem hiding this comment.
Is there an advantage moving mocha opts to within package.json? I notice you dropped the recursive flag. Is this now a default?
There was a problem hiding this comment.
mocha.opts is deprecated now. There are couple of other alternatives suggested but, I felt keeping it in package.json (which is one of those recommended options) makes it more simpler. I didn't find recursive flag among the set of options in their documentation. Better to recheck once and confirm.
There was a problem hiding this comment.
Thanks. Didn't know mocha.opts was deprecated. Will go with the package.json config
Updated all the dependencies using npm-check.
Created new instance of LRUCache in lib/counters/inMemory.js.
Added connection option in lib/config/test.js to setup higher heartbeats since, Subscription tests were failing because of heart beat timeout.
Bumped up package version to 7.0.1