Skip to content

Commit 23f7987

Browse files
committed
Fix pre/post strategy for modifier coverage (#595)
1 parent 350e699 commit 23f7987

File tree

6 files changed

+114
-61
lines changed

6 files changed

+114
-61
lines changed

lib/injector.js

Lines changed: 8 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -120,30 +120,20 @@ class Injector {
120120
injection += `modifier ${this._getModifierIdentifier(item.modifierId)}{ `;
121121

122122
let hash = this._getHash(item.modifierId);
123-
let injectable = this._getInjectable(item.contractId, hash, 'modifier-pre');
123+
let comment = `modifier-${item.condition}`;
124+
let injectable = this._getInjectable(item.contractId, hash, comment);
124125

125-
instrumentation[hash] = {
126-
id: item.branchId,
127-
type: 'requirePre',
128-
contractPath: item.fileName,
129-
hits: 0
130-
}
131-
132-
injection += injectable;
133-
injection += `_;`
134-
135-
hash = this._getHash(item.modifierId);
136-
injectable = this._getInjectable(item.contractId, hash, 'modifier-post');
126+
// Process modifiers in the same step as `require` stmts in coverage.js
127+
let type = (item.condition === 'pre') ? 'requirePre' : 'requirePost';
137128

138129
instrumentation[hash] = {
139130
id: item.branchId,
140-
type: 'requirePost',
131+
type: type,
141132
contractPath: item.fileName,
142133
hits: 0
143134
}
144135

145-
injection += injectable;
146-
injection += ` }\n`;
136+
injection += `${injectable} _; }\n`;
147137
}
148138
}
149139

