From 425b2b47c4f8c5e70f2535bfb736455336b83533 Mon Sep 17 00:00:00 2001 From: Jonathan Gros-Dubois Date: Thu, 6 Sep 2018 18:48:28 +0200 Subject: [PATCH 1/8] Network tests: Modified stopNode, startNode and restartNode functions to be asynchronous --- test/network/scenarios/common.js | 53 ++++++++++++++++-- test/network/scenarios/p2p/peer_disconnect.js | 54 ++++++++++++------- test/network/scenarios/p2p/peers_blacklist.js | 24 ++++++--- 3 files changed, 101 insertions(+), 30 deletions(-) diff --git a/test/network/scenarios/common.js b/test/network/scenarios/common.js index 30ea29f87a9..08f71baf367 100644 --- a/test/network/scenarios/common.js +++ b/test/network/scenarios/common.js @@ -17,6 +17,9 @@ const childProcess = require('child_process'); const utils = require('../utils'); +const NODE_READY_REGEX = /Finished sync/; +const NODE_READY_TIMEOUT = 20000; + const getPeersStatus = peers => { return Promise.all( peers.map(peer => { @@ -52,7 +55,7 @@ const getAllPeers = sockets => { ); }; -module.exports = { +const common = { setMonitoringSocketsConnections: (params, configurations) => { // eslint-disable-next-line mocha/no-top-level-hooks before(done => { @@ -78,13 +81,53 @@ module.exports = { getAllPeers, stopNode: nodeName => { - return childProcess.execSync(`pm2 stop ${nodeName}`); + return new Promise((resolve, reject) => { + childProcess.exec(`pm2 stop ${nodeName}`, err => { + if (err) { + return reject( + new Error(`Failed to stop node ${nodeName}: ${err.message}`) + ); + } + resolve(); + }); + }); }, startNode: nodeName => { - childProcess.execSync(`pm2 start ${nodeName}`); + return new Promise((resolve, reject) => { + childProcess.exec(`pm2 start ${nodeName}`, err => { + if (err) { + return reject( + new Error(`Failed to start node ${nodeName}: ${err.message}`) + ); + } + const nodeReadyTimeout = setTimeout(() => { + pm2LogProcess.stdout.removeAllListeners('data'); + pm2LogProcess.removeAllListeners('error'); + reject(new Error(`Failed to start node ${nodeName} before timeout`)); + }, NODE_READY_TIMEOUT); + + let pm2LogProcess = childProcess.spawn('pm2', ['logs', nodeName]); + pm2LogProcess.once('error', err => { + clearTimeout(nodeReadyTimeout); + pm2LogProcess.stdout.removeAllListeners('data'); + reject(new Error(`Failed to start node ${nodeName}: ${err.message}`)); + }); + pm2LogProcess.stdout.on('data', data => { + const dataString = data.toString(); + if (NODE_READY_REGEX.test(dataString)) { + clearTimeout(nodeReadyTimeout); + pm2LogProcess.stdout.removeAllListeners('error'); + pm2LogProcess.stdout.removeAllListeners('data'); + resolve(); + } + }); + }); + }); }, restartNode: nodeName => { - return childProcess.execSync(`pm2 restart ${nodeName}`); + return common.stopNode(nodeName).then(() => { + return common.startNode(nodeName); + }); }, getNodesStatus: (sockets, cb) => { getAllPeers(sockets) @@ -109,3 +152,5 @@ module.exports = { }); }, }; + +module.exports = common; diff --git a/test/network/scenarios/p2p/peer_disconnect.js b/test/network/scenarios/p2p/peer_disconnect.js index 8c5bdb9d223..bd1a695e542 100644 --- a/test/network/scenarios/p2p/peer_disconnect.js +++ b/test/network/scenarios/p2p/peer_disconnect.js @@ -48,10 +48,14 @@ module.exports = function( describe('when a node is stopped', () => { before(done => { - common.stopNode('node_1'); - setTimeout(() => { - done(); - }, 2000); + common + .stopNode('node_1') + .then(() => { + done(); + }) + .catch(err => { + done(err.message); + }); }); it(`peer manager should remove peer from the list and there should be ${EXPECTED_OUTOGING_CONNECTIONS - @@ -77,10 +81,14 @@ module.exports = function( describe('when a stopped node is started', () => { before(done => { - common.startNode('node_1'); - setTimeout(() => { - done(); - }, 2000); + common + .startNode('node_1') + .then(() => { + done(); + }) + .catch(err => { + done(err.message); + }); }); it(`there should be ${EXPECTED_OUTOGING_CONNECTIONS} established connections from 500[0-9] ports`, done => { @@ -108,23 +116,33 @@ module.exports = function( // Need to keep one peer so that we can validate // Duplicate socket connection exists or not it('stop all the nodes in the network except node_0', done => { + const peersPromises = []; for (let i = 1; i < TOTAL_PEERS; i++) { - common.stopNode(`node_${i}`); + peersPromises.push(common.stopNode(`node_${i}`)); } - setTimeout(() => { - console.info('Wait for nodes to be stopped'); - done(); - }, 10000); + console.info('Wait for nodes to be stopped'); + Promise.all(peersPromises) + .then(() => { + done(); + }) + .catch(err => { + done(err.message); + }); }); it('start all nodes that were stopped', done => { + const peersPromises = []; for (let i = 1; i < TOTAL_PEERS; i++) { - common.startNode(`node_${i}`); + peersPromises.push(common.startNode(`node_${i}`)); } - setTimeout(() => { - console.info('Wait for nodes to be started'); - done(); - }, 10000); + console.info('Wait for nodes to be started'); + Promise.all(peersPromises) + .then(() => { + done(); + }) + .catch(err => { + done(err.message); + }); }); describe('after all the node restarts', () => { diff --git a/test/network/scenarios/p2p/peers_blacklist.js b/test/network/scenarios/p2p/peers_blacklist.js index 823eaecb5ed..50cbfff256c 100644 --- a/test/network/scenarios/p2p/peers_blacklist.js +++ b/test/network/scenarios/p2p/peers_blacklist.js @@ -77,10 +77,14 @@ module.exports = function( JSON.stringify(params.configurations[0], null, 4) ); // Restart the node to load the just changed configuration - common.restartNode('node_0'); - setTimeout(() => { - blockchainReady(done, null, null, 'http://127.0.0.1:4000'); - }, 8000); + common + .restartNode('node_0') + .then(() => { + blockchainReady(done, null, null, 'http://127.0.0.1:4000'); + }) + .catch(err => { + done(err.message); + }); }); it(`there should be ${EXPECTED_OUTOGING_CONNECTIONS_AFTER_BLACKLISTING} established connections from 500[0-9] ports`, done => { @@ -148,10 +152,14 @@ module.exports = function( JSON.stringify(params.configurations[0], null, 4) ); // Restart the node to load the just changed configuration - common.restartNode('node_0'); - setTimeout(() => { - blockchainReady(done, null, null, 'http://127.0.0.1:4000'); - }, 8000); + common + .restartNode('node_0') + .then(() => { + blockchainReady(done, null, null, 'http://127.0.0.1:4000'); + }) + .catch(err => { + done(err.message); + }); }); it(`there should be ${EXPECTED_OUTOGING_CONNECTIONS} established connections from 500[0-9] ports`, done => { From 8e8242629487a4587e0081aabe937a4c5f4f4195 Mon Sep 17 00:00:00 2001 From: Jonathan Gros-Dubois Date: Mon, 10 Sep 2018 12:33:17 +0200 Subject: [PATCH 2/8] Rename constant to be more specific --- test/network/scenarios/common.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/network/scenarios/common.js b/test/network/scenarios/common.js index 08f71baf367..52e3dd70faa 100644 --- a/test/network/scenarios/common.js +++ b/test/network/scenarios/common.js @@ -17,8 +17,8 @@ const childProcess = require('child_process'); const utils = require('../utils'); -const NODE_READY_REGEX = /Finished sync/; -const NODE_READY_TIMEOUT = 20000; +const NODE_FINISHED_SYNC_REGEX = /Finished sync/; +const NODE_READY_TIMEOUT = 40000; const getPeersStatus = peers => { return Promise.all( @@ -114,7 +114,7 @@ const common = { }); pm2LogProcess.stdout.on('data', data => { const dataString = data.toString(); - if (NODE_READY_REGEX.test(dataString)) { + if (NODE_FINISHED_SYNC_REGEX.test(dataString)) { clearTimeout(nodeReadyTimeout); pm2LogProcess.stdout.removeAllListeners('error'); pm2LogProcess.stdout.removeAllListeners('data'); From 55fc17dd0609f7ce84c37cbce9ee71ac926f4ddc Mon Sep 17 00:00:00 2001 From: Jonathan Gros-Dubois Date: Mon, 10 Sep 2018 14:15:44 +0200 Subject: [PATCH 3/8] Shorten the code related to the done callback in network tests --- test/network/scenarios/p2p/peer_disconnect.js | 36 +++++-------------- 1 file changed, 8 insertions(+), 28 deletions(-) diff --git a/test/network/scenarios/p2p/peer_disconnect.js b/test/network/scenarios/p2p/peer_disconnect.js index 75007b080d9..c9b36d5e3d3 100644 --- a/test/network/scenarios/p2p/peer_disconnect.js +++ b/test/network/scenarios/p2p/peer_disconnect.js @@ -56,12 +56,8 @@ module.exports = function( before(done => { common .stopNode('node_1') - .then(() => { - done(); - }) - .catch(err => { - done(err.message); - }); + .then(done) + .catch(done); }); it(`peer manager should remove peer from the list and there should be ${EXPECTED_TOTAL_CONNECTIONS_AFTER_REMOVING_PEER} established connections from 500[0-9] ports`, done => { @@ -91,12 +87,8 @@ module.exports = function( before(done => { common .startNode('node_1') - .then(() => { - done(); - }) - .catch(err => { - done(err.message); - }); + .then(done) + .catch(done); }); it(`there should be ${EXPECTED_TOTAL_CONNECTIONS} established connections from 500[0-9] ports`, done => { @@ -123,34 +115,22 @@ module.exports = function( // To validate peers holding socket connection // Need to keep one peer so that we can validate // Duplicate socket connection exists or not - it('stop all the nodes in the network except node_0', done => { + it('stop all the nodes in the network except node_0', () => { const peersPromises = []; for (let i = 1; i < TOTAL_PEERS; i++) { peersPromises.push(common.stopNode(`node_${i}`)); } console.info('Wait for nodes to be stopped'); - Promise.all(peersPromises) - .then(() => { - done(); - }) - .catch(err => { - done(err.message); - }); + return Promise.all(peersPromises); }); - it('start all nodes that were stopped', done => { + it('start all nodes that were stopped', () => { const peersPromises = []; for (let i = 1; i < TOTAL_PEERS; i++) { peersPromises.push(common.startNode(`node_${i}`)); } console.info('Wait for nodes to be started'); - Promise.all(peersPromises) - .then(() => { - done(); - }) - .catch(err => { - done(err.message); - }); + return Promise.all(peersPromises); }); describe('after all the node restarts', () => { From 8e93e43c642f642aedcf3fadd5613b79d384c07d Mon Sep 17 00:00:00 2001 From: Jonathan Gros-Dubois Date: Mon, 10 Sep 2018 17:33:58 +0200 Subject: [PATCH 4/8] Move pm2LogProcess declaration up above the timeout block to avoid confusion --- test/network/scenarios/common.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/network/scenarios/common.js b/test/network/scenarios/common.js index 52e3dd70faa..2cd59ed2d9f 100644 --- a/test/network/scenarios/common.js +++ b/test/network/scenarios/common.js @@ -100,13 +100,15 @@ const common = { new Error(`Failed to start node ${nodeName}: ${err.message}`) ); } + + const pm2LogProcess = childProcess.spawn('pm2', ['logs', nodeName]); + const nodeReadyTimeout = setTimeout(() => { pm2LogProcess.stdout.removeAllListeners('data'); pm2LogProcess.removeAllListeners('error'); reject(new Error(`Failed to start node ${nodeName} before timeout`)); }, NODE_READY_TIMEOUT); - let pm2LogProcess = childProcess.spawn('pm2', ['logs', nodeName]); pm2LogProcess.once('error', err => { clearTimeout(nodeReadyTimeout); pm2LogProcess.stdout.removeAllListeners('data'); From b5dbb4a655c256c2b79c21c3b1f3a722a29d3b83 Mon Sep 17 00:00:00 2001 From: Jonathan Gros-Dubois Date: Mon, 10 Sep 2018 17:36:47 +0200 Subject: [PATCH 5/8] Renamed constant in network tests --- test/network/scenarios/common.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/network/scenarios/common.js b/test/network/scenarios/common.js index 2cd59ed2d9f..cfaac3f95b8 100644 --- a/test/network/scenarios/common.js +++ b/test/network/scenarios/common.js @@ -18,7 +18,7 @@ const childProcess = require('child_process'); const utils = require('../utils'); const NODE_FINISHED_SYNC_REGEX = /Finished sync/; -const NODE_READY_TIMEOUT = 40000; +const NODE_FINISHED_SYNC_TIMEOUT = 40000; const getPeersStatus = peers => { return Promise.all( @@ -107,7 +107,7 @@ const common = { pm2LogProcess.stdout.removeAllListeners('data'); pm2LogProcess.removeAllListeners('error'); reject(new Error(`Failed to start node ${nodeName} before timeout`)); - }, NODE_READY_TIMEOUT); + }, NODE_FINISHED_SYNC_TIMEOUT); pm2LogProcess.once('error', err => { clearTimeout(nodeReadyTimeout); From fd40d9b324c9cc4a074488b62ddb6385016aa398 Mon Sep 17 00:00:00 2001 From: Jonathan Gros-Dubois Date: Tue, 11 Sep 2018 10:05:26 +0200 Subject: [PATCH 6/8] Add comment about why we wait for the 'Finished sync' event on nodes --- test/network/scenarios/common.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/network/scenarios/common.js b/test/network/scenarios/common.js index cfaac3f95b8..5cfee1bf2ae 100644 --- a/test/network/scenarios/common.js +++ b/test/network/scenarios/common.js @@ -116,6 +116,8 @@ const common = { }); pm2LogProcess.stdout.on('data', data => { const dataString = data.toString(); + // Make sure that all nodes have fully synced before we + // run the test cases. if (NODE_FINISHED_SYNC_REGEX.test(dataString)) { clearTimeout(nodeReadyTimeout); pm2LogProcess.stdout.removeAllListeners('error'); From b4220f8c22cda7019ade9f17deddad3b8e150e98 Mon Sep 17 00:00:00 2001 From: Jonathan Gros-Dubois Date: Wed, 12 Sep 2018 16:54:23 +0200 Subject: [PATCH 7/8] Use pm2 module from node_modules directory --- test/network/scenarios/common.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/network/scenarios/common.js b/test/network/scenarios/common.js index 5cfee1bf2ae..dd573242bec 100644 --- a/test/network/scenarios/common.js +++ b/test/network/scenarios/common.js @@ -82,7 +82,7 @@ const common = { stopNode: nodeName => { return new Promise((resolve, reject) => { - childProcess.exec(`pm2 stop ${nodeName}`, err => { + childProcess.exec(`node_modules/.bin/pm2 stop ${nodeName}`, err => { if (err) { return reject( new Error(`Failed to stop node ${nodeName}: ${err.message}`) @@ -94,14 +94,17 @@ const common = { }, startNode: nodeName => { return new Promise((resolve, reject) => { - childProcess.exec(`pm2 start ${nodeName}`, err => { + childProcess.exec(`node_modules/.bin/pm2 start ${nodeName}`, err => { if (err) { return reject( new Error(`Failed to start node ${nodeName}: ${err.message}`) ); } - const pm2LogProcess = childProcess.spawn('pm2', ['logs', nodeName]); + const pm2LogProcess = childProcess.spawn('node_modules/.bin/pm2', [ + 'logs', + nodeName, + ]); const nodeReadyTimeout = setTimeout(() => { pm2LogProcess.stdout.removeAllListeners('data'); From bf16d9ea5bf4d4381da72ab4e48f8109f64e9ef2 Mon Sep 17 00:00:00 2001 From: Jonathan Gros-Dubois Date: Wed, 12 Sep 2018 16:56:11 +0200 Subject: [PATCH 8/8] Do not --bail if one of the test cases fails; this prevents the mocha after() hook from running which prevents cleanup of nodes --- Gruntfile.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gruntfile.js b/Gruntfile.js index 643b29d7c64..5213af23bf7 100644 --- a/Gruntfile.js +++ b/Gruntfile.js @@ -69,7 +69,7 @@ module.exports = function(grunt) { 'The specified tag is not supported.\n\nExample: `grunt mocha:::[section]` or `npm test -- mocha:::[section]`\n\n- Where tag can be one of default | unstable | slow | extensive (required)\n- Where suite can be one of unit | integration | functional | network (required)\n- Where section can be one of get | post | ws (optional)' ); } - return `./node_modules/.bin/_mocha --bail test/network/index.js ${filter}`; + return `./node_modules/.bin/_mocha test/network/index.js ${filter}`; } var toExecute = [tagFilter, suite, section] .filter(val => val)