8000 Improve test coverage by skozin · Pull Request #3 · banteg/lido-vault · GitHub
[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

Improve test coverage #3

Merged
merged 16 commits into from
Dec 24, 2020
Merged

Improve test coverage #3

merged 16 commits into from
Dec 24, 2020

Conversation

skozin
Copy link
Contributor
@skozin skozin commented Dec 23, 2020 •
edited
Loading

This PR aims to increase test coverage, including the ERC20 spec.

skozin and others added 16 commits December 22, 2020 22:12
@skozin skozin marked this pull request as ready for review December 23, 2020 22:27
@skozin
Copy link
Contributor Author
skozin commented Dec 23, 2020 •
edited
Loading

@banteg done! Due to the gas optimization in 313f400 (feat: cheaper transfer from self), we had to disable two ERC20 tests related to transferring to the same address via transferFrom:

Feel free to modify these two tests or remove them altogether if you think this behavior is expected.

@skozin
Copy link
Contributor Author
skozin commented Dec 23, 2020 •
edited
Loading

The coverage:

image

The less-than-full coverage in most cases is due to uncovered branches in assertions like this:

assert ERC20(steth).transfer(recipient, tokens)

stETH always throws on failure instead of returning False so the assertion itself never throws.

@skozin skozin changed the title Extend test coverage improve test coverage Dec 23, 2020
@skozin skozin changed the title improve test coverage Improve test coverage Dec 23, 2020
@banteg banteg merged commit 831166f into banteg:master Dec 24, 2020
@skozin skozin deleted the test/extend-coverage branch December 24, 2020 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skozin @banteg @ujenjt
0