Unit tests for node/getStatus function - Closes #2312 #2327
Conversation
test/unit/modules/node.js
Outdated
| done(); | ||
| }); | ||
|
|
||
| it('should call modules.peers.networkHeight and call callback with result containing networkHeight = network.height', done => { |
There was a problem hiding this comment.
Looks like 2 separate tests for me - "call callback" and "call modules.peers.networkHeight"
test/unit/modules/node.js
Outdated
| }); | ||
| }); | ||
|
|
||
| it('should call callback with result containing networkHeight = null if modules.peers.networkHeight returns networkHeight as null', done => { |
There was a problem hiding this comment.
"if modules.peers.networkHeight returns networkHeight as null" condition can be extracted into describe block
| ); | ||
| describe('modules.loader.getNetwork', () => { | ||
| beforeEach(done => { | ||
| stubs.networkHeight = sinonSandbox |
There was a problem hiding this comment.
Does this stub get restored after test?
There was a problem hiding this comment.
@SargeKhan Not at the moment. I just made sure in beforeEach hooks they are being reassigned. Actually i was hoping to get a direction on this. This discussion #1417 suggests to use a _global_hooks.js file but it doesn't exist yet and I followed the structure in test/unit/modules/multisignatures.js since it's written recently by @4miners.
So please feel free to guide me here.
There was a problem hiding this comment.
I think it's always better to restore spy/stubs in afterEach if they were created in a beforeEach block.
| @@ -169,7 +169,7 @@ Node.prototype.shared = { | |||
| modules.peers.networkHeight({ normalized: false }, (err, networkHeight) => { | |||
| setImmediate(cb, null, { | |||
| broadhash: modules.system.getBroadhash(), | |||
| consensus: modules.peers.getLastConsensus() || null, | |||
| consensus: modules.peers.getLastConsensus(), | |||
There was a problem hiding this comment.
Did you find the reason as to why || null was there earlier?
There was a problem hiding this comment.
@SargeKhan No, actually consensus property is used here https://github.com/LiskHQ/lisk/blob/2312-unit-tests-for-node-getStatus/api/controllers/node.js#L101 in a weird way because if it's null we are converting it back to 0. So || null was totally useless in my opinion. If you think it's better to fix it in a separate issue I can create one for that.
There was a problem hiding this comment.
I was just asking. I think modules.peers.getLastConsensus() might return undefined. And it makes sense to set the default values in the controller as opposed to modules.
There was a problem hiding this comment.
I could find only to places that we assign it's value. here: https://github.com/LiskHQ/lisk/blob/2312-unit-tests-for-node-getStatus/modules/peers.js#L70 and here https://github.com/LiskHQ/lisk/blob/2312-unit-tests-for-node-getStatus/modules/peers.js#L475. Seems like it always returns an integer?
There was a problem hiding this comment.
You are right! We don't need that check. I thought maybe on application start getLastConsensus may have undefined value.
Review checklist