Fix transaction pool bug in "transactionInPool" plus tests - Closes #707 #708

vekexasia · 2017-08-09T08:16:17Z

Fixes #707

karmacoma

Thank you @vekexasia 👍

karmacoma · 2017-08-09T09:36:13Z

test/unit/logic/transactionPool.js

+}
+
+describe('transactionPool', function () {
+  let txPool;


We are still on es5. The project is scheduled to be updated at the end of the current milestone.

oops you're right. missed that let

karmacoma · 2017-08-09T09:37:11Z

test/unit/logic/transactionPool.js

+  let txPool;
+
+  before(function (done) {
+    // Init transaction logic


I believe we now have a test helper for this. Not sure if applicable @4miners?

Yes, but that is not merged yet.

karmacoma · 2017-08-09T09:38:01Z

logic/transactionPool.js

-	].filter(Boolean).length > 0;
+	].filter(function (index) {
+		return typeof(index) === 'number';
+  }).length > 0;


Indentation

karmacoma · 2017-08-09T09:39:36Z

test/unit/logic/transactionPool.js

+
+  });
+
+  describe('receiveTransactions', function () {


Please adjust the formatting to:

describe('description', function () { it('example 1'); it('example 2'); it('example 3'); });

hmm @karmacoma I might be blind but i don't see what's different between your formatting and mine

4miners · 2017-08-09T09:51:23Z

logic/transactionPool.js

@@ -101,7 +101,9 @@ TransactionPool.prototype.transactionInPool = function (id) {
 		self.bundled.index[id],
 		self.queued.index[id],
 		self.multisignature.index[id]
-	].filter(Boolean).length > 0;
+	].filter(function (index) {
+		return typeof(index) === 'number';


return index !== undefined is more clear ans should do the same

@4miners I'd disagree about the clearness of doing index !== undefined.
While the result would be the same (Starting from es5 since before undefined used to be mutable) it does not give any hint on what index should be.

Keeping it as I proposed will clearly give code-reader a clue of what self.bundled.*[id] should be => a number.

fine for me :)

I would suggest to use some as it returns a boolean and does not iterate through a whole array if finds a match before:
[...].some(function(index){return typeof index === 'number'})

4miners · 2017-08-09T09:58:27Z

test/unit/logic/transactionPool.js

+  let txPool;
+
+  before(function (done) {
+    // Init transaction logic


Yes, but that is not merged yet.

Isabello · 2017-08-09T13:14:41Z

Please fix your ESLINT failures:

https://jenkins.lisk.io/job/lisk-core/job/PR-708/6/execution/node/204/log/

Isabello

Eslint failures

MaciejBaj · 2017-08-10T12:47:59Z

test/unit/logic/transactionPool.js

+			//   modulesLoader.initModuleWithDb(TransactionModule, cb, {
+			//     transaction: result.transactionLogic
+			//   });
+			// }]


get rid of comments

MaciejBaj

remove the commented code from tests
I reccomend to use some function, explained in the comment

vekexasia · 2017-08-11T12:42:47Z

Hello,

I've just pushed a commit where I removed some comments in the test files and used some instead of filter as proposed by @MaciejBaj

4miners · 2017-08-14T07:56:08Z

test/unit/logic/transactionPool.js

+var AccountModule    = require('../../../modules/accounts.js');
+var BlocksModule     = require('../../../modules/blocks.js');
+var AccountLogic     = require('../../../logic/account.js');
+var Rounds           = require('../../../modules/rounds.js');


Rounds module is no longer there, please remove all references to it.

MaciejBaj · 2017-08-14T08:11:58Z

logic/transactionPool.js

-	].filter(Boolean).length > 0;
+	].some(function (index) {
+		return typeof(index) === 'number';
+	}).length > 0;


some returns true / false so need to check the length here

my bad. sorry.

…ansactionPool tests.

4miners · 2017-08-14T12:45:26Z

test/unit/logic/transactionPool.js

+					blocks  : result.blockModule
+				});
+				var sendLogic = result.transactionLogic.attachAssetType(transactionTypes.SEND, new TransferLogic());
+				sendLogic.bind(account, /* rounds */ null);


do we need that null here?

I think it's best to leave it like that for clarity as proper initialization for further testing might need rounds too.

No, rounds are not exist anymore.

hmm? since when?

Btw i committed with requested modifications :)

Since #597, thanks :)

