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

chore: upgrade solidity-coverage to v0.6+ #541

Merged
merged 4 commits into from
Jul 15, 2019
Merged

Conversation

sohkai
Copy link
Contributor

@sohkai sohkai commented Jul 13, 2019

Upgrades solidity-coverage.

A number of tests invoking the fallback function are now failing with an out of gas on the new version, likely due to new instrumentation rules. Previously they were operating within the limit, even under instrumentation.

cc @cgewecke

@coveralls
Copy link

coveralls commented Jul 13, 2019

Coverage Status

Coverage decreased (-0.3%) to 99.265% when pulling 5706335 on upgrade-solidity-coverage into c85d34e on next.

@cgewecke
Copy link
Contributor

cgewecke commented Jul 13, 2019

@sohkai Taking a look today... send and transfer continue to fail on our tests as well even though the updated coverage client (6.4.5) uses a new vm option which should make instrumentation free. For example these tests at zeppelin run a gas check for ERC165 interfaces and seem ok.

@cgewecke
Copy link
Contributor

cgewecke commented Jul 14, 2019

@sohkai Could you retry with 0.6.2 when you have a chance? SC's send/transfer tests pass now and hopefully you can unskip all of yours as well.

Also left some notes about failing coverage jobs for aragon-apps at this issue thread in solidity-coverage.

@sohkai
Copy link
Contributor Author

sohkai commented Jul 14, 2019

@cgewecke Can confirm with the new version that all the ETH transfers are working under coverage 🎉.

Thanks so much for helping out on this!

Copy link
Contributor

@facuspagnuolo facuspagnuolo left a comment

Choose a reason for hiding this comment

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

Hell yea!!! 🤟🤟🤟🤟

@facuspagnuolo
Copy link
Contributor

Let's create an issue to tackle the uncovered lines

@sohkai
Copy link
Contributor Author

sohkai commented Jul 15, 2019

@facuspagnuolo Not sure why coverage decreased (if anything, not skipping those tests should increase it...), but we do have #514 for the few areas of coverage we previously missed :).

Copy link
Contributor

@izqui izqui left a comment

Choose a reason for hiding this comment

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

🙌🙌🙌

@sohkai sohkai merged commit fbf0fad into next Jul 15, 2019
@sohkai sohkai deleted the upgrade-solidity-coverage branch July 15, 2019 15:23
@dapplion dapplion mentioned this pull request Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants