Unit tests for logic/vote #26

jarenal · 2017-10-13T14:28:40Z

Test ready for review! Some test will fail until I fix some little issues that I found in logic/vote

vekexasia

Please remove all "..." texts on it. I think the other texts don't have that

jarenal · 2017-10-13T16:41:13Z

Done! '...' Removed

nerdvibe

@jarenal Thanks a lot for your code, it's really appreciated!
I've made some notes about your code.

Some things that I would recommend you:

Use semi columns. There are no semi columns in the code.
Avoid to make a function call for every variable that you need to get from the original file. Always try to cache locally as much data as you can :)
Try to follow code patterns/stylings. I'm my opinion the whole code should look like it has been written only by only one person. E.g. I would avoid to use contexts if they have not been used it also before.

I'd also recommend you to setup prettier and use it with webstorm. It's an 💥 combination that will style your code in an awesome way.

nerdvibe · 2017-10-18T21:15:30Z

tests/unit/logic/vote.spec.js

+
+		describe('logger and schema should be setted properly into library', function () {
+			it('should be an object', function () {
+				expect(Vote.__get__('library').logger).to.equals(logger)


I think you could get library at the beginning of the "it" and check then in every expect what you need to expect.
In this way you your code will look a bit better and be a bit more performant :)

nerdvibe · 2017-10-18T21:34:04Z

tests/unit/logic/vote.spec.js

+		})
+	})
+
+	describe('when we call to...', function () {


I see here a different pattern in writing tests. I believe we should agree on something.
Roman's pattern (which is the one that I'm following) is to not wrap all the methods inside another wrapper that is inside another wrapper like in this case.

I'd reccomend to follow his pattern:

describe('logic/vote', function () { describe('constructor', function () { it('test', ..) it('test', ..) it('test', ..) }) describe('method', function () { it('test', ..) it('test', ..) it('test', ..) }) describe('method2', function () { it('test', ..) it('test', ..) it('test', ..) }) })

I agree, Lets stick with the later pattern.

nerdvibe · 2017-10-18T21:35:50Z

tests/unit/logic/vote.spec.js

+
+			it('delegates, rounds and system should be setted properly into modules variable', function () {
+				vote.bind(delegates, rounds, system)
+				expect(Vote.__get__('modules').delegates).to.equals(delegates)


I'd recommend to call get only once :)

Yes 👍 it also improves code readability.

nerdvibe · 2017-10-18T21:54:11Z

tests/unit/logic/vote.spec.js

+
+			it('should return vote\'s fee', function () {
+				vote.bind(delegates, rounds, system)
+				var fee = vote.calculateFee(trs, sender, height)


I'd recommend to stub calculateFees and check if it's called.

Hmm? you mean removing this test entirely? I would actually keep it @xunga

No modifications were done

nerdvibe · 2017-10-18T21:58:19Z

tests/unit/logic/vote.spec.js

+
+			context('if trs.recipientId and trs.senderId are not equals', function () {
+
+				beforeEach(function () {


This is more a question from my side. Do we really need a beforeEach and afterEach when we have only one test case? In my opinion I would put the logic directly into the it. Maybe this is not a correct approach, I would like to get an opinion on this :)

Well test cases should only test behavioural stuff. all init and teardown should be kept separate as much as you can.

nerdvibe · 2017-10-19T13:42:42Z

tests/unit/logic/vote.spec.js

+			context('if everything is ok', function () {
+				it('should return an object with votes as property in Array format', function () {
+					var result = vote.dbRead(raw_success)
+					expect(result).to.have.deep.property('votes')


I think that the next expect is enough, or?

I added the second one for to verify that the values received are correct.

nerdvibe · 2017-10-19T13:43:09Z

tests/unit/logic/vote.spec.js

+
+
+		})
+


Test of dbTable and dbFields are missing...

I don't think that to test a static object is within the testing standards. What do you think guys?

nerdvibe · 2017-10-19T13:44:57Z

tests/unit/logic/vote.spec.js

+				trs = {id: 123, asset: {votes: [1, 2, 3]}}
+			})
+
+			it('should return an object with a valid db schema', function () {


I would say that expect(result).to.instanceof(Object) is useless and I would also test what happens if trs.asset.votes is not an array...

nerdvibe · 2017-10-19T13:45:27Z

tests/unit/logic/vote.spec.js

+			beforeEach(function () {
+				trs = {}
+				trs_case_4 = {signatures: [1, 2, 3]}
+				trs_case_4_b = {signatures: [1, 2]}


Same of before, no need of this code in here :)

nerdvibe · 2017-10-19T13:46:06Z

tests/unit/logic/vote.spec.js

+				})
+			})
+
+			context('Fail 5: If v_votes is not a string', function () {


This test is failing

Depends on issue #31

vekexasia · 2017-10-19T17:01:31Z

tests/unit/logic/vote.spec.js

+
+			it('delegates, rounds and system should be setted properly into modules variable', function () {
+				vote.bind(delegates, rounds, system)
+				expect(Vote.__get__('modules').delegates).to.equals(delegates)


Yes 👍 it also improves code readability.

vekexasia · 2017-10-19T17:02:35Z

tests/unit/logic/vote.spec.js

+
+			it('should return vote\'s fee', function () {
+				vote.bind(delegates, rounds, system)
+				var fee = vote.calculateFee(trs, sender, height)


Hmm? you mean removing this test entirely? I would actually keep it @xunga

vekexasia · 2017-10-19T17:03:50Z

tests/unit/logic/vote.spec.js

+
+			context('if trs.recipientId and trs.senderId are not equals', function () {
+
+				beforeEach(function () {


Well test cases should only test behavioural stuff. all init and teardown should be kept separate as much as you can.

vekexasia · 2017-10-19T17:08:27Z

tests/unit/logic/vote.spec.js

+
+				beforeEach(function () {
+					clock = sinon.useFakeTimers()
+					Vote.__set__('setImmediate', setImmediate)


I see that you use context instead of sub describe while it's a safe guard we shouldn't misuse contexts especially if they're not really used.

Also it would be better to have one beforeEach and afterEach where you sinon.useFakeTimers and also do other shared stuff between tests (like the setImmediate thing?)

Right now there is a lot of boilerplate involving the init/teardown of test groups.

vekexasia · 2017-10-19T17:08:43Z

tests/unit/logic/vote.spec.js

+				clock.restore()
+			})
+
+			it('should call to callback without errors', function () {


vekexasia · 2017-10-19T17:09:42Z

tests/unit/logic/vote.spec.js

+				sender = {address: 123}
+				block = {id: 123, height: 123}
+				Vote.__set__('setImmediate', setImmediate)
+				Vote.__set__('self.checkConfirmedDelegates', function (trs, cb) {


vekexasia · 2017-10-19T17:10:39Z

tests/unit/logic/vote.spec.js

+				clock.restore()
+			})
+
+			it('should call to callback', function () {


The test says should call callback and tests that. If there's a need to check the parent.scope.account then another test (probably above this one) needs to be written

vekexasia · 2017-10-19T17:11:04Z

tests/unit/logic/vote.spec.js

+				it('should call to scope.account.merge() and callback()', function () {
+					clock.runAll()
+					var result = Vote.__get__('self.scope')
+					expect(result.account.merge.called).to.be.true


coveralls · 2017-10-27T11:28:33Z

Coverage increased (+4.4%) to 59.389% when pulling 694b166 on unit-test-logic-vote into 5f23a12 on development.

jarenal · 2017-10-27T11:55:14Z

This PR is failing because depends on these issues #31 and #43

Unit tests for logic/vote

f10b65b

vekexasia suggested changes Oct 13, 2017

View reviewed changes

jarenal added 2 commits October 13, 2017 18:19

Improvements for undo() and undoUnconfirmed() tests

8db161b

Removed '...' strings

428921e

nerdvibe suggested changes Oct 19, 2017

View reviewed changes

vekexasia reviewed Oct 19, 2017

View reviewed changes

jarenal and others added 4 commits October 24, 2017 21:58

Merge branch 'development' into unit-test-logic-vote

bd98539

Merge branch 'development' into unit-test-logic-vote

ba2a79e

Applied improvements after review

583029b

fixed tests

694b166

vekexasia approved these changes Oct 27, 2017

View reviewed changes

nerdvibe approved these changes Oct 27, 2017

View reviewed changes

vekexasia merged commit 8966c11 into development Oct 27, 2017

vekexasia deleted the unit-test-logic-vote branch October 27, 2017 15:21


		context('if trs.recipientId and trs.senderId are not equals', function () {

		beforeEach(function () {

Unit tests for logic/vote #26

Unit tests for logic/vote #26

Conversation

jarenal commented Oct 13, 2017

vekexasia left a comment

Choose a reason for hiding this comment

jarenal commented Oct 13, 2017

nerdvibe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 27, 2017

jarenal commented Oct 27, 2017