Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Increase coverage #149

Merged
merged 18 commits into from
Dec 3, 2020
Merged

Increase coverage #149

merged 18 commits into from
Dec 3, 2020

Conversation

bingen
Copy link
Collaborator

@bingen bingen commented Dec 1, 2020

For issue #74

@bingen bingen requested a review from RickGriff December 1, 2020 20:35
@bingen bingen self-assigned this Dec 1, 2020
@bingen bingen mentioned this pull request Dec 1, 2020
19 tasks

await web3.eth.sendTransaction({ to: defaultPool.address, from: owner, value: amount })

await th.assertRevert(defaultPool.sendETH(nonPayable.address, amount, { from: owner }), 'DefaultPool: sending ETH failed')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm does this revert because defaultPool.sendETH is only callable TroveManager though? If so, it wouldn't hit the final _require(success, ...) that we want to test here

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, but no, because owner is the TroveManager here 😉 :
https://github.com/liquity/dev/pull/149/files#diff-ff756b67b4b4fded33dc971414822ef458d9dc958a1ee0bb38f11c99d6fc4daaR17

Anyway, this highlights again the importance of revert messages. While testing these PRs I restore the message check in testHelpers, but I may forget it sometimes. Hopefully we can have issue #99 fixed soon! 🤞

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I see. Nice.

Definitely, thanks!

@bingen bingen merged commit 6efffeb into main Dec 3, 2020
@bingen bingen deleted the coverage_2 branch December 3, 2020 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants