Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Use asynchronous childProcess commands in Network tests - Closes #2334 #2382

Merged
merged 10 commits into from
Sep 14, 2018
Merged

Use asynchronous childProcess commands in Network tests - Closes #2334 #2382

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
425b2b4
Network tests: Modified stopNode, startNode and restartNode functions…
jondubois Sep 6, 2018
ed88a37
Merge branch 'development' into 2334-network_tests_async
shuse2 Sep 7, 2018
8e82426
Rename constant to be more specific
jondubois Sep 10, 2018
55fc17d
Shorten the code related to the done callback in network tests
jondubois Sep 10, 2018
8e93e43
Move pm2LogProcess declaration up above the timeout block to avoid co…
jondubois Sep 10, 2018
b5dbb4a
Renamed constant in network tests
jondubois Sep 10, 2018
fd40d9b
Add comment about why we wait for the 'Finished sync' event on nodes
jondubois Sep 11, 2018
b4220f8
Use pm2 module from node_modules directory
jondubois Sep 12, 2018
bf16d9e
Do not --bail if one of the test cases fails; this prevents the mocha…
jondubois Sep 12, 2018
b835a01
Merge branch 'development' into 2334-network_tests_async
MaciejBaj Sep 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 49 additions & 4 deletions test/network/scenarios/common.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,9 @@
const childProcess = require('child_process');
const utils = require('../utils');

const NODE_FINISHED_SYNC_REGEX = /Finished sync/;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we using Finished sync as a regular expression?

Copy link
Contributor Author

@jondubois jondubois Sep 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I like doing regex.test(...) instead of string.indexOf(...) !== ... it really doesn't matter for me though.

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 added a comment in the code.

const NODE_READY_TIMEOUT = 40000;

const getPeersStatus = peers => {
return Promise.all(
peers.map(peer => {
Expand Down Expand Up @@ -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 => {
Expand All @@ -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');
Copy link
Contributor

Choose a reason for hiding this comment

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

why not remove error listener here as well?

Copy link
Contributor Author

@jondubois jondubois Sep 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It uses the once('error', ...) handler so if the handler executes once then it gets removed implicitly. I can change it to on('error', ...) and then explicitly remove the 'error' event handler if you prefer.

Copy link
Contributor Author

@jondubois jondubois Sep 10, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Also, based on your earlier feedback, I moved up the let pm2LogProcess = ... declaration since that's more clear. Also I made it const.

reject(new Error(`Failed to start node ${nodeName}: ${err.message}`));
});
pm2LogProcess.stdout.on('data', data => {
const dataString = data.toString();
if (NODE_FINISHED_SYNC_REGEX.test(dataString)) {
clearTimeout(nodeReadyTimeout);
pm2LogProcess.stdout.removeAllListeners('error');
pm2LogProcess.stdout.removeAllListeners('data');
resolve();
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Better practice to handle this use case would be to use pm2 builtin feature for it. PM2 have a CLI option –wait-ready and then from the process you can send an event process.send('ready') to notify PM2 that app is ready.

This would be the best way to in the core and then either PM2 or some other process manager can also use it.

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 didn't know about this feature. I'll give it a try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That feature would be perfect but I can't get it to work for some reason.

Copy link
Contributor

Choose a reason for hiding this comment

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

May be we can sit together to check it out.

});
});
},
restartNode: nodeName => {
return childProcess.execSync(`pm2 restart ${nodeName}`);
return common.stopNode(nodeName).then(() => {
return common.startNode(nodeName);
});
},
getNodesStatus: (sockets, cb) => {
getAllPeers(sockets)
Expand All @@ -109,3 +152,5 @@ module.exports = {
});
},
};

module.exports = common;
38 changes: 18 additions & 20 deletions test/network/scenarios/p2p/peer_disconnect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,10 +54,10 @@ 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(done);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check if pm2 kill is being executed after the tests failed.

Copy link
Contributor Author

@jondubois jondubois Sep 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We were running the network tests with the mocha --bail flag which meant that our after() hook was skipped (when an error was thrown inside our before() hook) and so the test suite would not cleanup after itself.

This is an open issue with mocha: mochajs/mocha#3398

Removing the --bail flag fixed the issue.

});

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 => {
Expand Down Expand Up @@ -85,10 +85,10 @@ 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(done);
});

it(`there should be ${EXPECTED_TOTAL_CONNECTIONS} established connections from 500[0-9] ports`, done => {
Expand All @@ -115,24 +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++) {
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');
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++) {
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');
return Promise.all(peersPromises);
});

describe('after all the node restarts', () => {
Expand Down
24 changes: 16 additions & 8 deletions test/network/scenarios/p2p/peers_blacklist.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,10 +82,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_TOTAL_CONNECTIONS_AFTER_BLACKLISTING} established connections from 500[0-9] ports`, done => {
Expand Down Expand Up @@ -153,10 +157,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_TOTAL_CONNECTIONS} established connections from 500[0-9] ports`, done => {
Expand Down