Solidity code coverage #692

andremedeiros · 2018-08-09T19:50:42Z

Overview

TL;DR
Added an output of a code coverage report for Solidity

Details

This PR brings in code coverage for Solidity in Embark. It does so by registering a new module ("coverage") that will listen to compiler, and block events, and request traces from Geth with the executed paths. For this effect, it outputs <dapp_root>/.embark/coverage.json after a dApp's tests run.

One thing to note is that, due to the lack of information from the transaction and trace request, we have to do some "matching" of the bytecode to the actual contract/method being executed. This is a bit of a naive approach for now, so I'll keep an eye out on a better approach to reach this goal.

PR with embark coverage command will follow.

alaibe

💯 very nice, just a few comments about code style:

Usage of forEach instead of reduce (there is others than just my 2 comments)
Usage of var instead of let

alaibe · 2018-08-10T09:59:10Z

lib/modules/coverage/contract_source.js

+      }
+
+      // +1 here factors in newline characters.
+      this.lineOffsets[line] = this.lineOffsets[line-1] + this.lineLengths[line-1] + 1;


could it be: this.lineOffsets[line] = 2 * this.lineLengths[line-1] + 1;

good eyes, just realized that!

alaibe · 2018-08-10T10:01:27Z

lib/modules/coverage/contract_source.js

+    this.lineCount = this.lineLengths.length;
+
+    this.lineOffsets = [];
+    this.lineLengths.forEach((length, line) => {


I think you could use reduce here

Reduce, as far as I know, only returns one value, right?

alaibe · 2018-08-10T10:06:20Z

lib/modules/solidity/index.js

@@ -90,6 +107,10 @@ class Solidity {
              }
            }
          }
+
+          //self.plugins.emitAndRunActionsForEvent('contracts:compiled:solc', output);


No need for this comment right?

alaibe · 2018-08-10T10:14:59Z

lib/modules/coverage/index.js

