JoinColony / solcover

Code coverage for solidity
MIT License
64 stars 8 forks source link

Preprocessor improvements #34

Closed area closed 7 years ago

area commented 7 years ago

Depends on the new solution to #28, so not to be merged until that is.

Introduces an improvement to the preprocessor and instrumentation in general. The preprocessor improvement is the fix to #32.

The instrumentation improvement derived from how I saw SolCover behaving with statements immediately following comments.

Previously, we traversed the whole AST, and when we encountered a statement we checked if we could instrument it i.e. if it was standalone. This introduced a bunch of cases that we wanted to check (is it in a block? it is immediately after a block? etc.) and some edge cases that didn’t work properly (comments).

Instead, we now stop traversing the AST in situations where we know we can’t instrument, and assume that if we find a statement that we can instrument it. We can’t instrument f(x) easily (see below) in these situations, so we don’t traverse them:

Of course, going forward we might ‘decide’ that we can in fact instrument these. e.g. arr[f(x)] could be replaced by

var _some_unique_id = f(x);
arr[_some_unique_id];

and then instrumented as appropriate.

area commented 7 years ago

Those tests all pass locally for me, so I'm not sure what's going on here....

cgewecke commented 7 years ago

@area the failing tests on CI are because of this PR was just merged at solidity-parser. I pegged SP to master because it had bug fixes that weren't published to npm but that's obviously a bad idea. I think just getting SP off master in the package will run everything clear for this PR. And i'll open another branch right now that makes the changes necessary for SP #57 to work with solcover.

area commented 7 years ago

Ah, okay. I'm fine with pinning to master on solidity-parser, but it should probably be a specific commit / tag.

cgewecke commented 7 years ago

@area Sorry - missed that last comment. I just opened a PR for solcover master that is a temporary patch for the SP problem. I just used the result of npm install solidity-parser which is ^0.2.0. I'll push a change here now but pinned to 0.2.0.

cgewecke commented 7 years ago

Highlighting is perfect from what I can see. 👍 Really good. I'm looking at instanbul and solcover reports side by side for a bunch of if / else / loop code and solcover is actually slightly more accurate.

area commented 7 years ago

Rebased, so you'll need to git pull --rebase if you want to work on this branch.

cgewecke commented 7 years ago

@area I have nothing to add here. I'll happily write some unit tests for any instrumentSolidity.js lines that aren't getting codecov coverage when you merge this though. Looks really good.