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.

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

Pararellize travis jobs #410

Merged
merged 2 commits into from Feb 1, 2017
Merged

Conversation

MaciejBaj
Copy link
Contributor

Create a Travis jobs matrix consisting of seperate tests.

closes #407

@@ -42,7 +42,7 @@ describe('POST /peer/transactions', function () {

postTransaction(transaction, function (err, res) {
node.expect(res.body).to.have.property('success').to.be.not.ok;
node.expect(res.body).to.have.property('message').to.equal('Invalid sender public key: b26dd40ba33e4785e49ddc4f106c0493ed00695817235c778f487aea5866400a expected: ce33db918b059a6e99c402963b42cf51c695068007ef01d8c383bb8a41270263');
node.expect(res.body).to.have.property('message').to.equal('Failed to verify signature');
Copy link
Contributor

Choose a reason for hiding this comment

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

Why changed expectations here? Did we change response in this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Expectation changed so the tests simply pass now :) The changes in current PR has nothing common with changing the logic of transactions, so I believe that the problem with this test failing occurred before and is connected with #400

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After the talk with Oliver we came to the conclusion, that the result of that transaction insertion is changed because the tests which are not fired now were changing the state of DB table. This is probably true- the commit (1fc0f03) that changed this expectation was probably added after introducing new test in a different test suite, that mutated db state before.

@karmacoma karmacoma self-assigned this Jan 31, 2017
.travis.yml Outdated
@@ -1,16 +1,54 @@
dist: 'trusty'
language: node_js
node_js:
- '0.12.17'
- 6
Copy link
Member

Choose a reason for hiding this comment

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

Can we be more specific on version number? Production release will be on 6.9.4.

.travis.yml Outdated
- node app.js &> .app.log &
env:
global:
- HOST=http://0.0.0.0:4000
Copy link
Member

Choose a reason for hiding this comment

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

This environment variable is needed?

before(function (done) {
require('./../common/globalBefore').waitUntilBlockchainReady(done);
});

Copy link
Member

Choose a reason for hiding this comment

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

Seeing as we execute this before hook for every test file. Please consolidate these before hooks as one within: https://github.com/LiskHQ/lisk/blob/development/test/node.js. This would be better imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running the blockchain loaded check in https://github.com/LiskHQ/lisk/blob/development/test/node.js will introduce the need of loading it asynchronously. The condition will have to be run every time when requiring the module. After all, I propose to leave the hooks as they are now.

/**
* @param {string} table
* @param {Logger} logger
* @param {Object} db
* @param {Function} cb
*/
function clearDatabaseTable (db, logger, table, cb) {
function clearDatabaseTable(db, logger, table, cb) {
Copy link
Member

Choose a reason for hiding this comment

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

function clearDatabaseTable (db, logger, table, cb) {

db.query('DELETE FROM ' + table).then(function (result) {
cb(null, result);
}).catch(function (err) {
logger.err('Failed to clear database table: ' + table);
logger.err('unable to delete data from table: ' + table);
Copy link
Member

Choose a reason for hiding this comment

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

logger.err('Failed to clear database table: ' + table);

* @param {Number} [retries=10] retries
* @param {Number} [timeout=200] timeout
*/
function waitUntilBlockchainReady(cb, retries, timeout) {
Copy link
Member

Choose a reason for hiding this comment

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

function waitUntilBlockchainReady (cb, retries, timeout) {

if (!timeout) {
timeout = 200;
}
(function fetchBlockchainStatus() {
Copy link
Member

Choose a reason for hiding this comment

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

(function fetchBlockchainStatus () {

Create a Travis jobs matrix consisting of seperate tests.

closes LiskHQ#407
@MaciejBaj MaciejBaj force-pushed the 407-pararell-travis branch 2 times, most recently from f5335d0 to 78504a6 Compare February 1, 2017 15:26
@karmacoma karmacoma changed the title pararellize Travis jobs Pararellize travis jobs Feb 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parallelize travis jobs
3 participants