Skip to content

Commit 9b7bc44

Browse files
cgeweckecgewecke
committed
Add branch coverage for logical OR conditions (#499)
* Add initial OR coverage test cases * Add logicalOR coverage for "require" conditions * Add logicalOR coverage for "while" conditions * Add logicalOR coverage for "return" conditions * Add logicalOR coverage for "if" conditions * Add logicalOR branch highlighting for html report
1 parent 7cb2e41 commit 9b7bc44

29 files changed

+936
-22
lines changed

‎lib/coverage.js

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,15 +69,19 @@ class Coverage {
6969
const data = collectedData[hash];
7070
const contractPath = collectedData[hash].contractPath;
7171
const id = collectedData[hash].id;
72+
73+
// NB: Any branch using the injected fn which returns boolean will have artificially
74+
// doubled hits (because of something internal to Solidity about how the stack is managed)
7275
const hits = collectedData[hash].hits;
7376

7477
switch(collectedData[hash].type){
75-
case 'line': this.data[contractPath].l[id] = hits; break;
76-
case 'function': this.data[contractPath].f[id] = hits; break;
77-
case 'statement': this.data[contractPath].s[id] = hits; break;
78-
case 'branch': this.data[contractPath].b[id][data.locationIdx] = hits; break;
79-
case 'assertPre': this.assertData[contractPath][id].preEvents = hits; break;
80-
case 'assertPost': this.assertData[contractPath][id].postEvents = hits; break;
78+
case 'line': this.data[contractPath].l[id] = hits; break;
79+
case 'function': this.data[contractPath].f[id] = hits; break;
80+
case 'statement': this.data[contractPath].s[id] = hits; break;
81+
case 'branch': this.data[contractPath].b[id][data.locationIdx] = hits; break;
82+
case 'logicalOR': this.data[contractPath].b[id][data.locationIdx] = hits / 2; break;
83+
case 'assertPre': this.assertData[contractPath][id].preEvents = hits; break;
84+
case 'assertPost': this.assertData[contractPath][id].postEvents = hits; break;
8185
}
8286
}
8387

‎lib/injector.js

Lines changed: 61 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,18 +13,29 @@ class Injector {
1313
}
1414

1515
_getInjectable(id, hash, type){
16-
return `${this._getMethodIdentifier(id)}(${hash}); /* ${type} */ \n`;
16+
switch(type){
17+
case 'logicalOR':
18+
return ` && ${this._getTrueMethodIdentifier(id)}(${hash}))`;
19+
default:
20+
return `${this._getDefaultMethodIdentifier(id)}(${hash}); /* ${type} */ \n`;
21+
}
1722
}
1823

1924
_getHash(id) {
2025
this.hashCounter++;
2126
return web3Utils.keccak256(`${id}:${this.hashCounter}`);
2227
}
2328

24-
_getMethodIdentifier(id){
29+
// Method returns void
30+
_getDefaultMethodIdentifier(id){
2531
return `coverage_${web3Utils.keccak256(id).slice(0,10)}`
2632
}
2733

34+
// Method returns boolean true
35+
_getTrueMethodIdentifier(id){
36+
return `coverage_true${web3Utils.keccak256(id).slice(0,10)}`
37+
}
38+
2839
_getInjectionComponents(contract, injectionPoint, id, type){
2940
const { start, end } = this._split(contract, injectionPoint);
3041
const hash = this._getHash(id)
@@ -39,17 +50,29 @@ class Injector {
3950
}
4051

4152
/**
42-
* Generates a solidity statement injection. Declared once per fn.
43-
* Definition is the same for every fn in file.
53+
* Generates a solidity statement injection defining a method
54+
* *which returns void* to pass instrumentation hash to.
4455
* @param {String} id
4556
* @return {String} ex: bytes32[1] memory _sc_82e0891
4657
*/
47-
_getHashMethodDefinition(id, contract){
58+
_getDefaultMethodDefinition(id){
4859
const hash = web3Utils.keccak256(id).slice(0,10);
49-
const method = this._getMethodIdentifier(id);
60+
const method = this._getDefaultMethodIdentifier(id);
5061
return `\nfunction ${method}(bytes32 c__${hash}) public pure {}\n`;
5162
}
5263

64+
/**
65+
* Generates a solidity statement injection defining a method
66+
* *which returns boolean true* to pass instrumentation hash to.
67+
* @param {String} fileName
68+
* @return {String} ex: bytes32[1] memory _sc_82e0891
69+
*/
70+
_getTrueMethodDefinition(id){
71+
const hash = web3Utils.keccak256(id).slice(0,10);
72+
const method = this._getTrueMethodIdentifier(id);
73+
return `\nfunction ${method}(bytes32 c__${hash}) public pure returns (bool){ return true; }\n`;
74+
}
75+
5376
injectLine(contract, fileName, injectionPoint, injection, instrumentation){
5477
const type = 'line';
5578
const { start, end } = this._split(contract, injectionPoint);
@@ -204,7 +227,38 @@ class Injector {
204227
const start = contract.instrumented.slice(0, injectionPoint);
205228
const end = contract.instrumented.slice(injectionPoint);
206229
const id = `${fileName}:${injection.contractName}`;
207-
contract.instrumented = `${start}${this._getHashMethodDefinition(id)}${end}`;
230+
contract.instrumented = `${start}` +
231+
`${this._getDefaultMethodDefinition(id)}` +
232+
`${this._getTrueMethodDefinition(id)}` +
233+
`${end}`;
234+
}
235+
236+
injectOpenParen(contract, fileName, injectionPoint, injection, instrumentation){
237+
const start = contract.instrumented.slice(0, injectionPoint);
238+
const end = contract.instrumented.slice(injectionPoint);
239+
contract.instrumented = `${start}(${end}`;
240+
}
241+
242+
injectLogicalOR(contract, fileName, injectionPoint, injection, instrumentation){
243+
const type = 'logicalOR';
244+
const id = `${fileName}:${injection.contractName}`;
245+
246+
const {
247+
start,
248+
end,
249+
hash,
250+
injectable
251+
} = this._getInjectionComponents(contract, injectionPoint, id, type);
252+
253+
instrumentation[hash] = {
254+
id: injection.branchId,
255+
locationIdx: injection.locationIdx,
256+
type: type,
257+
contractPath: fileName,
258+
hits: 0
259+
}
260+
261+
contract.instrumented = `${start}${injectable}${end}`;
208262
}
209263
};
210264

‎lib/instrumenter.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,12 @@ class Instrumenter {
8484

8585
// Line instrumentation has to happen first
8686
contract.injectionPoints[injectionPoint].sort((a, b) => {
87-
const injections = ['injectBranch', 'injectEmptyBranch', 'injectLine'];
87+
const injections = [
88+
'injectLogicalOR',
89+
'injectBranch',
90+
'injectEmptyBranch',
91+
'injectLine'
92+
];
8893
return injections.indexOf(b.type) - injections.indexOf(a.type);
8994
});
9095

‎lib/parse.js

Lines changed: 53 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,17 +20,47 @@ parse.Block = function(contract, expression) {
2020
}
2121
};
2222

23-
parse.BinaryOperation = function(contract, expression) {
24-
register.statement(contract, expression);
23+
parse.BinaryOperation = function(contract, expression, skipStatementRegistry) {
24+
// Regular binary operation
25+
if (!skipStatementRegistry){
26+
register.statement(contract, expression);
27+
28+
// LogicalOR conditional search...
29+
} else {
30+
parse[expression.left.type] &&
31+
parse[expression.left.type](contract, expression.left, true);
32+
33+
parse[expression.right.type] &&
34+
parse[expression.right.type](contract, expression.right, true);
35+
36+
if (expression.operator === '||'){
37+
register.logicalOR(contract, expression);
38+
}
39+
}
40+
}
41+
42+
parse.TupleExpression = function(contract, expression, skipStatementRegistry) {
43+
expression.components.forEach(component => {
44+
parse[component.type] &&
45+
parse[component.type](contract, component, skipStatementRegistry);
46+
});
2547
}
2648

27-
parse.FunctionCall = function(contract, expression) {
49+
parse.FunctionCall = function(contract, expression, skipStatementRegistry) {
2850
// In any given chain of call expressions, only the last one will fail this check.
2951
// This makes sure we don't instrument a chain of expressions multiple times.
3052
if (expression.expression.type !== 'FunctionCall') {
31-
register.statement(contract, expression);
53+
54+
// Don't register sub-expressions (like intermediate method calls)
55+
if (!skipStatementRegistry){
56+
register.statement(contract, expression);
57+
}
58+
3259
if (expression.expression.name === 'assert' || expression.expression.name === 'require') {
3360
register.assertOrRequire(contract, expression);
61+
expression.arguments.forEach(arg => {
62+
parse[arg.type] && parse[arg.type](contract, arg, true);
63+
});
3464
}
3565
parse[expression.expression.type] &&
3666
parse[expression.expression.type](contract, expression.expression);
@@ -40,10 +70,13 @@ parse.FunctionCall = function(contract, expression) {
4070
}
4171
};
4272

43-
parse.Conditional = function(contract, expression) {
44-
register.statement(contract, expression);
45-
// TODO: Investigate node structure
46-
// There are potential substatements here we aren't measuring
73+
parse.Conditional = function(contract, expression, skipStatementRegistry) {
74+
if (!skipStatementRegistry){
75+
register.statement(contract, expression);
76+
}
77+
78+
parse[expression.condition.type] &&
79+
parse[expression.condition.type](contract, expression.condition, skipStatementRegistry);
4780
};
4881

4982
parse.ContractDefinition = function(contract, expression) {
@@ -120,6 +153,10 @@ parse.FunctionDefinition = function(contract, expression) {
120153
parse.IfStatement = function(contract, expression) {
121154
register.statement(contract, expression);
122155
register.ifStatement(contract, expression);
156+
157+
parse[expression.condition.type] &&
158+
parse[expression.condition.type](contract, expression.condition, true);
159+
123160
parse[expression.trueBody.type] &&
124161
parse[expression.trueBody.type](contract, expression.trueBody);
125162

@@ -163,6 +200,10 @@ parse.SourceUnit = function(contract, expression) {
163200

164201
parse.ReturnStatement = function(contract, expression) {
165202
register.statement(contract, expression);
203+
204+
expression.expression &&
205+
parse[expression.expression.type] &&
206+
parse[expression.expression.type](contract, expression.expression, true);
166207
};
167208

168209
// TODO:Investigate node structure
@@ -182,6 +223,10 @@ parse.VariableDeclarationStatement = function (contract, expression) {
182223

183224
parse.WhileStatement = function (contract, expression) {
184225
register.statement(contract, expression);
226+
227+
parse[expression.condition.type] &&
228+
parse[expression.condition.type](contract, expression.condition, true);
229+
185230
parse[expression.body.type] &&
186231
parse[expression.body.type](contract, expression.body);
187232
};

‎lib/registrar.js

Lines changed: 101 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,107 @@ class Registrar {
179179
};
180180
};
181181

182+
/**
183+
* Registers injections for branch measurements. Used by logicalOR registration method.
184+
* @param {Object} contract instrumentation target
185+
* @param {Object} expression AST node
186+
*/
187+
addNewLogicalORBranch(contract, expression) {
188+
let start;
189+
190+
// Instabul HTML highlighting location data...
191+
const leftZero = expression.left.range[0];
192+
const leftOne = expression.left.range[1];
193+
const rightZero = expression.right.range[0];
194+
const rightOne = expression.right.range[1];
195+
196+
start = contract.instrumented.slice(0, leftZero);
197+
const leftStartLine = ( start.match(/\n/g) || [] ).length + 1;
198+
const leftStartCol = leftZero - start.lastIndexOf('\n') - 1;
199+
200+
start = contract.instrumented.slice(0, leftOne);
201+
const leftEndLine = ( start.match(/\n/g) || [] ).length + 1;
202+
const leftEndCol = leftOne - start.lastIndexOf('\n') - 1;
203+
204+
start = contract.instrumented.slice(0, rightZero);
205+
const rightStartLine = ( start.match(/\n/g) || [] ).length + 1;
206+
const rightStartCol = rightZero - start.lastIndexOf('\n') - 1;
207+
208+
start = contract.instrumented.slice(0, rightOne);
209+
const rightEndLine = ( start.match(/\n/g) || [] ).length + 1;
210+
const rightEndCol = rightOne - start.lastIndexOf('\n') - 1;
211+
212+
contract.branchId += 1;
213+
214+
// NB locations for if branches in istanbul are zero
215+
// length and associated with the start of the if.
216+
contract.branchMap[contract.branchId] = {
217+
line: leftStartLine,
218+
type: 'cond-expr',
219+
locations: [{
220+
start: {
221+
line: leftStartLine, column: leftStartCol,
222+
},
223+
end: {
224+
line: leftEndLine, column: leftEndCol,
225+
},
226+
}, {
227+
start: {
228+
line: rightStartLine, column: rightStartCol,
229+
},
230+
end: {
231+
line: rightEndLine, column: rightEndCol,
232+
},
233+
}],
234+
};
235+
};
236+
237+
/**
238+
* Registers injections for logicalOR clause measurements (branches)
239+
* @param {Object} contract instrumentation target
240+
* @param {Object} expression AST node
241+
* @param {Number} injectionIdx pre/post branch index (left=0, right=1)
242+
*/
243+
logicalOR(contract, expression) {
244+
this.addNewLogicalORBranch(contract, expression);
245+
246+
// Left
247+
this._createInjectionPoint(
248+
contract,
249+
expression.left.range[0],
250+
{
251+
type: 'injectOpenParen',
252+
}
253+
);
254+
this._createInjectionPoint(
255+
contract,
256+
expression.left.range[1] + 1,
257+
{
258+
type: 'injectLogicalOR',
259+
branchId: contract.branchId,
260+
locationIdx: 0
261+
}
262+
);
263+
264+
// Right
265+
this._createInjectionPoint(
266+
contract,
267+
expression.right.range[0],
268+
{
269+
type: 'injectOpenParen',
270+
}
271+
);
272+
this._createInjectionPoint(
273+
contract,
274+
expression.right.range[1] + 1,
275+
{
276+
type: 'injectLogicalOR',
277+
branchId: contract.branchId,
278+
locationIdx: 1
279+
}
280+
);
281+
}
282+
182283
/**
183284
* Registers injections for assert/require statement measurements (branches)
184285
* @param {Object} contract instrumentation target

‎test/integration/projects/logical-or/.solcover.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
// Testing hooks
2+
const fn = (msg, config) => config.logger.log(msg);
3+
4+
module.exports = {
5+
skipFiles: ['Migrations.sol'],
6+
silent: process.env.SILENT ? true : false,
7+
istanbulReporter: ['json-summary', 'text'],
8+
}

‎test/integration/projects/logical-or/buidler.config.js

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
const { loadPluginFile } = require("@nomiclabs/buidler/plugins-testing");
2+
loadPluginFile(__dirname + "/../plugins/buidler.plugin");
3+
usePlugin("@nomiclabs/buidler-truffle5");
4+
5+
module.exports={
6+
defaultNetwork: "buidlerevm"
7+
};

0 commit comments

Comments
 (0)