All tests now use account names, and dont use accounts[0] (except ERC… #1137
Conversation
test/ownership/HasNoEther.test.js
Outdated
| assert.equal(finalBalance, 0); | ||
| assert.isAbove(ownerFinalBalance, ownerStartBalance); | ||
| ownerFinalBalance.should.be.bignumber.gt(ownerStartBalance); |
There was a problem hiding this comment.
I know this doesn't belong here, but the previous test (assert.isAbove) was not actually working, since bignumbers are not supported.
|
Not sure what's up with coverage: coveralls reports it dropped on HasNoTokens, though I have no idea what caused that. |
|
yeah no idea about HasNoTokens; maybe the tests are silently failing so solc-coverage never sees that path executed? |
|
I think it may be an issue on their end? I tried testing locally but failed to do so, and noticed there's some strange mocks-related behavior. Anyway, I opened an issue: if there's indeed something that we need to fix, we should do it in a separate PR. |
| hasNoTokens = await HasNoTokens.new(); | ||
| token = await ERC223TokenMock.new(accounts[0], 100); | ||
| hasNoTokens = await HasNoTokens.new({ from: owner }); | ||
| token = await ERC223TokenMock.new(initialAccount, 100); |
There was a problem hiding this comment.
Hi @nventuro, visiting from solidity-coverage.
Is it possible an assumption changed here? The first ERC223 constructor arg is not identical to the from address of the HasNoTokens creation as before.
The test below (for the missing coverage) just throws an error - maybe it's a different one, e.g. it's hitting a revert somewhere else in the inheritance chain?
We haven't published any changes over there for a bit and all your recent CI runs look good. . .
There was a problem hiding this comment.
Ha, I think you're right! Only InitialAccount will have balance, and the other test fails on the transfer call. Thank you so much!
Test files now use explicit account names (
owner,minter,anyone, etc.), and don't use the first account. Behaviors receive all explicit actors (owner, etc.), plus an array of accounts they're free to use as they see fit. This will be included in the upcoming test style guide (#1077).The ERC721 tests are a bit of a mess, so I didn't tackle those, I think a more thorough refactor is due.
Fixes part of #1091