issue #57: Async await tests #97
Conversation
| let token = await BasicTokenMock.new(accounts[0], 100); | ||
| let totalSupply = await token.totalSupply(); | ||
| assert.equal(totalSupply, 100); | ||
| done(); |
There was a problem hiding this comment.
No need to call done here, and we can remove it from the parameter too.
There was a problem hiding this comment.
I have 2 or 3 tests that require the done. I basically went with consistency. Ill remove the done from all the other tests.
There was a problem hiding this comment.
Can you point me towards these tests? Mention me on the appropriate line.
There was a problem hiding this comment.
@federicobond Its the ones in bounty.js test. Its because those are events.
There was a problem hiding this comment.
Yes, it's fine to use done when you have to listen to events.
| assert.equal(firstAccountBalance, 0); | ||
| let secondAccountBalance = await token.balanceOf(accounts[1]); | ||
| assert.equal(secondAccountBalance, 100); | ||
| done(); |
There was a problem hiding this comment.
Same here with done. We can also add an empty line before each let/assert to improve readability.
| let transfer = await token.transfer(accounts[1], 101); | ||
| } catch(error) { | ||
| if (error.message.search('invalid JUMP') === -1) throw error | ||
| assert.isAbove(error.message.search('invalid JUMP'), -1, 'Invalid JUMP error must be returned'); |
There was a problem hiding this comment.
We should write a quick helper for asserting throws, as it will be used many times.
|
@federicobond ok, I addressed your comments. |
| it("checkInvariant returns false", async function(){ | ||
| let bounty = await InsecureTargetBounty.new(); | ||
| let target = await bounty.createTarget(); | ||
| let invarriantCall = await bounty.checkInvariant.call(); |
|
@sogoiii there is a failing test case for |
|
All tests passed locally for me. Weird. Is any test non-deterministic? |
|
@federicobond I was on node 6.3.1, which makes that error go away. I rememeber as i was doing this, i found an issue that suggested the downgrade. I currently cannot find that issue. But that was why i changed my node version. |
|
Hmm... I Dont know the combination of testrpc and node to make this work. |
|
@federicobond can you try truffle test instead of npm test? |
|
Yes, it works with my globally installed |
|
Yea, ill keep looking for the problem. Glad im not crazy about that npm and truffle difference. |
|
@federicobond @maraoz Looks like if i change the version of solc from 0.4.4 to either 0.4.5 or 0.4.6, the error appears. Looking at their repo, the only thing they added between 0.4.4 and 0.4.5 is this commit Just to be clear on my procedure. On my global truffle folder, i hardcoded the 0.4.4 and ran a successful |
|
@sogoiii thanks for the PR and the research on the problem with travis! |
|
sweet! |
issue OpenZeppelin#57: Async await tests
Transformed all tests to async await pattern.
Tests are now consistent ---> #57