Skip to content

Commit 8e8f668

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 5174ecd commit 8e8f668

29 files changed

+937
-21
lines changed

‎lib/coverage.js

+8-4
Original file line numberDiff line numberDiff line change
@@ -69,13 +69,17 @@ 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;
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;
7983
case 'requirePre': this.requireData[contractPath][id].preEvents = hits; break;
8084
case 'requirePost': this.requireData[contractPath][id].postEvents = hits; break;
8185
}

‎lib/injector.js

+64-8
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 `c_${web3Utils.keccak256(id).slice(0,10)}`
2632
}
2733

34+
// Method returns boolean true
35+
_getTrueMethodIdentifier(id){
36+
return `c_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)
@@ -44,7 +55,7 @@ class Injector {
4455
* @param {String} id
4556
* @return {String}
4657
*/
47-
_getHashMethodDefinition(id, contract){
58+
_getDefaultMethodDefinition(id){
4859
const hash = web3Utils.keccak256(id).slice(0,10);
4960
const method = this._getMethodIdentifier(id);
5061
return `\nfunction ${method}(bytes32 c__${hash}) internal pure {}\n`;
@@ -58,8 +69,20 @@ class Injector {
5869
*/
5970
_getFileScopedHashMethodDefinition(id, contract){
6071
const hash = web3Utils.keccak256(id).slice(0,10);
61-
const method = this._getMethodIdentifier(id);
62-
return `\nfunction ${method}(bytes32 c__${hash}) pure {}\n`;
72+
const method = this._getDefaultMethodIdentifier(id);
73+
return `\nfunction ${method}(bytes32 c__${hash}) public pure {}\n`;
74+
}
75+
76+
/**
77+
* Generates a solidity statement injection defining a method
78+
* *which returns boolean true* to pass instrumentation hash to.
79+
* @param {String} fileName
80+
* @return {String} ex: bytes32[1] memory _sc_82e0891
81+
*/
82+
_getTrueMethodDefinition(id){
83+
const hash = web3Utils.keccak256(id).slice(0,10);
84+
const method = this._getTrueMethodIdentifier(id);
85+
return `\nfunction ${method}(bytes32 c__${hash}) public pure returns (bool){ return true; }\n`;
6386
}
6487

6588
injectLine(contract, fileName, injectionPoint, injection, instrumentation){
@@ -217,9 +240,42 @@ class Injector {
217240
const end = contract.instrumented.slice(injectionPoint);
218241
const id = `${fileName}:${injection.contractName}`;
219242

220-
contract.instrumented = (injection.isFileScoped)
221-
? `${start}${this._getFileScopedHashMethodDefinition(id)}${end}`
222-
: `${start}${this._getHashMethodDefinition(id)}${end}`;
243+
const defaultMethodDefinition = (injection.isFileScoped)
244+
? this._getFileScopedHashMethodDefinition(id)
245+
: this._getHashMethodDefinition(id);
246+
247+
contract.instrumented = `${start}` +
248+
`${defaultMethodDefinition}` +
249+
`${this._getTrueMethodDefinition(id)}` +
250+
`${end}`;
251+
}
252+
253+
injectOpenParen(contract, fileName, injectionPoint, injection, instrumentation){
254+
const start = contract.instrumented.slice(0, injectionPoint);
255+
const end = contract.instrumented.slice(injectionPoint);
256+
contract.instrumented = `${start}(${end}`;
257+
}
258+
259+
injectLogicalOR(contract, fileName, injectionPoint, injection, instrumentation){
260+
const type = 'logicalOR';
261+
const id = `${fileName}:${injection.contractName}`;
262+
263+
const {
264+
start,
265+
end,
266+
hash,
267+
injectable
268+
} = this._getInjectionComponents(contract, injectionPoint, id, type);
269+
270+
instrumentation[hash] = {
271+
id: injection.branchId,
272+
locationIdx: injection.locationIdx,
273+
type: type,
274+
contractPath: fileName,
275+
hits: 0
276+
}
277+
278+
contract.instrumented = `${start}${injectable}${end}`;
223279
}
224280
};
225281

‎lib/instrumenter.js

+6-1
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,12 @@ class Instrumenter {
8888

8989
// Line instrumentation has to happen first
9090
contract.injectionPoints[injectionPoint].sort((a, b) => {
91-
const injections = ['injectBranch', 'injectEmptyBranch', 'injectLine'];
91+
const injections = [
92+
'injectLogicalOR',
93+
'injectBranch',
94+
'injectEmptyBranch',
95+
'injectLine'
96+
];
9297
return injections.indexOf(b.type) - injections.indexOf(a.type);
9398
});
9499

‎lib/parse.js

+53-8
Original file line numberDiff line numberDiff line change
@@ -32,17 +32,47 @@ parse.Block = function(contract, expression) {
3232
}
3333
};
3434

35-
parse.BinaryOperation = function(contract, expression) {
36-
register.statement(contract, expression);
35+
parse.BinaryOperation = function(contract, expression, skipStatementRegistry) {
36+
// Regular binary operation
37+
if (!skipStatementRegistry){
38+
register.statement(contract, expression);
39+
40+
// LogicalOR conditional search...
41+
} else {
42+
parse[expression.left.type] &&
43+
parse[expression.left.type](contract, expression.left, true);
44+
45+
parse[expression.right.type] &&
46+
parse[expression.right.type](contract, expression.right, true);
47+
48+
if (expression.operator === '||'){
49+
register.logicalOR(contract, expression);
50+
}
51+
}
52+
}
53+
54+
parse.TupleExpression = function(contract, expression, skipStatementRegistry) {
55+
expression.components.forEach(component => {
56+
parse[component.type] &&
57+
parse[component.type](contract, component, skipStatementRegistry);
58+
});
3759
}
3860

39-
parse.FunctionCall = function(contract, expression) {
61+
parse.FunctionCall = function(contract, expression, skipStatementRegistry) {
4062
// In any given chain of call expressions, only the last one will fail this check.
4163
// This makes sure we don't instrument a chain of expressions multiple times.
4264
if (expression.expression.type !== 'FunctionCall') {
43-
register.statement(contract, expression);
65+
66+
// Don't register sub-expressions (like intermediate method calls)
67+
if (!skipStatementRegistry){
68+
register.statement(contract, expression);
69+
}
70+
4471
if (expression.expression.name === 'require') {
4572
register.requireBranch(contract, expression);
73+
expression.arguments.forEach(arg => {
74+
parse[arg.type] && parse[arg.type](contract, arg, true);
75+
});
4676
}
4777
parse[expression.expression.type] &&
4878
parse[expression.expression.type](contract, expression.expression);
@@ -52,10 +82,13 @@ parse.FunctionCall = function(contract, expression) {
5282
}
5383
};
5484

55-
parse.Conditional = function(contract, expression) {
56-
register.statement(contract, expression);
57-
// TODO: Investigate node structure
58-
// There are potential substatements here we aren't measuring
85+
parse.Conditional = function(contract, expression, skipStatementRegistry) {
86+
if (!skipStatementRegistry){
87+
register.statement(contract, expression);
88+
}
89+
90+
parse[expression.condition.type] &&
91+
parse[expression.condition.type](contract, expression.condition, skipStatementRegistry);
5992
};
6093

6194
parse.ContractDefinition = function(contract, expression) {
@@ -145,6 +178,10 @@ parse.FunctionDefinition = function(contract, expression) {
145178
parse.IfStatement = function(contract, expression) {
146179
register.statement(contract, expression);
147180
register.ifStatement(contract, expression);
181+
182+
parse[expression.condition.type] &&
183+
parse[expression.condition.type](contract, expression.condition, true);
184+
148185
parse[expression.trueBody.type] &&
149186
parse[expression.trueBody.type](contract, expression.trueBody);
150187

@@ -220,6 +257,10 @@ parse.SourceUnit = function(contract, expression) {
220257

221258
parse.ReturnStatement = function(contract, expression) {
222259
register.statement(contract, expression);
260+
261+
expression.expression &&
262+
parse[expression.expression.type] &&
263+
parse[expression.expression.type](contract, expression.expression, true);
223264
};
224265

225266
// TODO:Investigate node structure
@@ -255,6 +296,10 @@ parse.VariableDeclarationStatement = function (contract, expression) {
255296

256297
parse.WhileStatement = function (contract, expression) {
257298
register.statement(contract, expression);
299+
300+
parse[expression.condition.type] &&
301+
parse[expression.condition.type](contract, expression.condition, true);
302+
258303
parse[expression.body.type] &&
259304
parse[expression.body.type](contract, expression.body);
260305
};

‎lib/registrar.js

+101
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,107 @@ class Registrar {
186186
};
187187
};
188188

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

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

+8
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

+7
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)