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

Fix coverage #16

Merged
merged 1 commit into from
Jan 15, 2020
Merged

Fix coverage #16

merged 1 commit into from
Jan 15, 2020

Conversation

cgewecke
Copy link
Contributor

@cgewecke cgewecke commented Jan 14, 2020

Hi @nachomazzara :) this is a patch to resolve solidity-coverage 467

Screen Shot 2020-01-14 at 2 20 21 PM

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 of config.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.

grep: "@skip-on-coverage",
invert: true
}
}
Copy link
Contributor Author

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'
}
}
Copy link
Contributor Author

@cgewecke cgewecke Jan 14, 2020

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,
Copy link
Contributor Author

@cgewecke cgewecke Jan 14, 2020

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() {
Copy link
Contributor Author

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.

@nachomazzara
Copy link
Collaborator

Thanks! will take a look

@nachomazzara nachomazzara merged commit 744dcf0 into decentraland:master Jan 15, 2020
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.

Cannot keep mock contracts in test directory
2 participants