Merge pull request #79 from sc-forks/feature/assert-require-branches

Cover Assert/Require statements as branches

.gitignore

@@ -6,3 +6,4 @@ coverage/
 node_modules/
 .changelog
 .DS_Store
+package-lock.json

README.md

@@ -92,6 +92,7 @@ Solutions to common issues people run into using this tool:
 + [Running out of time (in mocha)](https://github.com/sc-forks/solidity-coverage/blob/master/docs/faq.md#running-out-of-time-in-mocha)
 + [Using alongside HDWalletProvider](https://github.com/sc-forks/solidity-coverage/blob/master/docs/faq.md#using-alongside-hdwalletprovider)
 + [Integrating into CI](https://github.com/sc-forks/solidity-coverage/blob/master/docs/faq.md#continuous-integration-installing-metacoin-on-travisci-with-coveralls)
++ [Why are asserts and requires highlighted as branch points?](https://github.com/sc-forks/solidity-coverage/blob/master/docs/faq.md#why-has-my-branch-coverage-decreased-why-is-assert-being-shown-as-a-branch-point)
 
 ### Example reports 
 + [metacoin](https://sc-forks.github.io/metacoin/) (Istanbul HTML)

docs/faq.md

@@ -152,3 +152,20 @@ And set up an npm script to run the coverage tool like this:
     "coverage": "SOLIDITY_COVERAGE=true ./node_modules/.bin/solidity-coverage"
 },
 ```
+
+### Why has my branch coverage decreased? Why is assert being shown as a branch point?
+
+`assert` and `require` check whether a condition is true or not. If it is, they allow execution to proceed. If not, they throw, and all changes are reverted. Indeed, prior to [Solidity 0.4.10](https://github.com/ethereum/solidity/releases/tag/v0.4.10), when `assert` and `require` were introduced, this functionality was achieved by code that looked like
+
+```
+if (!x) throw;
+```
+rather than 
+
+```
+require(x)
+```
+
+Clearly, the coverage should be the same in these situations, as the code is (functionally) identical. Older versions of solidity-coverage did not treat these as branch points, and they were not considered in the branch coverage filter. Newer versions *do* count these as branch points, so if your tests did not include failure scenarios for `assert` or `require`, you may see a decrease in your coverage figures when upgrading `solidity-coverage`.
+
+If an `assert` or `require` is marked with an `I` in the coverage report, then during your tests the conditional is never true. If it is marked with an `E`, then it is never false.

lib/coverageMap.js

@@ -17,10 +17,13 @@ module.exports = class CoverageMap {
 
   constructor() {
     this.coverage = {};
+    this.assertCoverage = {};
     this.lineTopics = [];
     this.functionTopics = [];
     this.branchTopics = [];
     this.statementTopics = [];
+    this.assertPreTopics = [];
+    this.assertPostTopics = [];
   }
 
   /**
@@ -42,6 +45,7 @@ module.exports = class CoverageMap {
       statementMap: {},
       branchMap: {},
     };
+    this.assertCoverage[canonicalContractPath] = { };
 
     info.runnableLines.forEach((item, idx) => {
       this.coverage[canonicalContractPath].l[info.runnableLines[idx]] = 0;
@@ -53,6 +57,10 @@ module.exports = class CoverageMap {
     this.coverage[canonicalContractPath].branchMap = info.branchMap;
     for (let x = 1; x <= Object.keys(info.branchMap).length; x++) {
       this.coverage[canonicalContractPath].b[x] = [0, 0];
+      this.assertCoverage[canonicalContractPath][x] = {
+        preEvents: 0,
+        postEvents: 0,
+      };
     }
     this.coverage[canonicalContractPath].statementMap = info.statementMap;
     for (let x = 1; x <= Object.keys(info.statementMap).length; x++) {
@@ -69,13 +77,17 @@ module.exports = class CoverageMap {
     const fnHash = keccakhex('__FunctionCoverage' + info.contractName + '(string,uint256)');
     const branchHash = keccakhex('__BranchCoverage' + info.contractName + '(string,uint256,uint256)');
     const statementHash = keccakhex('__StatementCoverage' + info.contractName + '(string,uint256)');
+    const assertPreHash = keccakhex('__AssertPreCoverage' + info.contractName + '(string,uint256)');
+    const assertPostHash = keccakhex('__AssertPostCoverage' + info.contractName + '(string,uint256)');
 
     this.lineTopics.push(lineHash);
     this.functionTopics.push(fnHash);
     this.branchTopics.push(branchHash);
     this.statementTopics.push(statementHash);
+    this.assertPreTopics.push(assertPreHash);
+    this.assertPostTopics.push(assertPostHash);
 
-    const topics = `${lineHash}\n${fnHash}\n${branchHash}\n${statementHash}\n`;
+    const topics = `${lineHash}\n${fnHash}\n${branchHash}\n${statementHash}\n${assertPreHash}\n${assertPostHash}\n`;
     fs.appendFileSync('./scTopics', topics);
   }
 
@@ -106,8 +118,27 @@ module.exports = class CoverageMap {
         const data = SolidityCoder.decodeParams(['string', 'uint256'], event.data.replace('0x', ''));
         const canonicalContractPath = data[0];
         this.coverage[canonicalContractPath].s[data[1].toNumber()] += 1;
+      } else if (event.topics.filter(t => this.assertPreTopics.indexOf(t) >= 0).length > 0) {
+        const data = SolidityCoder.decodeParams(['string', 'uint256'], event.data.replace('0x', ''));
+        const canonicalContractPath = data[0];
+        this.assertCoverage[canonicalContractPath][data[1].toNumber()].preEvents += 1;
+      } else if (event.topics.filter(t => this.assertPostTopics.indexOf(t) >= 0).length > 0) {
+        const data = SolidityCoder.decodeParams(['string', 'uint256'], event.data.replace('0x', ''));
+        const canonicalContractPath = data[0];
+        this.assertCoverage[canonicalContractPath][data[1].toNumber()].postEvents += 1;
       }
     }
+    // Finally, interpret the assert pre/post events
+    Object.keys(this.assertCoverage).forEach(contractPath => {
+      const contract = this.coverage[contractPath];
+      for (let i = 1; i <= Object.keys(contract.b).length; i++) {
+        const branch = this.assertCoverage[contractPath][i];
+        if (branch.preEvents > 0) {
+          // Then it was an assert branch.
+          this.coverage[contractPath].b[i] = [branch.postEvents, branch.preEvents - branch.postEvents];
+        }
+      }
+    });
     return Object.assign({}, this.coverage);
   }
 };

lib/injector.js

@@ -30,6 +30,19 @@ injector.callEmptyBranchEvent = function injectCallEmptyBranchEvent(contract, fi
     contract.instrumented.slice(injectionPoint);
 };
 
+
+injector.callAssertPreEvent = function callAssertPreEvent(contract, fileName, injectionPoint, injection) {
+  contract.instrumented = contract.instrumented.slice(0, injectionPoint) +
+  '__AssertPreCoverage' + contract.contractName + '(\'' + fileName + '\',' + injection.branchId + ');\n' +
+  contract.instrumented.slice(injectionPoint);
+};
+
+injector.callAssertPostEvent = function callAssertPostEvent(contract, fileName, injectionPoint, injection) {
+  contract.instrumented = contract.instrumented.slice(0, injectionPoint) +
+  '__AssertPostCoverage' + contract.contractName + '(\'' + fileName + '\',' + injection.branchId + ');\n' +
+  contract.instrumented.slice(injectionPoint);
+};
+
 injector.openParen = function injectOpenParen(contract, fileName, injectionPoint, injection) {
   contract.instrumented = contract.instrumented.slice(0, injectionPoint) + '(' + contract.instrumented.slice(injectionPoint);
 };
@@ -54,6 +67,9 @@ injector.eventDefinition = function injectEventDefinition(contract, fileName, in
     'event __FunctionCoverage' + contract.contractName + '(string fileName, uint256 fnId);\n' +
     'event __StatementCoverage' + contract.contractName + '(string fileName, uint256 statementId);\n' +
     'event __BranchCoverage' + contract.contractName + '(string fileName, uint256 branchId, uint256 locationIdx);\n' +
+    'event __AssertPreCoverage' + contract.contractName + '(string fileName, uint256 branchId);\n' +
+    'event __AssertPostCoverage' + contract.contractName + '(string fileName, uint256 branchId);\n' +
+
      contract.instrumented.slice(injectionPoint);
 };
 

lib/instrumenter.js

@@ -186,7 +186,7 @@ instrumenter.instrumentFunctionDeclaration = function instrumentFunctionDeclarat
   }
 };
 
-instrumenter.instrumentIfStatement = function instrumentIfStatement(contract, expression) {
+instrumenter.addNewBranch = function addNewBranch(contract, expression) {
   contract.branchId += 1;
   const startline = (contract.instrumented.slice(0, expression.start).match(/\n/g) || []).length + 1;
   const startcol = expression.start - contract.instrumented.slice(0, expression.start).lastIndexOf('\n') - 1;
@@ -210,6 +210,20 @@ instrumenter.instrumentIfStatement = function instrumentIfStatement(contract, ex
       },
     }],
   };
+};
+
+instrumenter.instrumentAssertOrRequire = function instrumentAssertOrRequire(contract, expression) {
+  instrumenter.addNewBranch(contract, expression);
+  createOrAppendInjectionPoint(contract, expression.start, {
+    type: 'callAssertPreEvent', branchId: contract.branchId,
+  });
+  createOrAppendInjectionPoint(contract, expression.end + 1, {
+    type: 'callAssertPostEvent', branchId: contract.branchId,
+  });
+};
+
+instrumenter.instrumentIfStatement = function instrumentIfStatement(contract, expression) {
+  instrumenter.addNewBranch(contract, expression);
   if (expression.consequent.type === 'BlockStatement') {
     createOrAppendInjectionPoint(contract, expression.consequent.start + 1, {
       type: 'callBranchEvent', branchId: contract.branchId, locationIdx: 0,

lib/parse.js

@@ -27,6 +27,9 @@ parse.CallExpression = function parseCallExpression(contract, expression) {
   // In any given chain of call expressions, only the head callee is an Identifier node
   if (expression.callee.type === 'Identifier') {
     instrumenter.instrumentStatement(contract, expression);
+    if (expression.callee.name === 'assert' || expression.callee.name === 'require') {
+      instrumenter.instrumentAssertOrRequire(contract, expression);
+    }
     parse[expression.callee.type] &&
     parse[expression.callee.type](contract, expression.callee);
   } else {

package.json

@@ -39,12 +39,13 @@
     "eslint-plugin-import": "^2.2.0",
     "eslint-plugin-mocha": "^4.8.0",
     "ethereumjs-account": "~2.0.4",
+    "ethereumjs-vm": "https://github.com/sc-forks/ethereumjs-vm-sc.git#3.0.3",
     "ethereumjs-tx": "^1.2.2",
     "ethereumjs-util": "^5.0.1",
     "istanbul": "^0.4.5",
     "merkle-patricia-tree": "~2.1.2",
     "mocha": "^3.1.0",
-    "solc": "0.4.8",
+    "solc": "0.4.13",
     "truffle": "3.4.4"
   }
 }

test/assert.js

@@ -0,0 +1,110 @@
+/* eslint-env node, mocha */
+
+const path = require('path');
+const getInstrumentedVersion = require('./../lib/instrumentSolidity.js');
+const util = require('./util/util.js');
+const CoverageMap = require('./../lib/coverageMap');
+const vm = require('./util/vm');
+const assert = require('assert');
+
+describe('asserts and requires', () => {
+  const filePath = path.resolve('./test.sol');
+  const pathPrefix = './';
+
+  it('should cover assert statements as if they are if statements when they pass', done => {
+    const contract = util.getCode('assert/Assert.sol');
+    const info = getInstrumentedVersion(contract, filePath);
+    const coverage = new CoverageMap();
+    coverage.addContract(info, filePath);
+
+    vm.execute(info.contract, 'a', [true]).then(events => {
+      const mapping = coverage.generate(events, pathPrefix);
+      assert.deepEqual(mapping[filePath].l, {
+        5: 1,
+      });
+      assert.deepEqual(mapping[filePath].b, {
+        1: [1, 0],
+      });
+      assert.deepEqual(mapping[filePath].s, {
+        1: 1,
+      });
+      assert.deepEqual(mapping[filePath].f, {
+        1: 1,
+      });
+      done();
+    }).catch(done);
+  });
+
+  it('should cover assert statements as if they are if statements when they fail', done => {
+    const contract = util.getCode('assert/Assert.sol');
+    const info = getInstrumentedVersion(contract, filePath);
+    const coverage = new CoverageMap();
+    coverage.addContract(info, filePath);
+
+    vm.execute(info.contract, 'a', [false]).then(events => {
+      const mapping = coverage.generate(events, pathPrefix);
+      assert.deepEqual(mapping[filePath].l, {
+        5: 1,
+      });
+      assert.deepEqual(mapping[filePath].b, {
+        1: [0, 1],
+      });
+      assert.deepEqual(mapping[filePath].s, {
+        1: 1,
+      });
+      assert.deepEqual(mapping[filePath].f, {
+        1: 1,
+      });
+      done();
+    }).catch(done);
+  });
+
+  it('should cover multi-line require statements as if they are if statements when they pass', done => {
+    const contract = util.getCode('assert/RequireMultiline.sol');
+    const info = getInstrumentedVersion(contract, filePath);
+    const coverage = new CoverageMap();
+    coverage.addContract(info, filePath);
+
+    vm.execute(info.contract, 'a', [true, true, true]).then(events => {
+      const mapping = coverage.generate(events, pathPrefix);
+      assert.deepEqual(mapping[filePath].l, {
+        5: 1,
+      });
+      assert.deepEqual(mapping[filePath].b, {
+        1: [1, 0],
+      });
+      assert.deepEqual(mapping[filePath].s, {
+        1: 1,
+      });
+      assert.deepEqual(mapping[filePath].f, {
+        1: 1,
+      });
+      done();
+    }).catch(done);
+  });
+
+  it('should cover multi-line require statements as if they are if statements when they fail', done => {
+      const contract = util.getCode('assert/RequireMultiline.sol');
+      const info = getInstrumentedVersion(contract, filePath);
+      const coverage = new CoverageMap();
+      coverage.addContract(info, filePath);
+
+      vm.execute(info.contract, 'a', [true, true, false]).then(events => {
+        const mapping = coverage.generate(events, pathPrefix);
+        assert.deepEqual(mapping[filePath].l, {
+          5: 1,
+        });
+        assert.deepEqual(mapping[filePath].b, {
+          1: [0, 1],
+        });
+        assert.deepEqual(mapping[filePath].s, {
+          1: 1,
+        });
+        assert.deepEqual(mapping[filePath].f, {
+          1: 1,
+        });
+        done();
+      }).catch(done);
+    });
+
+});

test/conditional.js

@@ -11,8 +11,6 @@ describe('conditional statements', () => {
   const filePath = path.resolve('./test.sol');
   const pathPrefix = './';
 
-  before(() => process.env.NO_EVENTS_FILTER = true);
-
   it('should cover a conditional that reaches the consequent (same-line)', done => {
     const contract = util.getCode('conditional/sameline-consequent.sol');
     const info = getInstrumentedVersion(contract, filePath);

test/expressions.js

@@ -13,8 +13,6 @@ const path = require('path');
 describe('generic expressions', () => {
   const filePath = path.resolve('./test.sol');
 
-  before(() => process.env.NO_EVENTS_FILTER = true);
-
   it('should compile after instrumenting a single binary expression', () => {
     const contract = util.getCode('expressions/single-binary-expression.sol');
     const info = getInstrumentedVersion(contract, filePath);

test/function.js

@@ -17,8 +17,6 @@ describe('function declarations', () => {
   const filePath = path.resolve('./test.sol');
   const pathPrefix = './';
 
-  before(() => process.env.NO_EVENTS_FILTER = true);
-
   it('should compile after instrumenting an ordinary function declaration', () => {
     const contract = util.getCode('function/function.sol');
     const info = getInstrumentedVersion(contract, 'test.sol');
@@ -96,20 +94,20 @@ describe('function declarations', () => {
     const info = getInstrumentedVersion(contract, filePath);
     const coverage = new CoverageMap();
     coverage.addContract(info, filePath);
-    // The vm runs out of gas here - but we can verify line / statement / fn
-    // coverage is getting mapped.
+    // We try and call a contract at an address where it doesn't exist and the VM
+    // throws, but we can verify line / statement / fn coverage is getting mapped.
     vm.execute(info.contract, 'a', []).then(events => {
       const mapping = coverage.generate(events, pathPrefix);
       assert.deepEqual(mapping[filePath].l, {
-        9: 0,
+        9: 1,
       });
       assert.deepEqual(mapping[filePath].b, {});
       assert.deepEqual(mapping[filePath].s, {
-        1: 0,
+        1: 1,
       });
       assert.deepEqual(mapping[filePath].f, {
         1: 0,
-        2: 0,
+        2: 1,
       });
       done();
     }).catch(done);
@@ -125,15 +123,15 @@ describe('function declarations', () => {
     vm.execute(info.contract, 'a', []).then(events => {
       const mapping = coverage.generate(events, pathPrefix);
       assert.deepEqual(mapping[filePath].l, {
-        10: 0,
+        10: 1,
       });
       assert.deepEqual(mapping[filePath].b, {});
       assert.deepEqual(mapping[filePath].s, {
-        1: 0,
+        1: 1,
       });
       assert.deepEqual(mapping[filePath].f, {
         1: 0,
-        2: 0,
+        2: 1,
       });
       done();
     }).catch(done);

test/if.js

@@ -11,8 +11,6 @@ describe('if, else, and else if statements', () => {
   const filePath = path.resolve('./test.sol');
   const pathPrefix = './';
 
-  before(() => process.env.NO_EVENTS_FILTER = true);
-
   it('should cover an if statement with a bracketed consequent', done => {
     const contract = util.getCode('if/if-with-brackets.sol');
     const info = getInstrumentedVersion(contract, filePath);

test/loops.js

@@ -11,8 +11,6 @@ describe('for and while statements', () => {
   const filePath = path.resolve('./test.sol');
   const pathPrefix = './';
 
-  before(() => process.env.NO_EVENTS_FILTER = true);
-
   it('should cover a for statement with a bracketed body (multiline)', done => {
     const contract = util.getCode('loops/for-with-brackets.sol');
     const info = getInstrumentedVersion(contract, filePath);

test/return.js

@@ -5,8 +5,7 @@ const getInstrumentedVersion = require('./../lib/instrumentSolidity.js');
 const util = require('./util/util.js');
 
 describe('return statements', () => {
-  before(() => process.env.NO_EVENTS_FILTER = true);
-
+  
   it('should compile after instrumenting function that returns true', () => {
     const contract = util.getCode('return/return.sol');
     const info = getInstrumentedVersion(contract, 'test.sol');

test/sources/assert/Assert.sol

@@ -0,0 +1,7 @@
+pragma solidity ^0.4.13;
+
+contract Test {
+  function a(bool test){
+    assert(test);
+  }
+}

test/sources/assert/RequireMultiline.sol

@@ -0,0 +1,9 @@
+pragma solidity ^0.4.13;
+
+contract Test {
+  function a(bool a, bool b, bool c){
+    require(a &&
+            b &&
+            c);
+  }
+}

test/sources/cli/Events.sol

@@ -2,13 +2,26 @@ pragma solidity ^0.4.3;
 
 contract Events {
     uint x = 0;
+    bool a;
+    bool b;
     event LogEventOne( uint x, address y);
     event LogEventTwo( uint x, address y);
 
     function test(uint val) {
+        // Assert / Require events
+        require(true);
+        
+        // Contract Events
         LogEventOne(100, msg.sender);
         x = x + val; 
         LogEventTwo(200, msg.sender);
+        
+        // Branch events
+        if (true) {
+            a = false;
+        } else {
+            b = false;
+        }
     }
 
     function getX() returns (uint){

test/sources/function/multiple.sol

@@ -8,7 +8,7 @@ contract Test {
     function f2(uint x){ x = 2; }
 
     address a;
-    
+
     function f3(uint y){
         y = 1;
     }

test/sources/statements/compilation-error.sol

@@ -0,0 +1,6 @@
+pragma solidity ^0.4.3;
+
+contract Test { 
+    address a;
+    address a;   
+}

test/statements.js

@@ -17,8 +17,6 @@ describe('generic statements', () => {
   const filePath = path.resolve('./test.sol');
   const pathPrefix = './';
 
-  before(() => process.env.NO_EVENTS_FILTER = true);
-
   it('should compile after instrumenting a single statement (first line of function)', () => {
     const contract = util.getCode('statements/single.sol');
     const info = getInstrumentedVersion(contract, filePath);
@@ -54,6 +52,18 @@ describe('generic statements', () => {
     util.report(output.errors);
   });
 
+  it('should NOT pass tests if the contract has a compilation error', () => {
+    const contract = util.getCode('statements/compilation-error.sol');
+    const info = getInstrumentedVersion(contract, filePath);
+    const output = solc.compile(info.contract, 1);
+    try {
+      util.report(output.errors);
+      assert.fail('WRONG'); // We shouldn't hit this.
+    } catch (err) {
+      (err.actual === 'WRONG') ? assert(false): assert(true);
+    }  
+  });
+
   it('should cover a statement following a close brace', done => {
     const contract = util.getCode('statements/post-close-brace.sol');
     const info = getInstrumentedVersion(contract, filePath);

test/util/util.js

@@ -11,7 +11,12 @@ module.exports.getCode = function getCode(_path) {
 };
 
 module.exports.report = function report(errors) {
-  if (errors) {
-    throw new Error(`Instrumented solidity invalid: ${errors}`);
-  }
+
+  if (errors){
+    errors.forEach(error => {
+        if (error.indexOf('Warning') === -1) {
+            throw new Error(`Instrumented solidity invalid: ${errors}`);
+        }
+    });
+  } 
 };

test/util/vm.js

@@ -1,4 +1,6 @@
 const solc = require('solc');
+const shell = require('shelljs');
+const fs = require('fs');
 const VM = require('ethereumjs-vm');
 const Account = require('ethereumjs-account');
 const Transaction = require('ethereumjs-tx');
@@ -53,7 +55,7 @@ function getAbi(source, compilation) {
     throw new Error('Could not parse contract name from source.');
   }
   const contractName = contractNameMatch[1];
-  return JSON.parse(compilation.contracts[contractName].interface);
+  return JSON.parse(compilation.contracts[':' + contractName].interface);
 }
 
 /**
@@ -116,15 +118,14 @@ function callMethod(vm, abi, address, functionName, args) {
     vm.runTx({
       tx,
     }, (err, results) => {
-      const seenEvents = [];
-      results.vm.runState.logs.forEach(log => {
-        const toWrite = {};
-        toWrite.address = log[0].toString('hex');
-        toWrite.topics = log[1].map(x => x.toString('hex'));
-        toWrite.data = log[2].toString('hex');
-        seenEvents.push(JSON.stringify(toWrite));
-      });
-      resolve(seenEvents);
+      try {
+        const events = fs.readFileSync('./allFiredEvents').toString().split('\n');
+        events.pop();
+        shell.rm('./allFiredEvents');
+        resolve(events);
+      } catch (err) {
+        resolve([]);
+      }
     });
   });
 }
@@ -139,7 +140,7 @@ function callMethod(vm, abi, address, functionName, args) {
  */
 module.exports.execute = function ex(contract, functionName, args) {
   const output = solc.compile(contract, 1);
-  const code = new Buffer(output.contracts.Test.bytecode, 'hex');
+  const code = new Buffer(output.contracts[':Test'].bytecode, 'hex');
   const abi = getAbi(contract, output);
   const stateTrie = new Trie();
   const vm = new VM({

test/zeppelin.js

@@ -5,8 +5,7 @@ const getInstrumentedVersion = require('./../lib/instrumentSolidity.js');
 const util = require('./util/util.js');
 
 describe('Battery test of production contracts: OpenZeppelin', () => {
-  before(() => process.env.NO_EVENTS_FILTER = true);
-
+  
   it('should compile after instrumenting zeppelin-solidity/Bounty.sol', () => {
     const bounty = getInstrumentedVersion(util.getCode('zeppelin/Bounty.sol'), 'Bounty.sol');
     const ownable = getInstrumentedVersion(util.getCode('zeppelin/Ownable.sol'), 'Ownable.sol');