Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solidity code coverage #692

Merged
merged 10 commits into from
Aug 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@
"no-unmodified-loop-condition": "error",
"no-unneeded-ternary": "error",
"no-unused-expressions": "error",
"no-unused-vars": ["error", { "argsIgnorePattern": "^_" }],
"no-unused-vars": ["error", { "argsIgnorePattern": "^_", "varsIgnorePattern": "^_"}],
"no-use-before-define": "off",
"no-useless-call": "off",
"no-useless-computed-key": "error",
Expand Down
6 changes: 5 additions & 1 deletion lib/core/engine.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,8 @@ class Engine {
"libraryManager": this.libraryManagerService,
"processManager": this.processManagerService,
"storage": this.storageService,
"graph": this.graphService
"graph": this.graphService,
"codeCoverage": this.codeCoverageService
};

let service = services[serviceName];
Expand Down Expand Up @@ -226,6 +227,9 @@ class Engine {
this.registerModule('library_manager');
}

codeCoverageService(_options) {
this.registerModule('coverage');
}
}

module.exports = Engine;
1 change: 0 additions & 1 deletion lib/modules/blockchain_process/proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ const parseResponse = function(ipc, resBody){
} catch(e) {
return; // Response is not a json. Do nothing
}

if(commList[jsonO.id]){
commList[jsonO.id].transactionHash = jsonO.result;
transactions[jsonO.result] = {commListId: jsonO.id};
Expand Down
290 changes: 290 additions & 0 deletions lib/modules/coverage/contract_source.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,290 @@
const SourceMap = require('./source_map');

class ContractSource {
constructor(file, body) {
let self = this;

this.file = file;
this.body = body;

this.lineLengths = body.split("\n").map((line) => { return line.length; });
this.lineCount = this.lineLengths.length;

this.lineOffsets = this.lineLengths.reduce((sum, _elt, i) => {
sum[i] = (i == 0) ? 0 : self.lineLengths[i-1] + sum[i-1] + 1;
return sum;
}, []);

this.contracts = {};
}

sourceMapToLocations(sourceMap) {
var [offset, length, ..._] = sourceMap.split(":").map((val) => {
return parseInt(val, 10);
});

var locations = {};

for(let i = 0; i < this.lineCount; i++) {
if(this.lineOffsets[i+1] <= offset) continue;

locations.start = {line: i, column: offset - this.lineOffsets[i]};
break;
}

for(var i = locations.start.line; i < this.lineCount; i++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you merge these two loops together and add mre ifs to detect the end and start?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this originally but it was hard to make sense of what was going on. You'll notice that I offset the second loop so that we're not doing any unnecessary iterations anyway, so this is as optimized as it gets.

if(this.lineOffsets[i+1] <= offset + length) continue;

locations.end = {line: i, column: ((offset + length) - this.lineOffsets[i])};
break;
}

// Ensure we return an "end" as a safeguard if the marker ends up to be
// or surpass the offset for last character.
if(!locations.end) {
var lastLine = this.lineCount - 1;
locations.end = {line: lastLine, column: this.lineLengths[lastLine]};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can just do this.lineLengths[this.lineCount - 1]
The lastLine variable is a bit redundant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use it twice so I figured I'd declare a variable above.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, sorry, my bad.

}

// Istanbul likes lines to be 1-indexed, so we'll increment here before returning.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's weird, do you know why?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, as per the coverage.json spec:

Location objects are a {start: {line, column}, end: {line, column}} object that describes the start and end of a piece of code. Note that line is 1-based, but column is 0-based.

locations.start.line++;
locations.end.line++;
return locations;
}

parseSolcOutput(source, contracts) {
this.id = source.id;
this.ast = source.ast;
this.contractBytecode = {};

for(var contractName in contracts) {
this.contractBytecode[contractName] = {};

var contract = contracts[contractName];
var bytecodeMapping = this.contractBytecode[contractName];
var opcodes = contract.evm.deployedBytecode.opcodes.trim().split(' ');
var sourceMaps = contract.evm.deployedBytecode.sourceMap.split(';');

var bytecodeIdx = 0;
var pc = 0;
var instructions = 0;
do {
var instruction = opcodes[bytecodeIdx];
var length = this._instructionLength(instruction);
bytecodeMapping[pc] = {
instruction: instruction,
sourceMap: sourceMaps[instructions],
seen: false
};

pc += length;
instructions++;
bytecodeIdx += (length > 1) ? 2 : 1;
} while(bytecodeIdx < opcodes.length);
}
}

/*eslint complexity: ["error", 27]*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Holy, that's a big function. Is there a way you could split it?

generateCodeCoverage(trace) {
if(!this.ast || !this.contractBytecode) throw new Error('Error generating coverage: solc output was not assigned');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think i18n is active in the tests. So you can use __('Error generating coverage: solc output was not assigned')
That way we can translate it later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL!


var coverage = {
code: this.body.split("\n"),
l: {},
path: this.file,
s: {},
b: {},
f: {},
fnMap: {},
statementMap: {},
branchMap: {}
};

var nodesRequiringVisiting = [this.ast];
var sourceMapToNodeType = {};

do {
var node = nodesRequiringVisiting.pop();
if(!node) continue;

var children = [];
var markLocations = [];

switch(node.nodeType) {
case 'Identifier':
case 'Literal':
case 'VariableDeclaration':
case 'PragmaDirective':
// We don't need to do anything with these. Just carry on.
break;

case 'IfStatement':
coverage.b[node.id] = [0,0];

var location = this.sourceMapToLocations(node.src);
var trueBranchLocation = this.sourceMapToLocations(node.trueBody.src);

var falseBranchLocation;
if(node.falseBody) {
falseBranchLocation = this.sourceMapToLocations(node.falseBody.src);
} else {
falseBranchLocation = trueBranchLocation;
}

coverage.branchMap[node.id] = {
type: 'if',
locations: [trueBranchLocation, falseBranchLocation],
line: location.start.line
};

children = [node.condition]
.concat(node.trueBody.statements);

if(node.falseBody) children = children.concat(node.falseBody.statements);

markLocations = [location, trueBranchLocation, falseBranchLocation];

if(node.trueBody.statements[0]) {
node.trueBody.statements[0]._parent = {type: 'b', id: node.id, idx: 0};
}

if(node.falseBody && node.falseBody.statements[0]) {
node.falseBody.statements[0]._parent = {type: 'b', id: node.id, idx: 1};
}

sourceMapToNodeType[node.src] = [{type: 'b', id: node.id, body: {loc: location}}];
break;

case 'BinaryOperation':
children = [node.leftExpression, node.rightExpression];
break;

case 'Return':
// Return is a bit of a special case, because the statement src is the
// return "body". We don't `break` here on purpose so it can share the
// same logic below.
node.src = node.expression.src;
// falls through

case 'Assignment':
case 'ExpressionStatement':
coverage.s[node.id] = 0;

location = this.sourceMapToLocations(node.src);
coverage.statementMap[node.id] = location;

if(!sourceMapToNodeType[node.src]) sourceMapToNodeType[node.src] = [];
sourceMapToNodeType[node.src].push({
type: 's',
id: node.id,
body: {loc: coverage.statementMap[node.id]},
parent: node._parent
});


children = node.expression ? [node.expression] : [];
markLocations = [location];
break;

case 'ContractDefinition':
case 'SourceUnit':
children = node.nodes;
break;

case 'FunctionDefinition':
// Istanbul only wants the function definition, not the body, so we're
// going to do some fun math here.
var functionSourceMap = new SourceMap(node.src);
var functionParametersSourceMap = new SourceMap(node.parameters.src);

var functionDefinitionSourceMap = new SourceMap(
functionSourceMap.offset,
(functionParametersSourceMap.offset + functionParametersSourceMap.length) - functionSourceMap.offset
).toString();

var fnName = node.isConstructor ? "(constructor)" : node.name;
location = this.sourceMapToLocations(functionDefinitionSourceMap);

coverage.f[node.id] = 0;
coverage.fnMap[node.id] = {
name: fnName,
line: location.start.line,
loc: location
};

// Record function positions.
sourceMapToNodeType[node.src] = [{type: 'f', id: node.id, body: coverage.fnMap[node.id]}];

if(node.body) children = node.body.statements;
markLocations = [location];
break;

default:
// console.log(`Don't know how to handle node type ${node.nodeType}`);
break;
}

nodesRequiringVisiting = nodesRequiringVisiting.concat(children);

markLocations.forEach((location) => {
for(var i = location.start.line; i <= location.end.line; i++) {
coverage.l[i] = 0;
}
});

} while(nodesRequiringVisiting.length > 0);

var contractMatches = true;
for(var contractName in this.contractBytecode) {
var bytecode = this.contractBytecode[contractName];

// Try to match the contract to the bytecode. If it doesn't,
// then we bail.
contractMatches = trace.structLogs.every((step) => { return bytecode[step.pc]; });
if(!contractMatches) break;

trace.structLogs.forEach((step) => {
step = bytecode[step.pc];
if(!step.sourceMap || step.sourceMap == '') return;

var nodes = sourceMapToNodeType[step.sourceMap];

if(!nodes) return;

var recordedLineHit = false;

nodes.forEach((node) => {
if(node.body && node.body.loc && !recordedLineHit) {
for(var line = node.body.loc.start.line; line <= node.body.loc.end.line; line++) {
coverage.l[line]++;
}

recordedLineHit = true;
}

if(node.type != 'b') coverage[node.type][node.id]++;

if(!node.parent) return;

switch(node.parent.type) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This switch is only for one condition. Can you convert to an if or is there supposed to be more conditions added later?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whenever we add Vyper support, there will be more conditions here.

case 'b':
coverage.b[node.parent.id][node.parent.idx]++;
break;

default:
// do nothing
}
});
});
}

return coverage;
}

_instructionLength(instruction) {
if(instruction.indexOf('PUSH') == -1) return 1;
return parseInt(instruction.match(/PUSH(\d+)/m)[1], 10) + 1;
}
}

module.exports = ContractSource;
Loading