Create tests for Shareable #100
Conversation
adklempner
force-pushed
the
shareable-test
branch
from
f7ebd64 to
0ec09f4
Compare
|
|
||
| function isOwnerConst(address _addr) constant returns (bool) { | ||
| return isOwner(_addr); | ||
| } |
There was a problem hiding this comment.
Why did you need to add this function?
There was a problem hiding this comment.
Calling isOwner() in web3js does not return the function's return value because it's not const. Instead, it returns the transaction hash. Adding the isOwnerConst() function to the mock contract allows me to get around that.
There was a problem hiding this comment.
Shouldn't isOwner be constant too? Yes, just checked and it makes no state changes.
Can you make that change before we merge? :)
There was a problem hiding this comment.
What I mean is, make isOwner constant, remove isOwnerConst from ShareableMock, and use isOwner directly in tests. In the future, feel free to change the base contracts when you're writing tests if you find this problems.
We want the code to be as simple/correct as possible.
| let hash = 1234; | ||
|
|
||
| let initCount = await shareable.count(); | ||
| initCount = initCount.toString(); |
There was a problem hiding this comment.
Using a string here seems confusing. What's the reason?
There was a problem hiding this comment.
I double checked and it doesn't make a difference. all the toString()'s in this test method can be removed.
| let hash = 1234; | ||
|
|
||
| let initCount = await shareable.count(); | ||
| initCount = initCount.toString(); |
|
Added a comment, and please re-check tests that seem to be failing |
|
Looks like some of those toString() calls are necessary. Comparing the objects is what's causing the tests to fail: |
|
LGTM |
Create tests for Shareable
No description provided.