Stop reporting assert statements as branches (#556)

Author: cgewecke

lib/coverage.js

@@ -9,7 +9,7 @@ class Coverage {
 
   constructor() {
     this.data = {};
-    this.assertData = {};
+    this.requireData = {};
   }
 
   /**
@@ -30,7 +30,7 @@ class Coverage {
       statementMap: {},
       branchMap: {},
     };
-    this.assertData[contractPath] = { };
+    this.requireData[contractPath] = { };
 
     info.runnableLines.forEach((item, idx) => {
       this.data[contractPath].l[info.runnableLines[idx]] = 0;
@@ -44,7 +44,7 @@ class Coverage {
     this.data[contractPath].branchMap = info.branchMap;
     for (let x = 1; x <= Object.keys(info.branchMap).length; x++) {
       this.data[contractPath].b[x] = [0, 0];
-      this.assertData[contractPath][x] = {
+      this.requireData[contractPath][x] = {
         preEvents: 0,
         postEvents: 0,
       };
@@ -76,19 +76,19 @@ class Coverage {
         case 'function':   this.data[contractPath].f[id] = hits;                   break;
         case 'statement':  this.data[contractPath].s[id] = hits;                   break;
         case 'branch':     this.data[contractPath].b[id][data.locationIdx] = hits; break;
-        case 'assertPre':  this.assertData[contractPath][id].preEvents = hits;     break;
-        case 'assertPost': this.assertData[contractPath][id].postEvents = hits;    break;
+        case 'requirePre':  this.requireData[contractPath][id].preEvents = hits;     break;
+        case 'requirePost': this.requireData[contractPath][id].postEvents = hits;    break;
       }
     }
 
     // Finally, interpret the assert pre/post events
-    const contractPaths = Object.keys(this.assertData);
+    const contractPaths = Object.keys(this.requireData);
 
     for (let contractPath of contractPaths){
       const contract = this.data[contractPath];
 
       for (let i = 1; i <= Object.keys(contract.b).length; i++) {
-        const branch = this.assertData[contractPath][i];
+        const branch = this.requireData[contractPath][i];
 
         // Was it an assert branch?
         if (branch && branch.preEvents > 0){

lib/injector.js

@@ -158,8 +158,8 @@ class Injector {
     contract.instrumented = `${start}else { ${injectable}}${end}`;
   }
 
-  injectAssertPre(contract, fileName, injectionPoint, injection, instrumentation) {
-    const type = 'assertPre';
+  injectRequirePre(contract, fileName, injectionPoint, injection, instrumentation) {
+    const type = 'requirePre';
     const id = `${fileName}:${injection.contractName}`;
 
     const {
@@ -179,8 +179,8 @@ class Injector {
     contract.instrumented = `${start}${injectable}${end}`;
   }
 
-  injectAssertPost(contract, fileName, injectionPoint, injection, instrumentation) {
-    const type = 'assertPost';
+  injectRequirePost(contract, fileName, injectionPoint, injection, instrumentation) {
+    const type = 'requirePost';
     const id = `${fileName}:${injection.contractName}`;
 
     const {

lib/parse.js

@@ -39,8 +39,8 @@ parse.FunctionCall = function(contract, expression) {
   // This makes sure we don't instrument a chain of expressions multiple times.
   if (expression.expression.type !== 'FunctionCall') {
     register.statement(contract, expression);
-    if (expression.expression.name === 'assert' || expression.expression.name === 'require') {
-      register.assertOrRequire(contract, expression);
+    if (expression.expression.name === 'require') {
+      register.requireBranch(contract, expression);
     }
     parse[expression.expression.type] &&
     parse[expression.expression.type](contract, expression.expression);

lib/registrar.js

@@ -152,7 +152,7 @@ class Registrar {
 
   /**
    * Registers injections for branch measurements. This generic is consumed by
-   * the `assert/require` and `if` registration methods.
+   * the `require` and `if` registration methods.
    * @param  {Object} contract   instrumentation target
    * @param  {Object} expression AST node
    */
@@ -187,25 +187,25 @@ class Registrar {
   };
 
   /**
-   * Registers injections for assert/require statement measurements (branches)
+   * Registers injections for require statement measurements (branches)
    * @param  {Object} contract   instrumentation target
    * @param  {Object} expression AST node
    */
-  assertOrRequire(contract, expression) {
+  requireBranch(contract, expression) {
     this.addNewBranch(contract, expression);
     this._createInjectionPoint(
       contract,
       expression.range[0],
       {
-        type: 'injectAssertPre',
+        type: 'injectRequirePre',
         branchId: contract.branchId,
       }
     );
     this._createInjectionPoint(
       contract,
       expression.range[1] + 2,
       {
-        type: 'injectAssertPost',
+        type: 'injectRequirePost',
         branchId: contract.branchId,
       }
     );

test/units/assert.js

@@ -16,7 +16,10 @@ describe('asserts and requires', () => {
   beforeEach(() => coverage = new Coverage());
   after(async() => await api.finish());
 
-  it('should cover assert statements as `if` statements when they pass', async function() {
+  // Assert was covered as a branch up to v0.7.11. But since those
+  // conditions are never meant to be fullfilled (and assert is really for smt)
+  // people disliked this...
+  it('should *not* cover assert statements as branches (pass)', async function() {
     const contract = await util.bootstrapCoverage('assert/Assert', api);
     coverage.addContract(contract.instrumented, util.filePath);
     await contract.instance.a(true);
@@ -25,9 +28,7 @@ describe('asserts and requires', () => {
     assert.deepEqual(mapping[util.filePath].l, {
       5: 1,
     });
-    assert.deepEqual(mapping[util.filePath].b, {
-      1: [1, 0],
-    });
+    assert.deepEqual(mapping[util.filePath].b, {});
     assert.deepEqual(mapping[util.filePath].s, {
       1: 1,
     });
@@ -36,9 +37,9 @@ describe('asserts and requires', () => {
     });
   });
 
-  // NB: Truffle replays failing txs as .calls to obtain the revert reason from the return
+  // NB: truffle/contract replays failing txs as .calls to obtain the revert reason from the return
   // data. Hence the 2X measurements.
-  it('should cover assert statements as `if` statements when they fail', async function() {
+  it('should *not* cover assert statements as branches (fail)', async function() {
     const contract = await util.bootstrapCoverage('assert/Assert', api);
     coverage.addContract(contract.instrumented, util.filePath);
 
@@ -48,9 +49,7 @@ describe('asserts and requires', () => {
     assert.deepEqual(mapping[util.filePath].l, {
       5: 2,
     });
-    assert.deepEqual(mapping[util.filePath].b, {
-      1: [0, 2],
-    });
+    assert.deepEqual(mapping[util.filePath].b, {});
     assert.deepEqual(mapping[util.filePath].s, {
       1: 2,
     });