Add modifier-invocation-as-branch coverage (#588)

README.md

@@ -98,6 +98,7 @@ module.exports = {
 | skipFiles | *Array* | `['Migrations.sol']` | Array of contracts or folders (with paths expressed relative to the `contracts` directory) that should be skipped when doing instrumentation. |
 | measureStatementCoverage | *boolean* | `true` | Computes statement (in addition to line) coverage. [More...][34] |
 | measureFunctionCoverage | *boolean* | `true` | Computes function coverage. [More...][34] |
+| measureModifierCoverage | *boolean* | `true` | Computes each modifier invocation as a code branch. [More...][34] |
 | istanbulFolder | *String* | `./coverage` |  Folder location for Istanbul coverage reports. |
 | istanbulReporter | *Array* | `['html', 'lcov', 'text', 'json']` | [Istanbul coverage reporters][2]  |
 | mocha | *Object* | `{ }` | [Mocha options][3] to merge into existing mocha config. `grep` and `invert` are useful for skipping certain tests under coverage using tags in the test descriptions.|

lib/injector.js

@@ -3,6 +3,7 @@ const web3Utils = require("web3-utils");
 class Injector {
   constructor(){
     this.hashCounter = 0;
+    this.modifiers = {};
   }
 
   _split(contract, injectionPoint){
@@ -18,6 +19,8 @@ class Injector {
         return ` && ${this._getTrueMethodIdentifier(id)}(${hash}))`;
       case 'or-false':
         return ` || ${this._getFalseMethodIdentifier(id)}(${hash}))`;
+      case 'modifier':
+        return ` ${this._getModifierIdentifier(id)} `;
       default:
         return `${this._getDefaultMethodIdentifier(id)}(${hash}); /* ${type} */ \n`;
     }
@@ -43,6 +46,10 @@ class Injector {
     return `c_false${web3Utils.keccak256(id).slice(0,10)}`
   }
 
+  _getModifierIdentifier(id){
+    return `c_mod${web3Utils.keccak256(id).slice(0,10)}`
+  }
+
   _getInjectionComponents(contract, injectionPoint, id, type){
     const { start, end } = this._split(contract, injectionPoint);
     const hash = this._getHash(id)
@@ -104,6 +111,57 @@ class Injector {
     return `function ${method}(bytes32 c__${hash}) public pure returns (bool){ return false; }\n`;
   }
 
+  _getModifierDefinitions(contractId, instrumentation){
+    let injection = '';
+
+    if (this.modifiers[contractId]){
+
+      for (const item of this.modifiers[contractId]){
+        injection += `modifier ${this._getModifierIdentifier(item.modifierId)}{ `;
+
+        let hash = this._getHash(item.modifierId);
+        let injectable = this._getInjectable(item.contractId, hash, 'modifier-pre');
+
+        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');
+
+        instrumentation[hash] = {
+          id: item.branchId,
+          type: 'requirePost',
+          contractPath: item.fileName,
+          hits: 0
+        }
+
+        injection += injectable;
+        injection += ` }\n`;
+      }
+    }
+
+    return injection;
+  }
+
+  _cacheModifier(injection){
+    if (!this.modifiers[injection.contractId]) {
+      this.modifiers[injection.contractId] = [];
+    }
+
+    this.modifiers[injection.contractId].push(injection);
+  }
+
+  resetModifierMapping(){
+    this.modifiers = {};
+  }
+
   injectLine(contract, fileName, injectionPoint, injection, instrumentation){
     const type = 'line';
     const { start, end } = this._split(contract, injectionPoint);
@@ -267,6 +325,7 @@ class Injector {
                             `${defaultMethodDefinition}` +
                             `${this._getTrueMethodDefinition(id)}` +
                             `${this._getFalseMethodDefinition(id)}` +
+                            `${this._getModifierDefinitions(id, instrumentation)}` +
                             `${end}`;
   }
 
@@ -319,6 +378,29 @@ class Injector {
 
     contract.instrumented = `${start}${injectable}${end}`;
   }
+
+  injectModifier(contract, fileName, injectionPoint, injection, instrumentation){
+    const type = 'modifier';
+    const contractId = `${fileName}:${injection.contractName}`;
+    const modifierId = `${fileName}:${injection.contractName}:` +
+                       `${injection.modifierName}:${injection.fnId}`;
+
+    const {
+      start,
+      end,
+      hash,
+      injectable
+    } = this._getInjectionComponents(contract, injectionPoint, modifierId, type);
+
+    this._cacheModifier({
+      contractId,
+      modifierId,
+      fileName,
+      ...injection
+    });
+
+    contract.instrumented = `${start}${injectable}${end}`;
+  }
 };
 
 module.exports = Injector;

lib/instrumenter.js

@@ -17,6 +17,7 @@ class Instrumenter {
     this.injector = new Injector();
     this.measureStatementCoverage = (config.measureStatementCoverage === false) ? false : true;
     this.measureFunctionCoverage = (config.measureFunctionCoverage === false) ? false: true;
+    this.measureModifierCoverage = (config.measureModifierCoverage === false) ? false: true;
   }
 
   _isRootNode(node){
@@ -56,16 +57,19 @@ class Instrumenter {
   instrument(contractSource, fileName) {
     const contract = {};
 
+    this.injector.resetModifierMapping();
+    parse.configureStatementCoverage(this.measureStatementCoverage)
+    parse.configureFunctionCoverage(this.measureFunctionCoverage)
+    parse.configureModifierCoverage(this.measureModifierCoverage)
+
     contract.source = contractSource;
     contract.instrumented = contractSource;
 
     this._initializeCoverageFields(contract);
-    parse.configureStatementCoverage(this.measureStatementCoverage)
-    parse.configureFunctionCoverage(this.measureFunctionCoverage)
 
     // First, we run over the original contract to get the source mapping.
     let ast = SolidityParser.parse(contract.source, {loc: true, range: true});
-    //console.log(JSON.stringify(ast, null, ' '))
+
     parse[ast.type](contract, ast);
     const retValue = JSON.parse(JSON.stringify(contract)); // Possibly apotropaic.
 

lib/parse.js

@@ -19,6 +19,10 @@ parse.configureFunctionCoverage = function(val){
   register.measureFunctionCoverage = val;
 }
 
+parse.configureModifierCoverage = function(val){
+  register.measureModifierCoverage = val;
+}
+
 // Nodes
 parse.AssignmentExpression = function(contract, expression) {
   register.statement(contract, expression);

lib/registrar.js

@@ -14,6 +14,7 @@ class Registrar {
     // These are set by user option and enable/disable the measurement completely
     this.measureStatementCoverage = true;
     this.measureFunctionCoverage = true;
+    this.measureModifierCoverage = true;
   }
 
   /**
@@ -110,14 +111,31 @@ class Registrar {
     if (!this.measureFunctionCoverage) return;
 
     let start = 0;
+    contract.fnId += 1;
 
-    // It's possible functions will have modifiers that take string args
-    // which contains an open curly brace. Skip ahead...
     if (expression.modifiers && expression.modifiers.length){
       for (let modifier of expression.modifiers ){
+
+        // It's possible functions will have modifiers that take string args
+        // which contains an open curly brace. Skip ahead...
         if (modifier.range[1] > start){
           start = modifier.range[1];
         }
+
+        // Add modifier branch coverage
+        if (!this.measureModifierCoverage) continue;
+
+        this.addNewModifierBranch(contract, modifier);
+        this._createInjectionPoint(
+          contract,
+          modifier.range[0],
+          {
+            type: 'injectModifier',
+            branchId: contract.branchId,
+            modifierName: modifier.name,
+            fnId: contract.fnId
+          }
+        );
       }
     } else {
       start = expression.range[0];
@@ -133,7 +151,6 @@ class Registrar {
       start + endlineDelta
     );
 
-    contract.fnId += 1;
     contract.fnMap[contract.fnId] = {
       name: expression.isConstructor ? 'constructor' : expression.name,
       line: startline,
@@ -186,6 +203,41 @@ 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...

lib/validator.js

@@ -22,6 +22,7 @@ const configSchema = {
     istanbulFolder:       {type: "string"},
     measureStatementCoverage: {type: "boolean"},
     measureFunctionCoverage: {type: "boolean"},
+    measureModifierCoverage: {type: "boolean"},
 
     // Hooks:
     onServerReady:        {type: "function", format: "isFunction"},

test/integration/projects/modifiers/.solcover.js

@@ -0,0 +1,8 @@
+// Testing hooks
+const fn = (msg, config) => config.logger.log(msg);
+
+module.exports = {
+  skipFiles: ['Migrations.sol'],
+  silent: process.env.SILENT ? true : false,
+  istanbulReporter: ['json-summary', 'text'],
+}

test/integration/projects/modifiers/contracts/ModifiersA.sol

@@ -0,0 +1,45 @@
+pragma solidity ^0.6.0;
+
+import "./ModifiersB.sol";
+
+/**
+ * New syntaxes in solc 0.6.x
+ */
+contract ModifiersA is ModifiersB {
+  uint counter;
+  bool flag = true;
+
+  modifier flippable {
+    require(flag);
+    _;
+  }
+
+  modifier overridden() override {
+    require(true);
+    _;
+  }
+
+  function flip() public {
+    flag = !flag;
+  }
+
+  function simpleSet(uint i)
+    public
+    override(ModifiersB)
+  {
+    counter = counter + i;
+  }
+
+  function simpleView(uint i)
+    view
+    overridden
+    external
+    returns (uint, bool)
+  {
+    return (counter + i, true);
+  }
+
+  function simpleSetFlip(uint i) flippable public {
+    counter = counter + i;
+  }
+}

test/integration/projects/modifiers/contracts/ModifiersB.sol

@@ -0,0 +1,19 @@
+pragma solidity ^0.6.0;
+
+
+contract ModifiersB {
+  uint value;
+  uint b;
+
+  constructor() public {
+  }
+
+  modifier overridden() virtual {
+    require(true);
+    _;
+  }
+
+  function simpleSet(uint i) public virtual {
+    value = 5;
+  }
+}

test/integration/projects/modifiers/contracts/ModifiersC.sol

@@ -0,0 +1,43 @@
+pragma solidity ^0.6.0;
+
+import "./ModifiersB.sol";
+
+/**
+ * New syntaxes in solc 0.6.x
+ */
+contract ModifiersC {
+  uint counter;
+  address owner;
+  bool flag = true;
+
+  constructor() public {
+    owner = msg.sender;
+  }
+
+  modifier flippable {
+    require(flag);
+    _;
+  }
+
+  function flip() public {
+    flag = !flag;
+  }
+
+  function simpleSetFlip(uint i) flippable public {
+    counter = counter + i;
+  }
+
+  modifier onlyOwner {
+    require(msg.sender == owner);
+    _;
+  }
+
+  function set(uint i)
+    onlyOwner
+    public
+    payable
+    virtual
+  {
+    counter = counter + i;
+  }
+}

test/integration/projects/modifiers/hardhat.config.js

@@ -0,0 +1,9 @@
+require("@nomiclabs/hardhat-truffle5");
+require(__dirname + "/../plugins/nomiclabs.plugin");
+
+module.exports = {
+  solidity: {
+    version: "0.6.7"
+  },
+  logger: process.env.SILENT ? { log: () => {} } : console,
+};

test/integration/projects/modifiers/test/modifiers.js

@@ -0,0 +1,40 @@
+const ModifiersA = artifacts.require("ModifiersA");
+const ModifiersC = artifacts.require("ModifiersC");
+
+contract("Modifiers", function(accounts) {
+  let instance;
+
+  before(async () => {
+    A = await ModifiersA.new();
+    C = await ModifiersC.new();
+  })
+
+  it('simpleSet (overridden method)', async function(){
+    await A.simpleSet(5);
+  });
+
+  it('simpleView (overridden modifier)', async function(){
+    await A.simpleView(5);
+  });
+
+  it('simpleSetFlip (both branches)', async function(){
+    await A.simpleSetFlip(5);
+    await A.flip();
+
+    try {
+      await A.simpleSetFlip(5);
+    } catch (e) {
+      /* ignore */
+    }
+  });
+
+  it('simpleSetFlip (false branch + other file)', async function(){
+    await C.flip();
+
+    try {
+      await C.simpleSetFlip(5);
+    } catch (e) {
+      /* ignore */
+    }
+  });
+});

test/integration/projects/modifiers/truffle-config.js

@@ -0,0 +1,9 @@
+module.exports = {
+  networks: {},
+  mocha: {},
+  compilers: {
+    solc: {
+      version: "0.6.2"
+    }
+  }
+}

test/sources/js/modified.js

@@ -0,0 +1,8 @@
+const Modified = artifacts.require('Modified');
+
+contract('Modified', () => {
+  it('should set counter', async function(){
+    const m = await Modified.new()
+    await m.set(5);
+  });
+});

test/sources/solidity/contracts/app/Modified.sol

@@ -0,0 +1,20 @@
+pragma solidity ^0.7.0;
+
+contract Modified {
+  uint counter;
+
+  modifier m  {
+    _;
+  }
+
+  // When modifier coverage is on, branch cov should be 50%
+  // When off: 100%
+  function set(uint i)
+    m
+    public
+    payable
+    virtual
+  {
+    counter = counter + i;
+  }
+}

test/sources/solidity/contracts/modifiers/both-branches.sol

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

test/sources/solidity/contracts/modifiers/listed-modifiers.sol

@@ -0,0 +1,21 @@
+pragma solidity ^0.7.0;
+
+contract Test {
+    modifier mmm {
+      require(true);
+      _;
+    }
+
+    modifier nnn {
+      require(true);
+      _;
+    }
+
+    function a()
+      mmm
+      nnn
+      public
+    {
+      uint x = 5;
+    }
+}

test/sources/solidity/contracts/modifiers/multiple-fns-same-mod.sol

@@ -0,0 +1,16 @@
+pragma solidity ^0.7.0;
+
+contract Test {
+    modifier mmm {
+      require(true);
+      _;
+    }
+
+    function a() mmm public {
+      uint x = 5;
+    }
+
+    function b() mmm public {
+      uint x = 5;
+    }
+}

test/sources/solidity/contracts/modifiers/multiple-mods-same-fn.sol

@@ -0,0 +1,17 @@
+pragma solidity ^0.7.0;
+
+contract Test {
+    modifier mmm {
+      require(true);
+      _;
+    }
+
+    modifier nnn {
+      require(true);
+      _;
+    }
+
+    function a() mmm nnn public {
+      uint x = 5;
+    }
+}

test/sources/solidity/contracts/modifiers/override-function.sol

@@ -0,0 +1,16 @@
+pragma solidity ^0.7.0;
+
+abstract contract IM {
+  function a() payable virtual public;
+}
+
+contract Test is IM {
+    modifier m {
+      require(true);
+      _;
+    }
+
+    function a() payable m public override {
+      uint x = 5;
+    }
+}

test/sources/solidity/contracts/modifiers/same-contract-fail.sol

@@ -0,0 +1,12 @@
+pragma solidity ^0.7.0;
+
+contract Test {
+    modifier m {
+      require(false);
+      _;
+    }
+
+    function a() m public {
+      uint x = 5;
+    }
+}

test/sources/solidity/contracts/modifiers/same-contract-pass.sol

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

test/units/function.js

@@ -84,7 +84,9 @@ describe('function declarations', () => {
     assert.deepEqual(mapping[util.filePath].l, {
       5: 1, 6: 1, 9: 1,
     });
-    assert.deepEqual(mapping[util.filePath].b, {});
+    assert.deepEqual(mapping[util.filePath].b, {
+      1: [1, 0]
+    });
     assert.deepEqual(mapping[util.filePath].s, {
       1: 1
     });

test/units/hardhat/standard.js

@@ -508,4 +508,41 @@ describe('Hardhat Plugin: standard use cases', function() {
 
     verify.branchCoverage(expected);
   })
+
+  it('modifiers (multi-file)', async function(){
+    mock.installFullProject('modifiers');
+    mock.hardhatSetupEnv(this);
+
+    await this.env.run("coverage");
+
+    const expected = [
+      {
+        file: mock.pathToContract(hardhatConfig, 'ModifiersA.sol'),
+        pct: 75
+      },
+      {
+        file: mock.pathToContract(hardhatConfig, 'ModifiersC.sol'),
+        pct: 25
+      },
+    ];
+
+    verify.branchCoverage(expected);
+  })
+
+  it('modifiers (measureModifierCoverage = false)', async function(){
+    solcoverConfig.measureModifierCoverage = false;
+
+    mock.install('Modified', 'modified.js', solcoverConfig);
+    mock.hardhatSetupEnv(this);
+    await this.env.run("coverage");
+
+    const expected = [
+      {
+        file: mock.pathToContract(hardhatConfig, 'Modified.sol'),
+        pct: 100
+      }
+    ];
+
+    verify.branchCoverage(expected);
+  });
 })

test/units/modifiers.js

@@ -0,0 +1,163 @@
+const assert = require('assert');
+const util = require('./../util/util.js');
+
+const client = require('ganache-cli');
+const Coverage = require('./../../lib/coverage');
+const Api = require('./../../lib/api')
+
+describe('modifiers', () => {
+  let coverage;
+  let api;
+
+  before(async () => {
+    api = new Api({silent: true});
+    await api.ganache(client);
+  })
+  beforeEach(() => coverage = new Coverage());
+  after(async() => await api.finish());
+
+  async function setupAndRun(solidityFile){
+    const contract = await util.bootstrapCoverage(solidityFile, api);
+    coverage.addContract(contract.instrumented, util.filePath);
+
+    /* some modifiers intentionally fail */
+    try {
+      await contract.instance.a();
+    } catch(e){}
+
+    return coverage.generate(contract.data, util.pathPrefix);
+  }
+
+  it('should cover a modifier branch which always succeeds', async function() {
+    const mapping = await setupAndRun('modifiers/same-contract-pass');
+
+    assert.deepEqual(mapping[util.filePath].l, {
+      5: 1, 6: 1, 10: 1,
+    });
+    assert.deepEqual(mapping[util.filePath].b, {
+      1: [1, 0], 2: [1, 0]
+    });
+    assert.deepEqual(mapping[util.filePath].s, {
+      1: 1, 2: 1,
+    });
+    assert.deepEqual(mapping[util.filePath].f, {
+      1: 1, 2: 1
+    });
+  });
+
+  // NB: Failures are replayed by truffle-contract
+  it('should cover a modifier branch which never succeeds', async function() {
+    const mapping = await setupAndRun('modifiers/same-contract-fail');
+
+    assert.deepEqual(mapping[util.filePath].l, {
+      5: 2, 6: 0, 10: 0,
+    });
+    assert.deepEqual(mapping[util.filePath].b, {
+      1: [0, 2], 2: [0, 2]
+    });
+    assert.deepEqual(mapping[util.filePath].s, {
+      1: 2, 2: 0,
+    });
+    assert.deepEqual(mapping[util.filePath].f, {
+      1: 2, 2: 0
+    });
+  });
+
+  it('should cover a modifier on an overridden function', async function() {
+    const mapping = await setupAndRun('modifiers/override-function');
+
+    assert.deepEqual(mapping[util.filePath].l, {
+      9: 1, 10: 1, 14: 1,
+    });
+    assert.deepEqual(mapping[util.filePath].b, {
+      1: [1, 0], 2: [1, 0]
+    });
+    assert.deepEqual(mapping[util.filePath].s, {
+      1: 1, 2: 1
+    });
+    assert.deepEqual(mapping[util.filePath].f, {
+      1: 1, 2: 1
+    });
+  });
+
+  it('should cover multiple modifiers on the same function', async function() {
+    const mapping = await setupAndRun('modifiers/multiple-mods-same-fn');
+
+    assert.deepEqual(mapping[util.filePath].l, {
+      5: 1, 6: 1, 10: 1, 11: 1, 15: 1
+    });
+    assert.deepEqual(mapping[util.filePath].b, {
+      1: [1, 0], 2: [1, 0], 3: [1, 0], 4: [1, 0]
+    });
+    assert.deepEqual(mapping[util.filePath].s, {
+      1: 1, 2: 1, 3: 1
+    });
+    assert.deepEqual(mapping[util.filePath].f, {
+      1: 1, 2: 1, 3: 1
+    });
+  });
+
+  it('should cover multiple functions which use the same modifier', async function() {
+    const contract = await util.bootstrapCoverage('modifiers/multiple-fns-same-mod', api);
+    coverage.addContract(contract.instrumented, util.filePath);
+    await contract.instance.a();
+    await contract.instance.b();
+    const mapping = coverage.generate(contract.data, util.pathPrefix);
+
+    assert.deepEqual(mapping[util.filePath].l, {
+      5: 2, 6: 2, 10: 1, 14: 1
+    });
+    assert.deepEqual(mapping[util.filePath].b, {
+      1: [2, 0], 2: [1, 0], 3: [1, 0]
+    });
+    assert.deepEqual(mapping[util.filePath].s, {
+      1: 2, 2: 1, 3: 1,
+    });
+    assert.deepEqual(mapping[util.filePath].f, {
+      1: 2, 2: 1, 3: 1
+    });
+  });
+
+  it('should cover when both modifier branches are hit', async function() {
+    const contract = await util.bootstrapCoverage('modifiers/both-branches', 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: 1, 12: 1, 16: 1
+    });
+    assert.deepEqual(mapping[util.filePath].b, {
+      1: [1, 2], 2: [1, 2],
+    });
+    assert.deepEqual(mapping[util.filePath].s, {
+      1: 3, 2: 1, 3: 1,
+    });
+    assert.deepEqual(mapping[util.filePath].f, {
+      1: 3, 2: 1, 3: 1
+    });
+  });
+
+  it('should cover when modifiers are listed with newlines', async function() {
+    const mapping = await setupAndRun('modifiers/listed-modifiers');
+
+    assert.deepEqual(mapping[util.filePath].l, {
+      5: 1, 6: 1, 10: 1, 11: 1, 19: 1
+    });
+    assert.deepEqual(mapping[util.filePath].b, {
+      1: [1, 0], 2: [1, 0], 3: [1, 0], 4: [1, 0]
+    });
+    assert.deepEqual(mapping[util.filePath].s, {
+      1: 1, 2: 1, 3: 1,
+    });
+    assert.deepEqual(mapping[util.filePath].f, {
+      1: 1, 2: 1, 3: 1
+    });
+  });
+});

test/units/validator.js

@@ -47,7 +47,8 @@ describe('config validation', () => {
       "silent",
       "autoLaunchServer",
       "measureStatementCoverage",
-      "measureFunctionCoverage"
+      "measureFunctionCoverage",
+      "measureModifierCoverage"
     ]
 
     options.forEach(name => {