Deal tests #11

naumenkogs · 2018-01-31T01:23:25Z

No description provided.

# Conflicts: # src/lib/eth.js

carchrae · 2018-01-31T01:28:00Z

src/lib/eth.js

- // Workaround, see https://github.com/FrontierFoundry/web3-server-tools/issues/3
- contractInstance.setProvider(provider);
- return contractInstance;
+ options.txParams.to = undefined;


instead of setting this to undefined, might be better to copy the options that are allowed. eg, what if someone passes in data or other junk that will make it crash

you're definitely right.

I was just too happy that I found this bug and completely forgot about it after hotfix. What if I just add it to the list for next week?

maybe we just leave this PR open.

carchrae · 2018-01-31T01:57:27Z

@expede - can you also take a look. no rush, but will be useful to get you up to speed on how we are interacting w smart contracts

This reverts commit 5528b26.

naumenkogs · 2018-02-06T23:14:21Z

closes #5 now
@carchrae

carchrae

looks pretty good. i would say the evm_increaseTime is the important thing to fix. having the test actually wait on wall clock time works, but makes for slower tests.

carchrae · 2018-02-08T00:27:56Z

src/lib/eth.js

+ gas: options.txParams.gas,
+ gasPrice: options.txParams.gasPrice
+ }
+ return contractBody.deploy({arguments: options.params}).send(txParams)


we should get eslint on this project. small things; add const txParams (undefined var otherwise)

i did think perhaps you should check options.txParams exists, otherwise code will throw error. however, thinking about it, it is probably better that it throws the error (and the error will be pretty clear)

carchrae · 2018-02-08T04:14:29Z

src/lib/eth.js