4miners · 2017-08-14T12:46:02Z

test/unit/logic/transactionPool.js

+
+				result.delegateModule.onBind({
+					accounts: __accountModule,
+					// transactions: result.transactionModule,


either enable or remove

4miners · 2017-08-14T12:46:12Z

test/unit/logic/transactionPool.js

+				account.onBind({
+					delegates: result.delegateModule,
+					accounts : account,
+					// transactions: result.transactionModule


Isabello · 2017-08-15T09:34:14Z

Tests are failing when merged against the 1.0.0 branch:

  transactionPool
    receiveTransactions
      ✓ should do nothing for empty array
      ✓ should return error for invalid tx
>> /home/lisk/workspace/lisk-core_PR-708-EDN5H7BPFPHQW4UHUSL4OK4TW2RIYZ4Z52RCLPU7BZC2MGVJYLVQ/node_modules/js-nacl/lib/nacl_factory.js:27
>> var Module;if(!Module)Module=(typeof Module!=="undefined"?Module:null)||{};var moduleOverrides={};for(var key in Module){if(Module.hasOwnProperty(key)){moduleOverrides[key]=Module[key]}}var ENVIRONMENT_IS_WEB=false;var ENVIRONMENT_IS_WORKER=false;var ENVIRONMENT_IS_NODE=false;var ENVIRONMENT_IS_SHELL=false;if(Module["ENVIRONMENT"]){if(Module["ENVIRONMENT"]==="WEB"){ENVIRONMENT_IS_WEB=true}else if(Module["ENVIRONMENT"]==="WORKER"){ENVIRONMENT_IS_WORKER=true}else if(Module["ENVIRONMENT"]==="NODE"){ENVIRONMENT_IS_NODE=true}else if(Module["ENVIRONMENT"]==="SHELL"){ENVIRONMENT_IS_SHELL=true}else{throw new Error("The provided Module['ENVIRONMENT'] value is not valid. It must be one of: WEB|WORKER|NODE|SHELL.")}}else{ENVIRONMENT_IS_WEB=typeof window==="object";ENVIRONMENT_IS_WORKER=typeof importScripts==="function";ENVIRONMENT_IS_NODE=typeof process==="object"&&typeof require==="fu
>> AssertionError: expected [TypeError: Cannot read property 'validate' of undefined] to not exist
>>   at Assertion.<anonymous> (/home/lisk/workspace/lisk-core_PR-708-EDN5H7BPFPHQW4UHUSL4OK4TW2RIYZ4Z52RCLPU7BZC2MGVJYLVQ/node_modules/chai/lib/chai/core/assertions.js:758:10)
>>   at Assertion.propertyGetter (/home/lisk/workspace/lisk-core_PR-708-EDN5H7BPFPHQW4UHUSL4OK4TW2RIYZ4Z52RCLPU7BZC2MGVJYLVQ/node_modules/chai/lib/chai/utils/addProperty.js:62:29)
>>   at Object.proxyGetter [as get] (/home/lisk/workspace/lisk-core_PR-708-EDN5H7BPFPHQW4UHUSL4OK4TW2RIYZ4Z52RCLPU7BZC2MGVJYLVQ/node_modules/chai/lib/chai/utils/proxify.js:86:22)
>>   at Immediate.<anonymous> (/home/lisk/workspace/lisk-core_PR-708-EDN5H7BPFPHQW4UHUSL4OK4TW2RIYZ4Z52RCLPU7BZC2MGVJYLVQ/test/unit/logic/transactionPool.js:108:23)
>>   at runCallback (timers.js:676:20)
>>   at tryOnImmediate (timers.js:645:5)
>>   at processImmediate [as _immediateCallback] (timers.js:617:5)
>> Exited with code: 7.
>> Error executing child process: Error: Process exited with code 7.
Warning: Task "exec:coverageSingle" failed.� Use --force to continue.

Isabello

@vekexasia please synchronize your branch with 1.0.0 and resolve the mentioned issues.

Isabello · 2017-08-16T11:44:12Z

This PR is superceded by #725

Thank you for the hard work.

isabello and others added 3 commits July 27, 2017 11:41

Merge branch '1.0.0' into development

da3b4d9

Merge branch '1.0.0' into development

8262714

Fix transaction pool bug in "transactionInPool" plus tests.

62d48ed

Isabello self-assigned this Aug 9, 2017

Isabello requested a review from a team August 9, 2017 08:27

Isabello added the pending review label Aug 9, 2017

Isabello changed the base branch from development to 1.0.0 August 9, 2017 08:28

karmacoma changed the title ~~Fix transaction pool bug in "transactionInPool" plus tests.~~ Fix transaction pool bug in "transactionInPool" plus tests Aug 9, 2017

karmacoma added the type: bug label Aug 9, 2017

karmacoma suggested changes Aug 9, 2017

View reviewed changes

karmacoma changed the title ~~Fix transaction pool bug in "transactionInPool" plus tests~~ Fix transaction pool bug in "transactionInPool" plus tests - Fixes #707 Aug 9, 2017

4miners reviewed Aug 9, 2017

View reviewed changes

Isabello suggested changes Aug 9, 2017

View reviewed changes

fixed eslint + removed let

d29dee6

diego-G changed the title ~~Fix transaction pool bug in "transactionInPool" plus tests - Fixes #707~~ Fix transaction pool bug in "transactionInPool" plus tests - Closes #707 Aug 9, 2017

MaciejBaj reviewed Aug 10, 2017

View reviewed changes

MaciejBaj suggested changes Aug 10, 2017

View reviewed changes

removed comments and used some instead of filter.

8f84232

4miners suggested changes Aug 14, 2017

View reviewed changes

MaciejBaj reviewed Aug 14, 2017

View reviewed changes

fixed wrong implementation of some check and removed rounds from tr…

70dfa20

…ansactionPool tests.

4miners suggested changes Aug 14, 2017

View reviewed changes

vekexasia and others added 6 commits August 14, 2017 16:05

removed useless code.

7693443

rounds are not used anymore.

ee798a1

Standardising requires

01f386b

Normalising space

65c01af

Standardising comments

11dc29b

Normalising space

7a0a767

Adding use strict

7d4051a

karmacoma approved these changes Aug 14, 2017

View reviewed changes

Isabello suggested changes Aug 15, 2017

View reviewed changes

karmacoma removed type: bug security labels Aug 15, 2017

Isabello closed this Aug 16, 2017

karmacoma added status: invalid and removed pending review labels Aug 16, 2017

karmacoma unassigned Isabello Dec 11, 2017

karmacoma self-assigned this Apr 15, 2019

karmacoma removed their assignment Dec 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix transaction pool bug in "transactionInPool" plus tests - Closes #707 #708

Fix transaction pool bug in "transactionInPool" plus tests - Closes #707 #708

vekexasia commented Aug 9, 2017

karmacoma left a comment

karmacoma Aug 9, 2017

vekexasia Aug 9, 2017

karmacoma Aug 9, 2017

4miners Aug 9, 2017

karmacoma Aug 9, 2017

karmacoma Aug 9, 2017

vekexasia Aug 9, 2017

4miners Aug 9, 2017

vekexasia Aug 9, 2017

4miners Aug 9, 2017

MaciejBaj Aug 10, 2017

4miners Aug 9, 2017

Isabello commented Aug 9, 2017 •

edited

Loading

Isabello left a comment

MaciejBaj Aug 10, 2017

MaciejBaj left a comment

vekexasia commented Aug 11, 2017

4miners Aug 14, 2017

MaciejBaj Aug 14, 2017

vekexasia Aug 14, 2017

4miners Aug 14, 2017

vekexasia Aug 14, 2017

4miners Aug 14, 2017

vekexasia Aug 14, 2017

4miners Aug 14, 2017

4miners Aug 14, 2017

4miners Aug 14, 2017

Isabello commented Aug 15, 2017

Isabello left a comment

Isabello commented Aug 16, 2017 •

edited

Loading

Fix transaction pool bug in "transactionInPool" plus tests - Closes #707 #708

Fix transaction pool bug in "transactionInPool" plus tests - Closes #707 #708

Conversation

vekexasia commented Aug 9, 2017

karmacoma 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

Isabello commented Aug 9, 2017 • edited Loading

Isabello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaciejBaj left a comment

Choose a reason for hiding this comment

vekexasia commented Aug 11, 2017

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

Isabello commented Aug 15, 2017

Isabello left a comment

Choose a reason for hiding this comment

Isabello commented Aug 16, 2017 • edited Loading

Isabello commented Aug 9, 2017 •

edited

Loading

Isabello commented Aug 16, 2017 •

edited

Loading