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

Fix coverage #16

Merged
merged 1 commit into from
Jan 15, 2020
Merged

Fix coverage #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .solcover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
module.exports = {
skipFiles: ['mocks', 'utils/Migrations.sol'],
providerOptions: {
default_balance_ether: 1000000000,
total_accounts: 20
},
mocha: {
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

6 changes: 0 additions & 6 deletions buidler.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,5 @@ module.exports = {
enabled: true,
runs: 200
}
},
networks: {
soliditycoverage: {
gas: 9000000,
url: 'http://localhost:8555'
}
}
Copy link
Contributor Author

@cgewecke cgewecke Jan 14, 2020 •
edited
Loading

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.

}
0 test/contracts/ENSBaseRegistrar.sol → contracts/mocks/ENSBaseRegistrar.sol
View file
Open in desktop
File renamed without changes.
0 test/contracts/ENSPublicResolver.sol → contracts/mocks/ENSPublicResolver.sol
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion test/contracts/ENSRegistry.sol → contracts/mocks/ENSRegistry.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
pragma solidity ^0.5.15;

import "../../contracts/interfaces/IENSRegistry.sol";
import "../interfaces/IENSRegistry.sol";

contract FakeENSRegistryFactory {
address public ensRegistryAddress;
Expand Down
2 changes: 1 addition & 1 deletion test/contracts/FakeDCLController.sol → contracts/mocks/FakeDCLController.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
pragma solidity ^0.5.15;

import "../../contracts/ens/DCLController.sol";
import "../ens/DCLController.sol";


contract FakeDCLController is DCLController {
Expand Down
2 changes: 1 addition & 1 deletion test/contracts/FakeDCLRegistrar.sol → contracts/mocks/FakeDCLRegistrar.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
pragma solidity ^0.5.15;

import "../../contracts/ens/DCLRegistrar.sol";
import "../ens/DCLRegistrar.sol";


contract FakeDCLRegistrar is DCLRegistrar {
Expand Down
26 changes: 13 additions & 13 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions test/Subdomain.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ describe('DCL Names V2', function() {

creationParams = {
...fromDeployer,
gas: 6e6,
//gas: 6e6,
Copy link
Contributor Author

@cgewecke cgewecke Jan 14, 2020 •
edited
Loading

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.

gasPrice: 21e9
}

Expand Down Expand Up @@ -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.

await dclControllerContract.register(subdomain1, user, fromUser)

await assertRevert(
Expand Down Expand Up @@ -2312,15 +2312,15 @@ describe('DCL Names V2', function() {
)
})

it('reverts when trying to set the owner for subdomain by an unauthorized account', async function() {
it('reverts when trying to set the owner for subdomain by an unauthorized account [ @skip-on-coverage ]', async function() {
await dclControllerContract.register(subdomain1, user, fromUser)

await assertRevert(
ensRegistryContract.setOwner(subdomain1Hash, anotherUser, fromHacker)
)
})

it('reverts when trying to set a subnode owner for a domain by an unauthorized account', async function() {
it('reverts when trying to set a subnode owner for a domain by an unauthorized account [ @skip-on-coverage ]', async function() {
await assertRevert(
ensRegistryContract.setSubnodeOwner(
dclDomainHash,
Expand Down