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.

Sign up for GitHub

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

Jump to bottom

Unit tests for node/getStatus function - Closes #2312 #2327

Merged
merged 4 commits into from Aug 25, 2018
Merged

Unit tests for node/getStatus function - Closes #2312 #2327

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f1e5697
:bug: Remove null from Short-circuit Evaluation for consensus
yatki Aug 22, 2018
17c7b1b
:white_check_mark: Add tests for node/getStatus function
yatki Aug 22, 2018
fb51799
:white_check_mark: Split & reorganize modules.loader.getNetwork tests
yatki Aug 23, 2018
3989910
:white_check_mark: Add sinonSandbox.restore to afterEach blocks
yatki Aug 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion modules/node.js
View file
Open in desktop
Expand Up @@ -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(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you find the reason as to why || null was there earlier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right! We don't need that check. I thought maybe on application start getLastConsensus may have undefined value.

currentTime: Date.now(),
secondsSinceEpoch: slots.getTime(),
height: modules.blocks.lastBlock.get().height,
Expand Down
147 changes: 124 additions & 23 deletions test/unit/modules/node.js
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@ describe('node', () => {
var testDelegate = genesisDelegates.delegates[0];
var defaultPassword;
var library;
const stubs = {};

before(done => {
application.init(
Expand Down Expand Up @@ -263,6 +264,13 @@ describe('node', () => {
});

describe('shared', () => {
let node_module;

beforeEach(done => {
node_module = library.modules.node;
done();
});

describe('getConstants', () => {
describe('when loaded = false', () => {
it('should call callback with error = "Blockchain is loading"');
Expand Down Expand Up @@ -313,40 +321,133 @@ describe('node', () => {

describe('getStatus', () => {
describe('when loaded = false', () => {
it('should call callback with error = "Blockchain is loading"');
before(done => {
library.rewiredModules.node.__set__('loaded', false);
done();
});

it('should call callback with error = "Blockchain is loading"', done => {
node_module.shared.getStatus(null, err => {
expect(err).to.equal('Blockchain is loading');
done();
});
});

after(done => {
library.rewiredModules.node.__set__('loaded', true);
done();
});
});

describe('when loaded = true', () => {
it('should call callback with error = null');
it('should call callback with error = null', done => {
node_module.shared.getStatus(null, err => {
expect(err).to.not.exist;
done();
});
});

it(
'should call callback with result containing broadhash = modules.system.getBroadhash result'
);
it('should return status object with properties', done => {
node_module.shared.getStatus(null, (err, status) => {
const properties = [
'broadhash',
'consensus',
'currentTime',
'secondsSinceEpoch',
'height',
'loaded',
'networkHeight',
'syncing',
];
properties.forEach(property => {
expect(status).to.have.property(property);
});
expect(Object.keys(status).length).to.equal(properties.length);
done();
});
});

it(
'should call callback with result containing consensus = modules.peers.calculateConsensus result'
);
it('should call callback with result containing broadhash = modules.system.getBroadhash() result', done => {
node_module.shared.getStatus(null, (err, status) => {
expect(status.broadhash).to.equal(
library.modules.system.getBroadhash()
);
done();
});
});

it(
'should call callback with result containing height = modules.blocks.lastBlock.get result'
);
it('should call callback with result containing consensus = modules.peers.getLastConsensus() result', done => {
node_module.shared.getStatus(null, (err, status) => {
expect(status.consensus).to.equal(
library.modules.peers.getLastConsensus()
);
done();
});
});

it(
'should call callback with result containing syncing = modules.loader.syncing result'
);
it('should call callback with result containing height = modules.blocks.lastBlock.get().height result', done => {
node_module.shared.getStatus(null, (err, status) => {
expect(status.height).to.equal(
library.modules.blocks.lastBlock.get().height
);
done();
});
});

it('should call modules.loader.getNetwork');
it('should call callback with result containing syncing = modules.loader.syncing() result', done => {
node_module.shared.getStatus(null, (err, status) => {
expect(status.syncing).to.equal(library.modules.loader.syncing());
done();
});
});

describe('when modules.loader.getNetwork fails', () => {
it(
'should call callback with result containing networkHeight = null'
);
it('should call callback with result containing loaded = modules.loader.loaded() result', done => {
node_module.shared.getStatus(null, (err, status) => {
expect(status.loaded).to.equal(library.modules.loader.loaded());
done();
});
});

describe('when modules.loader.getNetwork succeeds and returns network', () => {
it(
'should call callback with result containing networkHeight = network.height'
);
describe('modules.loader.getNetwork', () => {
beforeEach(done => {
stubs.networkHeight = sinonSandbox
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this stub get restored after test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's always better to restore spy/stubs in afterEach if they were created in a beforeEach block.

.stub()
.callsFake((filters, cb) => cb(null, 123));
const modules = library.rewiredModules.node.__get__('modules');
modules.peers.networkHeight = stubs.networkHeight;
done();
});

it('should call modules.peers.networkHeight', done => {
node_module.shared.getStatus(null, () => {
expect(library.modules.peers.networkHeight).to.have.been.called;
done();
});
});

it('should call callback with result containing networkHeight = network.height', done => {
node_module.shared.getStatus(null, (err, status) => {
expect(status.networkHeight).to.equal(123);
done();
});
});

describe('if modules.peers.networkHeight returns networkHeight as null', () => {
it('should call callback with result containing networkHeight = null', done => {
library.modules.peers.networkHeight = sinonSandbox
.stub()
.callsFake((filters, cb) => cb(null, null));
node_module.shared.getStatus(null, (err, status) => {
expect(status.networkHeight).to.equal(null);
done();
});
});
});

afterEach(done => {
sinonSandbox.restore();
done();
});
});
});
});
Expand Down