-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fix coverage #16
Fix coverage #16
Conversation
grep: "@skip-on-coverage", | ||
invert: true | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This config says:
- Do not instrument or report coverage for the fake contracts (or Migrations)
- Use the same number of accounts as the buidlerEVM run does (using ganache-cli)
- Begin with a high balance - coverage consumes a lot of gas
- Skip some tests that only fail with solidity-coverage
soliditycoverage: { | ||
gas: 9000000, | ||
url: 'http://localhost:8555' | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plugin will set this and it would overwrite this gas value to something bigger because the instrumented contracts are quite a bit larger and the number of opcodes run per tx is greater too.
@@ -104,7 +104,7 @@ describe('DCL Names V2', function() { | |||
|
|||
creationParams = { | |||
...fromDeployer, | |||
gas: 6e6, | |||
//gas: 6e6, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coverage needs to set this value higher. Defining it here overrides the custom provider settings and causes deployments to run out of gas.
If you wanted to keep 6e6
as an upper bound in the conventional test run I believe you could set that in the BuidlerEVM options.
@@ -2217,7 +2217,7 @@ describe('DCL Names V2', function() { | |||
expect(subdomainOwner).to.be.equal(anotherUser) | |||
}) | |||
|
|||
it('reverts when trying to change the resolver by an unauthorized account', async function() { | |||
it('reverts when trying to change the resolver by an unauthorized account [ @skip-on-coverage ]', async function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As noted in a comment in the parent issue at solidity-coverage, these tests fail with InvalidJump instead of revert under coverage.
Tbh I'm not sure if the coverage report is actually affected by skipping these - I had difficulty locating the implementation for the relevant methods in the code here.
Thanks! will take a look |
Hi @nachomazzara :) this is a patch to resolve solidity-coverage 467
This PR is just a guide for installation - please feel free to close if it's not helpful or you'd like re-organize things in a different way. I'll discuss details in the diff below, but the main thing that's done here is to move fake contracts to
contracts/mocks
.SC rewrites and recompiles your contracts to temporary folders in the project root. In this repo, the Solidity entry point for the tests is in the test directory. Those files have hardcoded import paths to
../../contracts/etc.sol
which can't be dynamically updated to discover instrumented sources in the temp folder. At the moment the plugin relies on the conventions ofconfig.paths.contractsDir
to locate instrumented sources correctly and supply them to the compiler.There are things the plugin could do to be more flexible here but they're relatively complicated to implement - so this is a quick fix (sorry).
When initially running
npx buidler test
, Buidler asked to update itself - hence the package-lock.json change.[EDIT] - another thing I noticed is that BuidlerEVM is pretty fast. The coverage run feels slow by comparison.