Restructure the integration tests for Jest - Closes #3986 #3985

nazarhussain · 2019-07-22T10:12:46Z

What was the problem?

There were old integration tests in "Mocha" which are some what outdated. So instead of fixing those, we decided to rewrite these tests in the Jest, obviously using the much of the code possible from old test.

How did I fix it?

In this PR, we just added the empty skeleton for integration tests.

Review checklist

The PR resolves Migrate the integration test description to Jest folder with standard. #3986
All new code is covered with unit tests
All new code was formatted with Prettier
Linting passes
Tests pass
Commit messages follow the commit guidelines
Documentation has been added/updated

yatki

This is quite a lot of effort. Thanks nazar.

I just have one concern, I highlighted. Let me know what you think @shuse2 @pablitovicente

yatki · 2019-07-22T11:24:42Z

framework/test/jest/integration/specs/modules/chain/synchronous_tasks.spec.js

@@ -0,0 +1,19 @@
+'use strict';


missing licence header

yatki · 2019-07-22T11:37:55Z

framework/test/jest/integration/specs/modules/chain/transactions/exceptions/signatures.spec.js

+describe('exceptions for senderPublicKey transactions', () => {
+	describe('send funds to account', () => {
+		describe('details of the accounts', () => {
+			describe('when forging block with transaction with collision publicKey', () => {


I'm just highlighting this line as an example. But In general, please avoid nested describe blocks as much as possible. The 3 describe blocks above can be merged into one. We can always add more describe blocks when it's really needed.

If we don't do it in this PR, others will follow the same pattern and we'll end up having bunch of redundant describe blocks.

So would be great if we could improve test descriptions by following the rules below,
1 - Each describe block (except the root one) should start with when.
2 - Each it should start with should.
3 - We don't need describe blocks which contain only one test. We should add them when it's really needed.

@shuse2 there are 53 files in the PR. I think @nazarhussain mainly copied the testing structure from mocha and wasn't planning to spend more time on it. But I think these refactorings are important and maybe we can assign more people to split the files to write test cases properly.

Let me know what you think.

@yatki I agree to all what you said and related to that I asked @shuse2 to decided on some practices e.g. nested level for describe etc and then individual person working on each tests will follow those.

In this PR, I just extracted the skeleton from existing tests and restructured the files a bit to follow current source structure.

shuse2 · 2019-07-22T12:13:31Z

@nazarhussain GJ on the PR.
@yatki
The main purpose of this PR is to setup the good test standard for the integration test.
So, I agree that it still needs to improve the description, as the way you pointed out, too.

At the end, Im expecting to have the good standard for writing the test description for integration and above that we can follow. However, at the same time, I didn't want to loose the current test scenarios too.
Therefore, I ask @nazarhussain to convert the current test to jest and we can improve from there.
Let's improve this PR to the point that can be standard with couple

framework/test/jest/integration/specs/modules/chain/blocks/blocks/on_receive_block.spec.js

pablitovicente · 2019-07-23T08:39:58Z

framework/test/jest/integration/specs/modules/chain/blocks/chain/apply_block.spec.js

+	describe('applyBlock', () => {
+		describe('applyConfirmedStep', () => {
+			describe('after applying new block fails', () => {
+				it.todo('should have pooled transactions in queued state');


Maybe combine this in an it with the describe above it?

There are multiple specs in this describe, so seems ok to not merge.

framework/test/jest/integration/specs/modules/chain/blocks/chain/delete_last_block.spec.js

pablitovicente · 2019-07-23T08:49:00Z

framework/test/jest/integration/specs/modules/chain/blocks/chain/pop_last_block.spec.js

+'use strict';
+
+describe('integration test (blocks) - chain/popLastBlock', () => {
+	describe('popLastBlock', () => {


Maybe simplify the level of describes here so we have less levels?

It follows the structure of the code, anyone who implement and refactor the code as well can improve it.

pablitovicente · 2019-07-23T08:50:43Z

framework/test/jest/integration/specs/modules/chain/blocks/util.spec.js

+		it.todo('should load block 3 from db: block with transactions');
+	});
+
+	describe('loadBlocksOffset() - block/transaction errors', () => {


Maybe split these into describe('loadBlocksOffset() - block errors') and describe('loadBlocksOffset() - transaction errors') ?

pablitovicente · 2019-07-23T08:53:02Z

...work/test/jest/integration/specs/modules/chain/loader/validate_own_chain/first_round.spec.js

+
+		it.todo('all blocks should have version = 0');
+
+		describe('increase block version = 1 and exceptions for height = 5', () => {


Maybe do not nest this cases and repeat setup for each of the three scenarios?

pablitovicente · 2019-07-23T08:53:36Z

.../test/jest/integration/specs/modules/chain/loader/validate_own_chain/full_two_rounds.spec.js

+
+'use strict';
+
+describe('validateOwnChain', () => {


Same comment as for framework/test/jest/integration/specs/modules/chain/loader/validate_own_chain/first_round.spec.js

pablitovicente · 2019-07-23T09:08:29Z

.../jest/integration/specs/modules/chain/loader/validate_own_chain/more_than_two_rounds.spec.js

+
+'use strict';
+
+describe('validateOwnChain', () => {


Same comment as for framework/test/jest/integration/specs/modules/chain/loader/validate_own_chain/first_round.spec.js

pablitovicente · 2019-07-23T09:08:39Z

...est/integration/specs/modules/chain/loader/validate_own_chain/valid_chain_full_round.spec.js

+
+'use strict';
+
+describe('validateOwnChain', () => {


Same comment as for framework/test/jest/integration/specs/modules/chain/loader/validate_own_chain/first_round.spec.js

pablitovicente · 2019-07-23T09:08:48Z

.../integration/specs/modules/chain/loader/validate_own_chain/valid_chain_partial_round.spec.js

+
+'use strict';
+
+describe('validateOwnChain', () => {


Same comment as for framework/test/jest/integration/specs/modules/chain/loader/validate_own_chain/first_round.spec.js

pablitovicente · 2019-07-23T09:09:48Z

framework/test/jest/integration/specs/modules/chain/matcher.spec.js

+	});
+
+	describe('when forging a new block', () => {
+		describe('when transaction pool is full and current context (last block height) at forging time, no longer matches the transaction matcher', () => {


Maybe remove the extra describe? or split in two different describes?

pablitovicente · 2019-07-23T09:10:26Z

framework/test/jest/integration/specs/modules/chain/rebuilding.spec.js

+'use strict';
+
+describe('rebuilding', () => {
+	describe('rebuilding to end of round 2 when blockchain contains 303 blocks', () => {


Maybe remove extra describe or add more scenarios if the idea was that there might be more?

nazarhussain · 2019-07-24T11:49:38Z

@shuse2 There seems to be a lot of updates going on this PR from different point of view. We should rather first agree on some basic stuff then refactor and review the PR accordingly. e.g. How much nested levels these test should support, should we split tests to different files based on different describe blocks

You want to make this refactoring a standard, although for me its vice versa. First we describe some standards or practices then refactor this PR accordingly. In any case, unless we have such coding practices documented anywhere, every individual person can't extract it from looking at test files.

nazarhussain · 2019-07-25T08:11:11Z

@pablitovicente @shuse2 @limiaspasdaniel @yatki Feel free to add your commits to the PR directly, so we can close it earliest possible.

framework/test/jest/integration/specs/modules/chain/rounds.spec.js

pablitovicente

I think we still can continue improving the descriptions. Once we start re-writing the tests we should check again the descriptions to make sure they work as very clear documentation.

addressed

nazarhussain added 2 commits July 18, 2019 14:40

Add empty structure for integration tets

46a6ebc

Update the structure of all integration tests

5cc95f3

nazarhussain self-assigned this Jul 22, 2019

nazarhussain requested a review from shuse2 July 22, 2019 10:12

nazarhussain added the 👁️ pending review label Jul 22, 2019

shuse2 requested review from pablitovicente and yatki July 22, 2019 11:11

yatki previously requested changes Jul 22, 2019

View reviewed changes

shuse2 changed the title ~~Restructure the integration tests for Jest~~ Restructure the integration tests for Jest - Closes #3986 Jul 22, 2019

shuse2 added 🏗️ in progress and removed 👁️ pending review labels Jul 22, 2019