@@ -383,7 +373,8 @@ class Injector {
383373
const type = 'modifier';
384374
const contractId = `${fileName}:${injection.contractName}`;
385375
const modifierId = `${fileName}:${injection.contractName}:` +
386-
`${injection.modifierName}:${injection.fnId}`;
376+
`${injection.modifierName}:${injection.fnId}:` +
377+
`${injection.condition}`;
387378

388379
const {
389380
start,

lib/registrar.js

Lines changed: 14 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -131,15 +131,27 @@ class Registrar {
131131
// Add modifier branch coverage
132132
if (!this.enabled.modifiers || expression.isConstructor) continue;
133133

134-
this.addNewModifierBranch(contract, modifier);
134+
this.addNewBranch(contract, modifier);
135135
this._createInjectionPoint(
136136
contract,
137137
modifier.range[0],
138138
{
139139
type: 'injectModifier',
140140
branchId: contract.branchId,
141141
modifierName: modifier.name,
142-
fnId: contract.fnId
142+
fnId: contract.fnId,
143+
condition: 'pre'
144+
}
145+
);
146+
this._createInjectionPoint(
147+
contract,
148+
modifier.range[1] + 1,
149+
{
150+
type: 'injectModifier',
151+
branchId: contract.branchId,
152+
modifierName: modifier.name,
153+
fnId: contract.fnId,
154+
condition: 'post'
143155
}
144156
);
145157
}
@@ -209,41 +221,6 @@ class Registrar {
209221
};
210222
};
211223

212-
/**
213-
* Registers injections for modifier branch measurements.
214-
* @param {Object} contract instrumentation target
215-
* @param {Object} expression AST node
216-
*/
217-
addNewModifierBranch(contract, expression) {
218-
const startContract = contract.instrumented.slice(0, expression.range[0]);
219-
const startline = ( startContract.match(/\n/g) || [] ).length + 1;
220-
const startcol = expression.range[0] - startContract.lastIndexOf('\n') - 1;
221-
222-
contract.branchId += 1;
223-
224-
// NB locations for if branches in istanbul are zero
225-
// length and associated with the start of the if.
226-
contract.branchMap[contract.branchId] = {
227-
line: startline,
228-
type: 'if',
229-
locations: [{
230-
start: {
231-
line: startline, column: startcol,
232-
},
233-
end: {
234-
line: startline, column: startcol,
235-
},
236-
}, {
237-
start: {
238-
line: startline, column: startcol,
239-
},
240-
end: {
241-
line: startline, column: startcol,
242-
},
243-
}],
244-
};
245-
};
246-
247224
addNewConditionalBranch(contract, expression){
248225
let start;
249226
// Instabul HTML highlighting location data...

plugins/resources/plugin.utils.js

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -222,13 +222,6 @@ function loadSolcoverJS(config={}){
222222
coverageConfig.cwd = config.workingDir;
223223
coverageConfig.originalContractsDir = config.contractsDir;
224224

225-
if (config.matrix){
226-
coverageConfig.measureBranchCoverage = false;
227-
coverageConfig.measureFunctionCoverage = false;
228-
coverageConfig.measureModifierCoverage = false;
229-
coverageConfig.measureStatementCoverage = false;
230-
}
231-
232225
// Solidity-Coverage writes to Truffle config
233226
config.mocha = config.mocha || {};
234227

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
pragma solidity ^0.7.0;
2+
3+
contract Test {
4+
bool flag = true;
5+
6+
modifier m {
7+
require(flag);
8+
_;
9+
}
10+
11+
function a(bool success) m public {
12+
require(success);
13+
}
14+
}
Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
pragma solidity ^0.7.0;
2+
3+
contract Test {
4+
bool flag = true;
5+
6+
modifier m {
7+
require(true);
8+
_;
9+
}
10+
11+
modifier n {
12+
require(flag);
13+
_;
14+
}
15+
16+
function flip() public {
17+
flag = !flag;
18+
}
19+
20+
function a() m n public {
21+
uint x = 5;
22+
}
23+
}

test/units/modifiers.js

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,6 +144,61 @@ describe('modifiers', () => {
144144
});
145145
});
146146

147+
// Case: when first modifier always suceeds but a subsequent modifier succeeds and fails,
148+
// there should be a missing `else` branch on first modifier
149+
it('should not be influenced by revert from a subsequent modifier', async function() {
150+
const contract = await util.bootstrapCoverage('modifiers/reverting-neighbor', api);
151+
coverage.addContract(contract.instrumented, util.filePath);
152+
await contract.instance.a();
153+
await contract.instance.flip();
154+
155+
try {
156+
await contract.instance.a();
157+
} catch(e) { /*ignore*/ }
158+
159+
const mapping = coverage.generate(contract.data, util.pathPrefix);
160+
161+
assert.deepEqual(mapping[util.filePath].l, {
162+
"7":3,"8":3,"12":3,"13":1,"17":1,"21":1
163+
});
164+
assert.deepEqual(mapping[util.filePath].b, {
165+
"1":[3,0],"2":[1,2],"3":[3,0],"4":[1,2]
166+
});
167+
assert.deepEqual(mapping[util.filePath].s, {
168+
"1":3,"2":3,"3":1,"4":1
169+
});
170+
assert.deepEqual(mapping[util.filePath].f, {
171+
"1":3,"2":3,"3":1,"4":1
172+
});
173+
});
174+
175+
// Case: when the modifier always suceeds but fn logic succeeds and fails, there should be
176+
// a missing `else` branch on modifier
177+
it('should not be influenced by revert within the function', async function() {
178+
const contract = await util.bootstrapCoverage('modifiers/reverting-fn', api);
179+
coverage.addContract(contract.instrumented, util.filePath);
180+
await contract.instance.a(true);
181+
182+
try {
183+
await contract.instance.a(false);
184+
} catch(e) { /*ignore*/ }
185+
186+
const mapping = coverage.generate(contract.data, util.pathPrefix);
187+
188+
assert.deepEqual(mapping[util.filePath].l, {
189+
7: 3, 8: 3, 12: 3
190+
});
191+
assert.deepEqual(mapping[util.filePath].b, {
192+
1: [3, 0], 2: [3, 0], 3: [1,2]
193+
});
194+
assert.deepEqual(mapping[util.filePath].s, {
195+
1: 3, 2: 3
196+
});
197+
assert.deepEqual(mapping[util.filePath].f, {
198+
1: 3, 2: 3
199+
});
200+
});
201+
147202
it('should cover when modifiers are listed with newlines', async function() {
148203
const mapping = await setupAndRun('modifiers/listed-modifiers');
149204

0 commit comments

Comments
 (0)