Fix pre/post strategy for modifier coverage (#595)

Author: cgewecke

lib/injector.js

@@ -120,30 +120,20 @@ class Injector {
         injection += `modifier ${this._getModifierIdentifier(item.modifierId)}{ `;
 
         let hash = this._getHash(item.modifierId);
-        let injectable = this._getInjectable(item.contractId, hash, 'modifier-pre');
+        let comment = `modifier-${item.condition}`;
+        let injectable = this._getInjectable(item.contractId, hash, comment);
 
-        instrumentation[hash] = {
-          id: item.branchId,
-          type: 'requirePre',
-          contractPath: item.fileName,
-          hits: 0
-        }
-
-        injection += injectable;
-        injection += `_;`
-
-        hash = this._getHash(item.modifierId);
-        injectable = this._getInjectable(item.contractId, hash, 'modifier-post');
+        // Process modifiers in the same step as `require` stmts in coverage.js
+        let type = (item.condition === 'pre') ? 'requirePre' : 'requirePost';
 
         instrumentation[hash] = {
           id: item.branchId,
-          type: 'requirePost',
+          type: type,
           contractPath: item.fileName,
           hits: 0
         }
 
-        injection += injectable;
-        injection += ` }\n`;
+        injection += `${injectable} _; }\n`;
       }
     }
 
@@ -383,7 +373,8 @@ class Injector {
     const type = 'modifier';
     const contractId = `${fileName}:${injection.contractName}`;
     const modifierId = `${fileName}:${injection.contractName}:` +
-                       `${injection.modifierName}:${injection.fnId}`;
+                       `${injection.modifierName}:${injection.fnId}:` +
+                       `${injection.condition}`;
 
     const {
       start,

lib/registrar.js

@@ -131,15 +131,27 @@ class Registrar {
         // Add modifier branch coverage
         if (!this.enabled.modifiers || expression.isConstructor) continue;
 
-        this.addNewModifierBranch(contract, modifier);
+        this.addNewBranch(contract, modifier);
         this._createInjectionPoint(
           contract,
           modifier.range[0],
           {
             type: 'injectModifier',
             branchId: contract.branchId,
             modifierName: modifier.name,
-            fnId: contract.fnId
+            fnId: contract.fnId,
+            condition: 'pre'
+          }
+        );
+        this._createInjectionPoint(
+          contract,
+          modifier.range[1] + 1,
+          {
+            type: 'injectModifier',
+            branchId: contract.branchId,
+            modifierName: modifier.name,
+            fnId: contract.fnId,
+            condition: 'post'
           }
         );
       }
@@ -209,41 +221,6 @@ class Registrar {
     };
   };
 
-  /**
-   * Registers injections for modifier branch measurements.
-   * @param  {Object} contract   instrumentation target
-   * @param  {Object} expression AST node
-   */
-  addNewModifierBranch(contract, expression) {
-    const startContract = contract.instrumented.slice(0, expression.range[0]);
-    const startline = ( startContract.match(/\n/g) || [] ).length + 1;
-    const startcol = expression.range[0] - startContract.lastIndexOf('\n') - 1;
-
-    contract.branchId += 1;
-
-    // NB locations for if branches in istanbul are zero
-    // length and associated with the start of the if.
-    contract.branchMap[contract.branchId] = {
-      line: startline,
-      type: 'if',
-      locations: [{
-        start: {
-          line: startline, column: startcol,
-        },
-        end: {
-          line: startline, column: startcol,
-        },
-      }, {
-        start: {
-          line: startline, column: startcol,
-        },
-        end: {
-          line: startline, column: startcol,
-        },
-      }],
-    };
-  };
-
   addNewConditionalBranch(contract, expression){
     let start;
     // Instabul HTML highlighting location data...

plugins/resources/plugin.utils.js

@@ -222,13 +222,6 @@ function loadSolcoverJS(config={}){
   coverageConfig.cwd = config.workingDir;
   coverageConfig.originalContractsDir = config.contractsDir;
 
-  if (config.matrix){
-    coverageConfig.measureBranchCoverage = false;
-    coverageConfig.measureFunctionCoverage = false;
-    coverageConfig.measureModifierCoverage = false;
-    coverageConfig.measureStatementCoverage = false;
-  }
-
   // Solidity-Coverage writes to Truffle config
   config.mocha = config.mocha || {};
 

test/sources/solidity/contracts/modifiers/reverting-fn.sol

@@ -0,0 +1,14 @@
+pragma solidity ^0.7.0;
+
+contract Test {
+    bool flag = true;
+
+    modifier m {
+      require(flag);
+      _;
+    }
+
+    function a(bool success) m public {
+        require(success);
+    }
+}

test/sources/solidity/contracts/modifiers/reverting-neighbor.sol

@@ -0,0 +1,23 @@
+pragma solidity ^0.7.0;
+
+contract Test {
+    bool flag = true;
+
+    modifier m {
+      require(true);
+      _;
+    }
+
+    modifier n {
+      require(flag);
+      _;
+    }
+
+    function flip() public {
+      flag = !flag;
+    }
+
+    function a() m n public {
+      uint x = 5;
+    }
+}

test/units/modifiers.js

@@ -144,6 +144,61 @@ describe('modifiers', () => {
     });
   });
 
+  // Case: when first modifier always suceeds but a subsequent modifier succeeds and fails,
+  // there should be a missing `else` branch on first modifier
+  it('should not be influenced by revert from a subsequent modifier', async function() {
+    const contract = await util.bootstrapCoverage('modifiers/reverting-neighbor', api);
+    coverage.addContract(contract.instrumented, util.filePath);
+    await contract.instance.a();
+    await contract.instance.flip();
+
+    try {
+      await contract.instance.a();
+    } catch(e) { /*ignore*/ }
+
+    const mapping = coverage.generate(contract.data, util.pathPrefix);
+
+    assert.deepEqual(mapping[util.filePath].l, {
+      "7":3,"8":3,"12":3,"13":1,"17":1,"21":1
+    });
+    assert.deepEqual(mapping[util.filePath].b, {
+      "1":[3,0],"2":[1,2],"3":[3,0],"4":[1,2]
+    });
+    assert.deepEqual(mapping[util.filePath].s, {
+      "1":3,"2":3,"3":1,"4":1
+    });
+    assert.deepEqual(mapping[util.filePath].f, {
+      "1":3,"2":3,"3":1,"4":1
+    });
+  });
+
+  // Case: when the modifier always suceeds but fn logic succeeds and fails, there should be
+  // a missing `else` branch on modifier
+  it('should not be influenced by revert within the function', async function() {
+    const contract = await util.bootstrapCoverage('modifiers/reverting-fn', api);
+    coverage.addContract(contract.instrumented, util.filePath);
+    await contract.instance.a(true);
+
+    try {
+      await contract.instance.a(false);
+    } catch(e) { /*ignore*/ }
+
+    const mapping = coverage.generate(contract.data, util.pathPrefix);
+
+    assert.deepEqual(mapping[util.filePath].l, {
+      7: 3, 8: 3, 12: 3
+    });
+    assert.deepEqual(mapping[util.filePath].b, {
+      1: [3, 0], 2: [3, 0], 3: [1,2]
+    });
+    assert.deepEqual(mapping[util.filePath].s, {
+      1: 3, 2: 3
+    });
+    assert.deepEqual(mapping[util.filePath].f, {
+      1: 3, 2: 3
+    });
+  });
+
   it('should cover when modifiers are listed with newlines', async function() {
     const mapping = await setupAndRun('modifiers/listed-modifiers');