Adds optional coverage for contracts in node_modules

[alecps]

I made these changes to app.js in order to demonstrate full test coverage of npm imported contracts (such as those from openzeppelin-solidity) in addition to custom contracts, while keeping imported contracts safely in node_modules. In order to activate this feature, you just set the (new) setting "instrumentImports" to true in .solcover.js .

[codecov-io]

Codecov Report

Merging #375 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #375   +/-   ##
=======================================
  Coverage   91.66%   91.66%           
=======================================
  Files           6        6           
  Lines         372      372           
  Branches       79       79           
=======================================
  Hits          341      341           
  Misses         31       31

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c2e687b...8d07e8c. Read the comment docs.

[cgewecke]

@AlecSchaefer Ah interesting! LGTM code-wise...I'm going to pull it down and test it out a bit. A couple questions:

demonstrate full test coverage of npm imported contracts

Could I get your views on the use cases for this? e.g. what does this look like in practice..does the developer maintain a full set of tests for the imports? FWIW Zeppelin's coverage is kept at 100% via their own tests, (although that doesn't diminish the value of this feature at all).

There's also a difficulty here because we are in the middle of writing a new architecture (in #372). At first glance instrumentImports would be challenging to include there. We won't be maintaining a separate node_modules in the coverageEnv. Instead we'll use plugin APIs provided by the host development stack (ex: Truffle) to configure tasks like compilation and testing to use the instrumented contracts. I think instrumenting dependencies might be quite complicated, possibly requiring that we copy them to a special file and then rewrite import paths in the contracts that use them as part of the instrumentation process.

[alecps]

The main use case would be for developers who want or need to be able to say they have full test coverage of all smart contracts used in the project (per the request of their manager, for example). The tests for the imported contracts would be maintained and reviewed by the developer. Though zeppelin contracts are rigorously tested, this might not convince all parties involved in the project that additional checks aren't required, or it may just be easier for someone to type a command and see full coverage than to look up zeppelin's testing practices. Furthermore, this tool could theoretically be used to test any packages that might not be as thoroughly vetted as zeppelin's.

I can't tell for sure how it would integrate with the new architecture, but if the problem is that the artifacts won't be available to determine which contracts to instrument (as they currently are after compilation) the feature could instead take in an explicit array of contracts to instrument (and their relative paths) outside of the contracts folder.

[cgewecke]

@AlecSchaefer Thanks for that explanation.

I can't tell for sure how it would integrate with the new architecture, but if the problem is that the artifacts won't be available to determine which contracts to instrument (as they currently are after compilation) the feature could instead take in an explicit array of contracts to instrument (and their relative paths) outside of the contracts folder.

Ok, I will need to think about this a bit. In the new architecture the state of a user's Truffle project at the moment tests are run for coverage looks something like this:

.coverage_contracts /
.coverage_artifacts /
build (optional) /
contracts /
migrations / 
test / 
node_modules (optional) /
truffle-config.js

...where .coverage_artifacts contains the compiled version of the instrumented contracts in .coverage_contracts. It's possible to ask Truffle to use those alternate sources by setting its directory config variables. I'm not sure I see where to locate instrumented contracts from node_modules in this scheme and resolve the import paths correctly. I feel like we'd end up having to copy & restore node_modules contents or something and tbh I'm a little wary of that.

Do you have any ideas?

Also, would you be interested in opening a companion issue to this PR in the issues just so anyone else who's wondering about this topic can easily find it and weigh in?

[alecps]

Sure thing. I've added the issue here (#376).

I see the problem. It seems like you'd either have to rewrite the import paths as part of instrumentation (as you said) or mess around with the node_modules folder (as you also said). Rewriting during instrumentation might be as easy as just appending './' to the beginning of the import paths, which sounds annoying but maybe less so than modifying and restoring node_modules. I'll think about it some more and follow up if I have other ideas.

[alcuadrado]

I think instrumenting dependencies might be quite complicated, possibly requiring that we copy them to a special file and then rewrite import paths in the contracts that use them as part of the instrumentation process.

I can't tell about Truffle, but in Buidler instrumentation could be implemented by modifying solidity's standard json input (i.e. overriding this task). That way no file path change would be needed, and including dependencies would be trivial.

[cgewecke]

@alcuadrado Oh yes! it looks like the entire instrumentation step can happen there. That's great.

[alcuadrado]

@alcuadrado Oh yes! it looks like the entire instrumentation step can happen there. That's great.

The only thing that may need some extra work is distinguishing local from non-local contract files. In Buidler you can identify the local ones because they start with `${path.relative(config.paths.root, config.paths.sources)}/`