+  compileSolc(input) {
+    var sources = {};
+
+    Object.keys(input.sources).forEach((path) => {


You could also use reduce here I believe

alaibe · 2018-08-10T10:18:54Z

test/coverage.js

+
+      var trace = JSON.parse(loadFixture('geth-debugtrace-output-h-5.json'));
+      var coverage = cs.generateCodeCoverage(trace);
+      // dumpToFile(coverage, '/tmp/coverage.json');


What about this comment?

andremedeiros · 2018-08-10T14:18:22Z

Thanks for the review @alaibe!

I think reduce is designed to compute a value based on an array, so it might not be what we need in these cases.

jrainville · 2018-08-10T13:44:11Z

lib/modules/coverage/contract_source.js

+
+    this.lineOffsets = [];
+    this.lineLengths.forEach((length, line) => {
+      if(line == 0) {


Friendly reminder that we try to always use ===. I added it to eslint... but only for area-51 🤦‍♂️

jrainville · 2018-08-10T13:47:14Z

lib/modules/coverage/contract_source.js

+      break;
+    }
+
+    for(var i = locations.start.line; i < this.lineCount; i++) {


Can't you merge these two loops together and add mre ifs to detect the end and start?

I did this originally but it was hard to make sense of what was going on. You'll notice that I offset the second loop so that we're not doing any unnecessary iterations anyway, so this is as optimized as it gets.

jrainville · 2018-08-10T13:48:38Z

lib/modules/coverage/contract_source.js

+    // or surpass the offset for last character.
+    if(!locations.end) {
+      var lastLine = this.lineCount - 1;
+      locations.end = {line: lastLine, column: this.lineLengths[lastLine]};


Can just do this.lineLengths[this.lineCount - 1]
The lastLine variable is a bit redundant.

We use it twice so I figured I'd declare a variable above.

Right, sorry, my bad.

jrainville · 2018-08-10T13:49:16Z

lib/modules/coverage/contract_source.js

+      locations.end = {line: lastLine, column: this.lineLengths[lastLine]};
+    }
+
+    // Istanbul likes lines to be 1-indexed, so we'll increment here before returning.


That's weird, do you know why?

Yes, as per the coverage.json spec:

Location objects are a {start: {line, column}, end: {line, column}} object that describes the start and end of a piece of code. Note that line is 1-based, but column is 0-based.

jrainville · 2018-08-10T13:51:33Z

lib/modules/coverage/contract_source.js

+    }
+  }
+
+  /*eslint complexity: ["error", 27]*/


Holy, that's a big function. Is there a way you could split it?

jrainville · 2018-08-10T14:07:18Z

lib/modules/coverage/contract_sources.js

+  parseSolcOutput(output) {
+    for(var file in output.contracts) {
+      var basename = path.basename(file);
+      var contractSource = this.files[basename];


basename is redundant. Can just do this.files[path.basename(file)];

jrainville · 2018-08-10T14:09:28Z

lib/modules/coverage/contract_sources.js

+
+    for(var file in this.files) {
+      var contractSource = this.files[file];
+      coverageReport[file] = contractSource.generateCodeCoverage(trace);


No need for contractSource. Can just do this.files[file].generateCodeCoverage(trace);

jrainville · 2018-08-10T14:12:47Z

lib/modules/coverage/index.js

+
+    process.on('exit', (_code) => {
+      const coverageReportPath = fs.dappPath('.embark', 'coverage.json');
+      fs.writeFileSync(coverageReportPath, JSON.stringify(self.coverageReport));


Wouldn't this create a race condition where your file write might not finish before the exit?

Not if it's sync. It'll wait for that to finish.

jrainville · 2018-08-10T14:16:59Z

lib/modules/coverage/source_map.js

+
+  subtract(sourceMap) {
+    var length = sourceMap.offset - this.offset;
+    return new SourceMap(this.offset, length);


Redundant variable length

jrainville · 2018-08-10T14:18:24Z

lib/tests/test.js

@@ -120,6 +120,7 @@ class Test {
      ipcRole: 'client'
    });
    this.events.request('deploy:setGasLimit', 6000000);
+    this.engine.startService("codeCoverage");


Picky comment. Can you move this one line up. That way all service starts are in one block.

jrainville · 2018-08-10T14:21:37Z

Really nice stuff. I feel like we could/should create an individual repository for the solidity code coverage part and publish it. I feel like it could be a really good standalone tool.
Though, I think there's some hooks that are embark only? Anyway, I think we should consider it. Then, in our module, we would only need to require the coverage tool.

iurimatias requested review from iurimatias, michaelsbradleyjr, alaibe, emizzle and jrainville August 9, 2018 19:52

Andre Medeiros added 6 commits August 9, 2018 16:10

Add coverage code

839d5bb

wip

a4cd51e

Progress

a3dd031

Progress

99df85d

Cleaning touches

94adf1f

Fix tests

22d809b

andremedeiros force-pushed the features/code-coverage branch from 39291e5 to 22d809b Compare August 9, 2018 20:11

Andre Medeiros added 2 commits August 9, 2018 16:46

Linting things

0ec3ff0

Don't dump temporary file

e10e758

alaibe reviewed Aug 10, 2018

View reviewed changes

iurimatias approved these changes Aug 10, 2018

View reviewed changes

jrainville reviewed Aug 10, 2018

View reviewed changes

Andre Medeiros added 2 commits August 10, 2018 10:40

Address feedback

a77f058

More feedback

17cbfe2

jrainville approved these changes Aug 10, 2018

View reviewed changes

andremedeiros merged commit f860f36 into develop Aug 10, 2018

andremedeiros deleted the features/code-coverage branch August 10, 2018 15:25

cgewecke mentioned this pull request Jul 14, 2019

v0.7.0 Plan (Experimental) sc-forks/solidity-coverage#346

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Solidity code coverage #692

Solidity code coverage #692

andremedeiros commented Aug 9, 2018

alaibe left a comment

alaibe Aug 10, 2018

alaibe Aug 10, 2018

alaibe Aug 10, 2018

andremedeiros Aug 10, 2018

alaibe Aug 10, 2018

alaibe Aug 10, 2018

alaibe Aug 10, 2018

andremedeiros commented Aug 10, 2018

jrainville Aug 10, 2018

jrainville Aug 10, 2018

andremedeiros Aug 10, 2018

jrainville Aug 10, 2018

andremedeiros Aug 10, 2018

jrainville Aug 10, 2018

jrainville Aug 10, 2018

andremedeiros Aug 10, 2018

jrainville Aug 10, 2018

jrainville Aug 10, 2018

jrainville Aug 10, 2018

jrainville Aug 10, 2018

andremedeiros Aug 10, 2018

jrainville Aug 10, 2018

jrainville Aug 10, 2018

jrainville commented Aug 10, 2018

Solidity code coverage #692

Solidity code coverage #692

Conversation

andremedeiros commented Aug 9, 2018

Overview

Details

alaibe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andremedeiros commented Aug 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrainville commented Aug 10, 2018