Added tests of PublicResolver. #84
Conversation
test/publicresolver_test.js
Outdated
| }) | ||
| .then(contract => { | ||
| resolver = Promise.promisifyAll(contract); | ||
| ens.setSubnodeOwnerAsync(0, web3.sha3('eth'), accounts[0], {from: accounts[0]}); |
There was a problem hiding this comment.
Shouldn't there be a return statement here?
test/publicresolver_test.js
Outdated
|
|
||
| describe('constructor', function() { | ||
|
|
||
| it('should not allow a 0 address for ens'); |
There was a problem hiding this comment.
Shouldn't there be a test in the constructor making sure that it was not passed the address 0 for ens?
There was a problem hiding this comment.
I don't think that's any different to passing any other invalid address for ENS; if you do that, the contract simply won't function.
There was a problem hiding this comment.
Yes, it would not function, but since you cannot change the ens address in the resolver instance, it is more damaging than just setting a resolved address to zero.
There was a problem hiding this comment.
Right, but the same goes for deploying it with an invalid address for ENS; either way, your only option is to redeploy the contract. Further, if we did have this check, it would cause the deployment to fail, leading to the exact same outcome - a moribund contract.
test/publicresolver_test.js
Outdated
|
|
||
| it.only('uses precise gas', function() { | ||
| return web3.eth.getTransactionReceiptAsync(resolver.transactionHash) | ||
| .then(receipt => assert.equal(receipt.gasUsed, 349348)); |
There was a problem hiding this comment.
I don't think this is a good test; it's brittle, and doesn't test anything useful.
There was a problem hiding this comment.
I see its usefulness in 2 places:
- it informs on the gas requirements if another contract needs to deploy a resolver.
- it ensures that the contract has no crazy gas requirement on deployment.
|
|
||
| }); | ||
|
|
||
| describe('has function', function() { |
There was a problem hiding this comment.
I have some bad news: has is obsolete, and is left in for legacy purposes. Thanks for testing it so thoroughly, though! :)
test/publicresolver_test.js
Outdated
| this.slow(200); | ||
| return resolver.setAddrAsync(utils.node, accounts[1], {from: accounts[0]}) | ||
| .then(web3.eth.getTransactionReceiptAsync) | ||
| .then(receipt => assert.equal(receipt.gasUsed, 46163)); |
There was a problem hiding this comment.
I don't think these precise gas tests are valuable either.
|
Can you remove the 'exact gas' tests? Other than that, this looks great. |
No description provided.