sc-forks / solidity-coverage

Code coverage for Solidity smart-contracts
MIT License
980 stars 265 forks source link

Cannot keep mock contracts in test directory #467

Open nachomazzara opened 4 years ago

nachomazzara commented 4 years ago

I have contracts under ./test/helpers/contracts which are not being compiled.

Contracts under . ./contracts folder are compiled ok.

Is there any setting that I am missing?

Using it within buidler

Versions: "buidler": "^0.2.8" "solidity-coverage": "^0.7.1"

buidler.config:

...
usePlugin('@nomiclabs/buidler-truffle5')
usePlugin('solidity-coverage')

module.exports = {
  defaultNetwork: 'buidlerevm',
  solc: {
    version: '0.5.15',
    optimizer: {
      enabled: true,
      runs: 200
    }
  }
}
alcuadrado commented 4 years ago

The reason @nachomazzara's contracts inside test/ are being compiled is that he is using @nomiclabs/buidler-truffle5. This plugin overrides TASK_COMPILE_GET_SOURCE_PATHS.

cgewecke commented 4 years ago

@alcuadrado Ah ok, thanks for solving.

@nachomazzara Is decentraland/avatars-contract a good target to test a fix against?

nachomazzara commented 4 years ago

@cgewecke right! just that repo.

cgewecke commented 4 years ago

@nachomazzara I've debugged this a bit and will open a PR today at avatars which gets most of the tests passing correctly under coverage.

However, I'm seeing a very unusual error in 3 tests for ENS Ecosystem. They are supposed to revert

+ reverts when trying to change the resolver by an unauthorized account
+ reverts when trying to set the owner for subdomain by an unauthorized account
+ reverts when trying to set a subnode owner for a domain by an unauthorized account

Instead, under coverage (which is using 20 ganache generated accounts) there is an Invalid Jump error that looks like this:

AssertionError: Expected "revert", got Error: Returned error: VM Exception while processing transaction: 
invalid JUMP at cad6eba5fdfe5ee65504c43ed6edd21adf904807e32b60b965d1e8887a2971da/9c995c4f935c08d2beb77a53a02e85f4c25f349a:142 
instead: expected 'Returned error: VM Exception while processing transaction: 
invalid JUMP at cad6eba5fdfe5ee65504c43ed6edd21adf904807e32b60b965d1e8887a2971da/9c995c4f935c08d2beb77a53a02e85f4c25f349a:142' to include 'revert'

Have you ever seen this before? I'm not sure if it's something that is being caused by the way coverage instruments these contracts or if it's a defect in the way I've configured things.

cgewecke commented 4 years ago

@alcuadrado It seems like Buidler is already deferring to the Truffle plugin's overload of that internal task. (Which is cool.)

When I look in the .coverage_artifacts temporary folder generated for the coverage run it contains the Fake contracts stored in the test directory.

However, those Solidity files have hardcoded import statements expecting to find sources in the ../../contracts directory. The coverage plugin relies on being able to set paths.sources so that TASK_COMPILE uses instrumented and skipped contracts residing in a temp folder called .coverage_contracts.

This could be resolved by the 'in-flight' instrumentation strategy you proposed in #375 which would also make it possible to cover contracts in node_modules.

Will look into that further - there's a lot of filtering etc that would need to be done to preserve the default expectation that contracts contains the targets you'd like a report about.

cgewecke commented 4 years ago

@nachomazzara Renamed your issue to reflect the core problem - hope that's ok.

nachomazzara commented 4 years ago

Hey, sorry for the late reply!

No, I have never seen that issue before.

Let me know if I can help with something else.

nachomazzara commented 4 years ago

The tests which are failing are related to the ensRegistry. That contract is the only one deployed by its bytecode (the code is in Vyper) https://github.com/decentraland/avatars-contract/blob/master/test/contracts/ENSRegistry.sol#L8.

Maybe it has something to be with it.

cgewecke commented 4 years ago

This was auto-closed by accident...still an issue.

The docs, (esp upgrade guide and FAQ) need to address this topic more aggressively. Almost every installation problem in the beta & 0.7.0 has revolved around hardcoded paths to /build or something similar.