@@ -188,7 +197,7 @@ const Eth = {
 .then(([gasPrice, transactionCount]) => {
 logger.debug('gas price', gasPrice);
 const gasPriceHex = web3.utils.numberToHex(gasPrice);
- const gasLimitHex = web3.utils.numberToHex(68308);
+ const gasLimitHex = web3.utils.numberToHex(683080);


this should probably be defined in an env var or some config.

carchrae · 2018-02-08T04:17:10Z

test/spec/lib/deal.spec.js

+ })
+ .then(result => {
+ assert.equal(result, 0);
+ // I wanted to use plain transfer but it reverts for now


will be fixed by #13 ? - perhaps just put // FIXME: in this comment and suggest the fix

carchrae · 2018-02-08T04:19:28Z

test/spec/lib/deal.spec.js

+ })
+ .then(result => {
+ console.log('Pause 2: ', Date.now())
+ return deal.methods.authorize(investor).send(options.txParams)


there are nicer ways to fastforward the EVM in ganache/testrpc. i haven't looked into it much, but did find it when i had a problem with the start time crashing

https://ethereum.stackexchange.com/questions/21509/truffle-testrpc-time-manipulation

this comment was mean about the use of setTimeout : https://github.com/Finhaven/web3-server-tools/pull/11/files/8908915344fd131b77d50304388abe16ebe839fc#diff-e1eaa7016e07dd7f005e46633acf3b5dR205

Creating a separate issue for it

carchrae · 2018-02-08T04:22:37Z

@expede - can you please review as well.

expede · 2018-02-27T18:32:03Z

test/spec/lib/deal-token.spec.js

+ .then(() => checkBalance(ownerAccount, 100))
+ .then(() => transfer(ownerAccount, userAccount, 33, shouldFail))
+ .then(() => checkBalance(userAccount, 33))
+ .then(() => checkBalance(ownerAccount, 67));


Since each then has no argument dependencies, we could parallelize some of these calls if this test is long-running (noticed the timeout). Promise.all([checkBalance(userAccount, 33), checkBalance(ownerAccount, 67)]) or similar

as you likely see, the ordering here is important for the first two.

yes, you could do the last two (as you suggest). i don't think this will be a big impact on speed though. to be clear, @naumenkogs - she means replacing 126+127 with .then(() => Promise.all([checkBalance(userAccount, 33), checkBalance(ownerAccount, 67)]))

i'm not sure i would do this here (code readability>minimal performance), but in general i agree that calling promises async where you can is good practice.

btw, the timeout is because he is using a setTimeout instead of evm_increaseTime - that would be the slowness you are seeing. i think it should be changed to evm_increaseTime if not already done

I agree that for single tests it may not be a huge improvement, but we did also have to up the mocha timeout. As we get more and more tests with more and more expectations, this will only grow.

RE: readability, Promise.all is ugly but can be made more readable with await: [await foo(), await bar()]

on readability - sure, but i'd stick it in a helper checkBalances to keep the test easy to read. re time, not sure you saw the comment about evm_increaseTime - that is the problem here.

Ah, just seeing the comment above now 👍🏻

Well, I'm not sure evm_increaseTime will help a lot here. I think the timeout is caused by the fact that I have to check "deal time-window" works well. And to be sure, I make windows bigger. It's easy to make mistakes by running unrealistic evm_increaseTime I think. And it's not that long.

to be clear; i am suggesting you use evm_increaseTime to skip the ganache EVM into the future. right now, you are using setTimeout in a few places instead. i believe evm_increaseTime would mean you don't need to use setTimeout (which causes a wall-clock (aka real) time delay )

expede · 2018-02-27T18:36:24Z

test/spec/lib/eth.spec.js

+ })
+ .then(newValueBN => {
+ //new value should be bignum
+ let newValue = newValueBN.toString();


Should be a const

expede · 2018-02-27T18:37:08Z

test/spec/lib/eth.spec.js

+ return simpleContract.methods.value().call()
+ .then(valueBN => {
+ //convert from bignum to string
+ let value = valueBN.toString();


Should be a const

i think we need some .eslint checking in this project

Agreed. I've submitted a PR to practices, and will bring that file here once that's given the 👌🏻

expede · 2018-02-27T18:37:52Z

test/spec/lib/eth.spec.js

 console.log('New balance ', balance.toString());
- assert.equal(balance.toString(), '2')
+ assert.equal(balance.toString(), '0.02')


Are all of these console logs here to debug the test?

Well, mostly yes, but it is also helps to double-check that everything is right by reading looks.

It's not the best practice, but we're in the world where there is no way to distinguish whether the error was "not enough tokens" or "transfer not permitted".

# Conflicts: # test/spec/lib/eth.spec.js

naumenkogs and others added 9 commits January 26, 2018 11:07

some WIP

26f3a27

wip update

86e78c3

restrict files when added as a dependency

3ed743f

WIP refactoring tests

056c5ea

fix

dc6e809

remove async-await

af7bc23

fixes

8d87b31

basic tests implemented

74ca40d

Merge branch 'master' into deal_tests

b119911

# Conflicts: # src/lib/eth.js

carchrae reviewed Jan 31, 2018

View reviewed changes

carchrae requested a review from expede January 31, 2018 01:56

naumenkogs added 6 commits February 5, 2018 12:28

graceful txParams.to fix

b31c411

tests refactoring

b1cbc6c

debug

5528b26

Revert "debug"

f90e750

This reverts commit 5528b26.

time fixes

e5a2126

cleanup

8908915

carchrae reviewed Feb 8, 2018

View reviewed changes

naumenkogs added 3 commits February 20, 2018 11:01

closes #14

870ca43

working on #13

15002c0

code cleanup

2f9b469

expede reviewed Feb 27, 2018

View reviewed changes

This was referenced Feb 28, 2018

deal-token and deal-factory tests using web3 #5

Closed

Transfer event #13

Open

naumenkogs added 3 commits February 28, 2018 14:43

Merge branch 'master' into deal_tests

485b14a

# Conflicts: # test/spec/lib/eth.spec.js

minor refactoring

b240423

proper checks on fail-based tests and refactoring

b94f130

naumenkogs closed this Mar 2, 2018

naumenkogs reopened this Mar 2, 2018

naumenkogs merged commit 86d4841 into master Mar 2, 2018

naumenkogs deleted the deal_tests branch March 2, 2018 01:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deal tests #11

Deal tests #11

naumenkogs commented Jan 31, 2018

carchrae Jan 31, 2018

naumenkogs Jan 31, 2018

carchrae Jan 31, 2018

carchrae commented Jan 31, 2018

naumenkogs commented Feb 6, 2018 •

edited

Loading

carchrae left a comment

carchrae Feb 8, 2018

carchrae Feb 8, 2018

carchrae Feb 8, 2018

carchrae Feb 8, 2018

carchrae Feb 28, 2018

naumenkogs Mar 2, 2018

carchrae commented Feb 8, 2018

expede Feb 27, 2018

carchrae Feb 27, 2018

carchrae Feb 27, 2018

expede Feb 27, 2018

carchrae Feb 27, 2018

expede Feb 27, 2018

naumenkogs Feb 28, 2018

carchrae Feb 28, 2018

expede Feb 27, 2018

expede Feb 27, 2018

carchrae Feb 27, 2018

expede Feb 27, 2018

expede Feb 27, 2018

naumenkogs Feb 28, 2018

Deal tests #11

Deal tests #11

Conversation

naumenkogs commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carchrae commented Jan 31, 2018

naumenkogs commented Feb 6, 2018 • edited Loading

carchrae 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

carchrae commented Feb 8, 2018

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

Choose a reason for hiding this comment

naumenkogs commented Feb 6, 2018 •

edited

Loading