diff --git a/lib/injector.js b/lib/injector.js index 4e3ddda5..a2215a06 100644 --- a/lib/injector.js +++ b/lib/injector.js @@ -108,30 +108,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`; } } @@ -366,7 +356,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, diff --git a/lib/registrar.js b/lib/registrar.js index 389a84e5..5c6da7c1 100644 --- a/lib/registrar.js +++ b/lib/registrar.js @@ -131,7 +131,7 @@ 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], @@ -139,7 +139,19 @@ class Registrar { 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... diff --git a/plugins/resources/plugin.utils.js b/plugins/resources/plugin.utils.js index e5f87364..39c5586c 100644 --- a/plugins/resources/plugin.utils.js +++ b/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 || {}; diff --git a/test/sources/solidity/contracts/modifiers/reverting-fn.sol b/test/sources/solidity/contracts/modifiers/reverting-fn.sol new file mode 100644 index 00000000..d5f3158f --- /dev/null +++ b/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); + } +} diff --git a/test/sources/solidity/contracts/modifiers/reverting-neighbor.sol b/test/sources/solidity/contracts/modifiers/reverting-neighbor.sol new file mode 100644 index 00000000..e63baca8 --- /dev/null +++ b/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; + } +} diff --git a/test/units/modifiers.js b/test/units/modifiers.js index 4af3a2bf..d41aa0b7 100644 --- a/test/units/modifiers.js +++ b/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');