fix: bounty tests, superceeds #799 #1112
Conversation
| assert.isFalse(reClaimedBounty); | ||
| } | ||
| try { | ||
| await bounty.withdrawPayments({ from: researcher }); |
There was a problem hiding this comment.
no idea what this was testing; the transaction actually goes through but obviously withdraws 0 wei. this is covered in pullpayment test, so I removed it here
nventuro
left a comment
nventuro
left a comment
There was a problem hiding this comment.
Looking good, thanks! The old test was a bit bizarre tbh.
I find the whole Bounty contract a bit confusing, and I'm not sure the mock contracts (SecureBountyTarget and InsecureBountyTarget) help in this regard. I wonder if a single mock, with a public function that 'broke' it (a la ConditionalEscrowMock) would be better suited for this test (requiring the researcher to 'break' the target before claiming the reward).
test/Bounty.test.js
Outdated
| let reward = web3.toWei(1, 'ether'); | ||
| let bounty = await SecureTargetBounty.new(); | ||
| await sendReward(owner, bounty.address, reward); | ||
| it('empties itself when destroyed', async function () { |
There was a problem hiding this comment.
Not sure if we should be testing this: this is not Bounty's code, but Destructible's.
test/Bounty.test.js
Outdated
| let reward = web3.toWei(1, 'ether'); | ||
| let bounty = await SecureTargetBounty.new(); | ||
| await sendReward(owner, bounty.address, reward); | ||
| it('sets reward', async function () { |
There was a problem hiding this comment.
I'd remove this it altogether, it's already being tested for in all other test cases (and if we were to add it, we should have it on both describes).
There was a problem hiding this comment.
I think it's ok to be explicit here. I've refactored the tests in the next commit to make this cleaner
test/Bounty.test.js
Outdated
| const event = await expectEvent.inLogs( | ||
| result.logs, | ||
| 'TargetCreated' | ||
| ); |
There was a problem hiding this comment.
nit, but why the newlines? The same line on the other it doesn't have them.
test/Bounty.test.js
Outdated
| balance.should.be.bignumber.eq(reward); | ||
|
|
||
| await this.bounty.claim(targetAddress, { from: researcher }); | ||
| const claim = await this.bounty.claimed.call(); |
There was a problem hiding this comment.
Can't you write .claimed()?
|
|
||
| await this.bounty.withdrawPayments({ from: researcher }); | ||
| const updatedBalance = await ethGetBalance(this.bounty.address); | ||
| updatedBalance.should.be.bignumber.eq(0); |
There was a problem hiding this comment.
We should also test:
a) that the researcher got the reward (curr - prev balances = reward)
b) that the bounty is no longer payable after claiming
2 participants
replaces #799