From ca19b64d76493ca33f3cbbb3fb4b4a0f9329db95 Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Mon, 24 Jul 2017 18:14:14 +0100 Subject: [PATCH 1/7] Upgrade to solc 0.4.13 --- package.json | 2 +- test/util/vm.js | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 21c0bb1..bd744ad 100644 --- a/package.json +++ b/package.json @@ -44,7 +44,7 @@ "istanbul": "^0.4.5", "merkle-patricia-tree": "~2.1.2", "mocha": "^3.1.0", - "solc": "0.4.8", + "solc": "0.4.13", "truffle": "3.4.4" } } diff --git a/test/util/vm.js b/test/util/vm.js index e90179d..0ebbe16 100644 --- a/test/util/vm.js +++ b/test/util/vm.js @@ -53,7 +53,7 @@ function getAbi(source, compilation) { throw new Error('Could not parse contract name from source.'); } const contractName = contractNameMatch[1]; - return JSON.parse(compilation.contracts[contractName].interface); + return JSON.parse(compilation.contracts[':' + contractName].interface); } /** @@ -139,7 +139,7 @@ function callMethod(vm, abi, address, functionName, args) { */ module.exports.execute = function ex(contract, functionName, args) { const output = solc.compile(contract, 1); - const code = new Buffer(output.contracts.Test.bytecode, 'hex'); + const code = new Buffer(output.contracts[':Test'].bytecode, 'hex'); const abi = getAbi(contract, output); const stateTrie = new Trie(); const vm = new VM({ From f1389eecdae654f61002b64ca354bd42358a6c54 Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Mon, 24 Jul 2017 18:18:20 +0100 Subject: [PATCH 2/7] First stab at instrumenting asserts --- lib/coverageMap.js | 33 +++++++++++++++++- lib/injector.js | 16 +++++++++ lib/instrumenter.js | 32 +++++++++++++++++ lib/parse.js | 3 ++ test/assert.js | 63 ++++++++++++++++++++++++++++++++++ test/sources/assert/Assert.sol | 7 ++++ 6 files changed, 153 insertions(+), 1 deletion(-) create mode 100644 test/assert.js create mode 100644 test/sources/assert/Assert.sol diff --git a/lib/coverageMap.js b/lib/coverageMap.js index 813c891..a3cec78 100644 --- a/lib/coverageMap.js +++ b/lib/coverageMap.js @@ -17,10 +17,13 @@ module.exports = class CoverageMap { constructor() { this.coverage = {}; + this.assertCoverage = {}; this.lineTopics = []; this.functionTopics = []; this.branchTopics = []; this.statementTopics = []; + this.assertPreTopics = []; + this.assertPostTopics = []; } /** @@ -42,6 +45,7 @@ module.exports = class CoverageMap { statementMap: {}, branchMap: {}, }; + this.assertCoverage[canonicalContractPath] = { }; info.runnableLines.forEach((item, idx) => { this.coverage[canonicalContractPath].l[info.runnableLines[idx]] = 0; @@ -53,6 +57,10 @@ module.exports = class CoverageMap { this.coverage[canonicalContractPath].branchMap = info.branchMap; for (let x = 1; x <= Object.keys(info.branchMap).length; x++) { this.coverage[canonicalContractPath].b[x] = [0, 0]; + this.assertCoverage[canonicalContractPath][x] = { + preEvents: 0, + postEvents: 0, + }; } this.coverage[canonicalContractPath].statementMap = info.statementMap; for (let x = 1; x <= Object.keys(info.statementMap).length; x++) { @@ -69,13 +77,17 @@ module.exports = class CoverageMap { const fnHash = keccakhex('__FunctionCoverage' + info.contractName + '(string,uint256)'); const branchHash = keccakhex('__BranchCoverage' + info.contractName + '(string,uint256,uint256)'); const statementHash = keccakhex('__StatementCoverage' + info.contractName + '(string,uint256)'); + const assertPreHash = keccakhex('__AssertPreCoverage' + info.contractName + '(string,uint256)'); + const assertPostHash = keccakhex('__AssertPostCoverage' + info.contractName + '(string,uint256)'); this.lineTopics.push(lineHash); this.functionTopics.push(fnHash); this.branchTopics.push(branchHash); this.statementTopics.push(statementHash); + this.assertPreTopics.push(assertPreHash); + this.assertPostTopics.push(assertPostHash); - const topics = `${lineHash}\n${fnHash}\n${branchHash}\n${statementHash}\n`; + const topics = `${lineHash}\n${fnHash}\n${branchHash}\n${statementHash}\n${assertPreHash}\n${assertPostHash}`; fs.appendFileSync('./scTopics', topics); } @@ -106,8 +118,27 @@ module.exports = class CoverageMap { const data = SolidityCoder.decodeParams(['string', 'uint256'], event.data.replace('0x', '')); const canonicalContractPath = data[0]; this.coverage[canonicalContractPath].s[data[1].toNumber()] += 1; + } else if (event.topics.filter(t => this.assertPreTopics.indexOf(t) >= 0).length > 0) { + const data = SolidityCoder.decodeParams(['string', 'uint256'], event.data.replace('0x', '')); + const canonicalContractPath = data[0]; + this.assertCoverage[canonicalContractPath][data[1].toNumber()].preEvents += 1; + } else if (event.topics.filter(t => this.assertPostTopics.indexOf(t) >= 0).length > 0) { + const data = SolidityCoder.decodeParams(['string', 'uint256'], event.data.replace('0x', '')); + const canonicalContractPath = data[0]; + this.assertCoverage[canonicalContractPath][data[1].toNumber()].postEvents += 1; } } + // Finally, interpret the assert pre/post events + Object.keys(this.assertCoverage).forEach(contractPath => { + const contract = this.coverage[contractPath]; + for (let i = 1; i <= Object.keys(contract.b).length; i++) { + const branch = this.assertCoverage[contractPath][i]; + if (branch.preEvents > 0) { + // Then it was an assert branch. + this.coverage[contractPath].b[i] = [branch.postEvents, branch.preEvents - branch.postEvents]; + } + } + }); return Object.assign({}, this.coverage); } }; diff --git a/lib/injector.js b/lib/injector.js index d3f687d..4105768 100644 --- a/lib/injector.js +++ b/lib/injector.js @@ -30,6 +30,19 @@ injector.callEmptyBranchEvent = function injectCallEmptyBranchEvent(contract, fi contract.instrumented.slice(injectionPoint); }; + +injector.callAssertPreEvent = function callAssertPreEvent(contract, fileName, injectionPoint, injection) { + contract.instrumented = contract.instrumented.slice(0, injectionPoint) + + '__AssertPreCoverage' + contract.contractName + '(\'' + fileName + '\',' + injection.branchId + ');\n' + + contract.instrumented.slice(injectionPoint); +}; + +injector.callAssertPostEvent = function callAssertPostEvent(contract, fileName, injectionPoint, injection) { + contract.instrumented = contract.instrumented.slice(0, injectionPoint) + + '__AssertPostCoverage' + contract.contractName + '(\'' + fileName + '\',' + injection.branchId + ');\n' + + contract.instrumented.slice(injectionPoint); +}; + injector.openParen = function injectOpenParen(contract, fileName, injectionPoint, injection) { contract.instrumented = contract.instrumented.slice(0, injectionPoint) + '(' + contract.instrumented.slice(injectionPoint); }; @@ -54,6 +67,9 @@ injector.eventDefinition = function injectEventDefinition(contract, fileName, in 'event __FunctionCoverage' + contract.contractName + '(string fileName, uint256 fnId);\n' + 'event __StatementCoverage' + contract.contractName + '(string fileName, uint256 statementId);\n' + 'event __BranchCoverage' + contract.contractName + '(string fileName, uint256 branchId, uint256 locationIdx);\n' + + 'event __AssertPreCoverage' + contract.contractName + '(string fileName, uint256 branchId);\n' + + 'event __AssertPostCoverage' + contract.contractName + '(string fileName, uint256 branchId);\n' + + contract.instrumented.slice(injectionPoint); }; diff --git a/lib/instrumenter.js b/lib/instrumenter.js index 024b4d8..389549e 100644 --- a/lib/instrumenter.js +++ b/lib/instrumenter.js @@ -186,6 +186,38 @@ instrumenter.instrumentFunctionDeclaration = function instrumentFunctionDeclarat } }; +instrumenter.instrumentAssertOrRequire = function instrumentAssertOrRequire(contract, expression){ + contract.branchId += 1; + const startline = (contract.instrumented.slice(0, expression.start).match(/\n/g) || []).length + 1; + const startcol = expression.start - contract.instrumented.slice(0, expression.start).lastIndexOf('\n') - 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, + }, + }], + }; + createOrAppendInjectionPoint(contract, expression.start, { + type: 'callAssertPreEvent', branchId: contract.branchId, + }); + createOrAppendInjectionPoint(contract, expression.end + 1, { + type: 'callAssertPostEvent', branchId: contract.branchId, + }); +} + instrumenter.instrumentIfStatement = function instrumentIfStatement(contract, expression) { contract.branchId += 1; const startline = (contract.instrumented.slice(0, expression.start).match(/\n/g) || []).length + 1; diff --git a/lib/parse.js b/lib/parse.js index ac16916..3ac5fd4 100644 --- a/lib/parse.js +++ b/lib/parse.js @@ -27,6 +27,9 @@ parse.CallExpression = function parseCallExpression(contract, expression) { // In any given chain of call expressions, only the head callee is an Identifier node if (expression.callee.type === 'Identifier') { instrumenter.instrumentStatement(contract, expression); + if (expression.callee.name === 'assert' || expression.callee.name === 'require') { + instrumenter.instrumentAssertOrRequire(contract, expression); + } parse[expression.callee.type] && parse[expression.callee.type](contract, expression.callee); } else { diff --git a/test/assert.js b/test/assert.js new file mode 100644 index 0000000..e0c21bd --- /dev/null +++ b/test/assert.js @@ -0,0 +1,63 @@ +/* eslint-env node, mocha */ + +const path = require('path'); +const getInstrumentedVersion = require('./../lib/instrumentSolidity.js'); +const util = require('./util/util.js'); +const CoverageMap = require('./../lib/coverageMap'); +const vm = require('./util/vm'); +const assert = require('assert'); + +describe('asserts and requires', () => { + const filePath = path.resolve('./test.sol'); + const pathPrefix = './'; + + before(() => process.env.NO_EVENTS_FILTER = true); + + it.only('should cover assert statements as if they are if statements when they pass', done => { + const contract = util.getCode('assert/Assert.sol'); + const info = getInstrumentedVersion(contract, filePath); + const coverage = new CoverageMap(); + coverage.addContract(info, filePath); + + vm.execute(info.contract, 'a', [true]).then(events => { + const mapping = coverage.generate(events, pathPrefix); + assert.deepEqual(mapping[filePath].l, { + 5: 1, + }); + assert.deepEqual(mapping[filePath].b, { + 1: [1, 0], + }); + assert.deepEqual(mapping[filePath].s, { + 1: 1, + }); + assert.deepEqual(mapping[filePath].f, { + 1: 1, + }); + done(); + }).catch(done); + }); + + it.only('should cover assert statements as if they are if statements when they fail', done => { + const contract = util.getCode('assert/Assert.sol'); + const info = getInstrumentedVersion(contract, filePath); + const coverage = new CoverageMap(); + coverage.addContract(info, filePath); + + vm.execute(info.contract, 'a', [false]).then(events => { + const mapping = coverage.generate(events, pathPrefix); + assert.deepEqual(mapping[filePath].l, { + 5: 1, + }); + assert.deepEqual(mapping[filePath].b, { + 1: [0, 1], + }); + assert.deepEqual(mapping[filePath].s, { + 1: 1, + }); + assert.deepEqual(mapping[filePath].f, { + 1: 1, + }); + done(); + }).catch(done); + }); +}); diff --git a/test/sources/assert/Assert.sol b/test/sources/assert/Assert.sol new file mode 100644 index 0000000..2f5b1d5 --- /dev/null +++ b/test/sources/assert/Assert.sol @@ -0,0 +1,7 @@ +pragma solidity ^0.4.13; + +contract Test { + function a(bool test){ + assert(test); + } +} From 2ead932203a6aba64f9d3733d6deeda6770360ff Mon Sep 17 00:00:00 2001 From: cgewecke Date: Mon, 24 Jul 2017 13:37:28 -0700 Subject: [PATCH 3/7] Make tests work --- .gitignore | 1 + lib/coverageMap.js | 2 +- package.json | 1 + test/assert.js | 55 ++++++++++++++++++++++-- test/conditional.js | 2 - test/expressions.js | 2 - test/function.js | 14 +++--- test/if.js | 2 - test/loops.js | 2 - test/return.js | 3 +- test/sources/assert/RequireMultiline.sol | 9 ++++ test/sources/cli/Events.sol | 13 ++++++ test/statements.js | 2 - test/util/util.js | 3 +- test/util/vm.js | 19 ++++---- test/zeppelin.js | 3 +- 16 files changed, 96 insertions(+), 37 deletions(-) create mode 100644 test/sources/assert/RequireMultiline.sol diff --git a/.gitignore b/.gitignore index 70336c6..50c9a4e 100644 --- a/.gitignore +++ b/.gitignore @@ -6,3 +6,4 @@ coverage/ node_modules/ .changelog .DS_Store +package-lock.json diff --git a/lib/coverageMap.js b/lib/coverageMap.js index a3cec78..89d994c 100644 --- a/lib/coverageMap.js +++ b/lib/coverageMap.js @@ -87,7 +87,7 @@ module.exports = class CoverageMap { this.assertPreTopics.push(assertPreHash); this.assertPostTopics.push(assertPostHash); - const topics = `${lineHash}\n${fnHash}\n${branchHash}\n${statementHash}\n${assertPreHash}\n${assertPostHash}`; + const topics = `${lineHash}\n${fnHash}\n${branchHash}\n${statementHash}\n${assertPreHash}\n${assertPostHash}\n`; fs.appendFileSync('./scTopics', topics); } diff --git a/package.json b/package.json index bd744ad..92564fc 100644 --- a/package.json +++ b/package.json @@ -39,6 +39,7 @@ "eslint-plugin-import": "^2.2.0", "eslint-plugin-mocha": "^4.8.0", "ethereumjs-account": "~2.0.4", + "ethereumjs-vm": "https://github.com/sc-forks/ethereumjs-vm-sc.git#3.0.3", "ethereumjs-tx": "^1.2.2", "ethereumjs-util": "^5.0.1", "istanbul": "^0.4.5", diff --git a/test/assert.js b/test/assert.js index e0c21bd..04e38a7 100644 --- a/test/assert.js +++ b/test/assert.js @@ -11,9 +11,7 @@ describe('asserts and requires', () => { const filePath = path.resolve('./test.sol'); const pathPrefix = './'; - before(() => process.env.NO_EVENTS_FILTER = true); - - it.only('should cover assert statements as if they are if statements when they pass', done => { + it('should cover assert statements as if they are if statements when they pass', done => { const contract = util.getCode('assert/Assert.sol'); const info = getInstrumentedVersion(contract, filePath); const coverage = new CoverageMap(); @@ -37,7 +35,7 @@ describe('asserts and requires', () => { }).catch(done); }); - it.only('should cover assert statements as if they are if statements when they fail', done => { + it('should cover assert statements as if they are if statements when they fail', done => { const contract = util.getCode('assert/Assert.sol'); const info = getInstrumentedVersion(contract, filePath); const coverage = new CoverageMap(); @@ -60,4 +58,53 @@ describe('asserts and requires', () => { done(); }).catch(done); }); + + it('should cover multi-line require statements as if they are if statements when they pass', done => { + const contract = util.getCode('assert/RequireMultiline.sol'); + const info = getInstrumentedVersion(contract, filePath); + const coverage = new CoverageMap(); + coverage.addContract(info, filePath); + + vm.execute(info.contract, 'a', [true, true, true]).then(events => { + const mapping = coverage.generate(events, pathPrefix); + assert.deepEqual(mapping[filePath].l, { + 5: 1, + }); + assert.deepEqual(mapping[filePath].b, { + 1: [1, 0], + }); + assert.deepEqual(mapping[filePath].s, { + 1: 1, + }); + assert.deepEqual(mapping[filePath].f, { + 1: 1, + }); + done(); + }).catch(done); + }); + + it('should cover multi-line require statements as if they are if statements when they fail', done => { + const contract = util.getCode('assert/RequireMultiline.sol'); + const info = getInstrumentedVersion(contract, filePath); + const coverage = new CoverageMap(); + coverage.addContract(info, filePath); + + vm.execute(info.contract, 'a', [true, true, false]).then(events => { + const mapping = coverage.generate(events, pathPrefix); + assert.deepEqual(mapping[filePath].l, { + 5: 1, + }); + assert.deepEqual(mapping[filePath].b, { + 1: [0, 1], + }); + assert.deepEqual(mapping[filePath].s, { + 1: 1, + }); + assert.deepEqual(mapping[filePath].f, { + 1: 1, + }); + done(); + }).catch(done); + }); + }); diff --git a/test/conditional.js b/test/conditional.js index 17e848e..582385f 100644 --- a/test/conditional.js +++ b/test/conditional.js @@ -11,8 +11,6 @@ describe('conditional statements', () => { const filePath = path.resolve('./test.sol'); const pathPrefix = './'; - before(() => process.env.NO_EVENTS_FILTER = true); - it('should cover a conditional that reaches the consequent (same-line)', done => { const contract = util.getCode('conditional/sameline-consequent.sol'); const info = getInstrumentedVersion(contract, filePath); diff --git a/test/expressions.js b/test/expressions.js index 0ce9290..fd3558b 100644 --- a/test/expressions.js +++ b/test/expressions.js @@ -13,8 +13,6 @@ const path = require('path'); describe('generic expressions', () => { const filePath = path.resolve('./test.sol'); - before(() => process.env.NO_EVENTS_FILTER = true); - it('should compile after instrumenting a single binary expression', () => { const contract = util.getCode('expressions/single-binary-expression.sol'); const info = getInstrumentedVersion(contract, filePath); diff --git a/test/function.js b/test/function.js index 70340f1..2deb735 100644 --- a/test/function.js +++ b/test/function.js @@ -17,8 +17,6 @@ describe('function declarations', () => { const filePath = path.resolve('./test.sol'); const pathPrefix = './'; - before(() => process.env.NO_EVENTS_FILTER = true); - it('should compile after instrumenting an ordinary function declaration', () => { const contract = util.getCode('function/function.sol'); const info = getInstrumentedVersion(contract, 'test.sol'); @@ -101,15 +99,15 @@ describe('function declarations', () => { vm.execute(info.contract, 'a', []).then(events => { const mapping = coverage.generate(events, pathPrefix); assert.deepEqual(mapping[filePath].l, { - 9: 0, + 9: 1, }); assert.deepEqual(mapping[filePath].b, {}); assert.deepEqual(mapping[filePath].s, { - 1: 0, + 1: 1, }); assert.deepEqual(mapping[filePath].f, { 1: 0, - 2: 0, + 2: 1, }); done(); }).catch(done); @@ -125,15 +123,15 @@ describe('function declarations', () => { vm.execute(info.contract, 'a', []).then(events => { const mapping = coverage.generate(events, pathPrefix); assert.deepEqual(mapping[filePath].l, { - 10: 0, + 10: 1, }); assert.deepEqual(mapping[filePath].b, {}); assert.deepEqual(mapping[filePath].s, { - 1: 0, + 1: 1, }); assert.deepEqual(mapping[filePath].f, { 1: 0, - 2: 0, + 2: 1, }); done(); }).catch(done); diff --git a/test/if.js b/test/if.js index 7bbf5d6..4c8fd82 100644 --- a/test/if.js +++ b/test/if.js @@ -11,8 +11,6 @@ describe('if, else, and else if statements', () => { const filePath = path.resolve('./test.sol'); const pathPrefix = './'; - before(() => process.env.NO_EVENTS_FILTER = true); - it('should cover an if statement with a bracketed consequent', done => { const contract = util.getCode('if/if-with-brackets.sol'); const info = getInstrumentedVersion(contract, filePath); diff --git a/test/loops.js b/test/loops.js index 64a1961..6cba5e5 100644 --- a/test/loops.js +++ b/test/loops.js @@ -11,8 +11,6 @@ describe('for and while statements', () => { const filePath = path.resolve('./test.sol'); const pathPrefix = './'; - before(() => process.env.NO_EVENTS_FILTER = true); - it('should cover a for statement with a bracketed body (multiline)', done => { const contract = util.getCode('loops/for-with-brackets.sol'); const info = getInstrumentedVersion(contract, filePath); diff --git a/test/return.js b/test/return.js index 299706a..04bfd48 100644 --- a/test/return.js +++ b/test/return.js @@ -5,8 +5,7 @@ const getInstrumentedVersion = require('./../lib/instrumentSolidity.js'); const util = require('./util/util.js'); describe('return statements', () => { - before(() => process.env.NO_EVENTS_FILTER = true); - + it('should compile after instrumenting function that returns true', () => { const contract = util.getCode('return/return.sol'); const info = getInstrumentedVersion(contract, 'test.sol'); diff --git a/test/sources/assert/RequireMultiline.sol b/test/sources/assert/RequireMultiline.sol new file mode 100644 index 0000000..b45bb2e --- /dev/null +++ b/test/sources/assert/RequireMultiline.sol @@ -0,0 +1,9 @@ +pragma solidity ^0.4.13; + +contract Test { + function a(bool a, bool b, bool c){ + require(a && + b && + c); + } +} \ No newline at end of file diff --git a/test/sources/cli/Events.sol b/test/sources/cli/Events.sol index f8cdbde..8a80ca4 100644 --- a/test/sources/cli/Events.sol +++ b/test/sources/cli/Events.sol @@ -2,13 +2,26 @@ pragma solidity ^0.4.3; contract Events { uint x = 0; + bool a; + bool b; event LogEventOne( uint x, address y); event LogEventTwo( uint x, address y); function test(uint val) { + // Assert / Require events + require(true); + + // Contract Events LogEventOne(100, msg.sender); x = x + val; LogEventTwo(200, msg.sender); + + // Branch events + if (true) { + a = false; + } else { + b = false; + } } function getX() returns (uint){ diff --git a/test/statements.js b/test/statements.js index 6e222d2..f2277f3 100644 --- a/test/statements.js +++ b/test/statements.js @@ -17,8 +17,6 @@ describe('generic statements', () => { const filePath = path.resolve('./test.sol'); const pathPrefix = './'; - before(() => process.env.NO_EVENTS_FILTER = true); - it('should compile after instrumenting a single statement (first line of function)', () => { const contract = util.getCode('statements/single.sol'); const info = getInstrumentedVersion(contract, filePath); diff --git a/test/util/util.js b/test/util/util.js index d7b7c9c..f17b162 100644 --- a/test/util/util.js +++ b/test/util/util.js @@ -11,7 +11,8 @@ module.exports.getCode = function getCode(_path) { }; module.exports.report = function report(errors) { - if (errors) { + + if (errors && errors.indexOf('Warning:') !== -1) { throw new Error(`Instrumented solidity invalid: ${errors}`); } }; diff --git a/test/util/vm.js b/test/util/vm.js index 0ebbe16..2bcabbd 100644 --- a/test/util/vm.js +++ b/test/util/vm.js @@ -1,4 +1,6 @@ const solc = require('solc'); +const shell = require('shelljs'); +const fs = require('fs'); const VM = require('ethereumjs-vm'); const Account = require('ethereumjs-account'); const Transaction = require('ethereumjs-tx'); @@ -116,15 +118,14 @@ function callMethod(vm, abi, address, functionName, args) { vm.runTx({ tx, }, (err, results) => { - const seenEvents = []; - results.vm.runState.logs.forEach(log => { - const toWrite = {}; - toWrite.address = log[0].toString('hex'); - toWrite.topics = log[1].map(x => x.toString('hex')); - toWrite.data = log[2].toString('hex'); - seenEvents.push(JSON.stringify(toWrite)); - }); - resolve(seenEvents); + try { + const events = fs.readFileSync('./allFiredEvents').toString().split('\n'); + events.pop(); + shell.rm('./allFiredEvents'); + resolve(events); + } catch (err) { + resolve([]); + } }); }); } diff --git a/test/zeppelin.js b/test/zeppelin.js index 834607c..957f49d 100644 --- a/test/zeppelin.js +++ b/test/zeppelin.js @@ -5,8 +5,7 @@ const getInstrumentedVersion = require('./../lib/instrumentSolidity.js'); const util = require('./util/util.js'); describe('Battery test of production contracts: OpenZeppelin', () => { - before(() => process.env.NO_EVENTS_FILTER = true); - + it('should compile after instrumenting zeppelin-solidity/Bounty.sol', () => { const bounty = getInstrumentedVersion(util.getCode('zeppelin/Bounty.sol'), 'Bounty.sol'); const ownable = getInstrumentedVersion(util.getCode('zeppelin/Ownable.sol'), 'Ownable.sol'); From be99e03f1b800d5edd2766dfbb6e98e5f5dc1c4b Mon Sep 17 00:00:00 2001 From: cgewecke Date: Mon, 24 Jul 2017 14:40:09 -0700 Subject: [PATCH 4/7] Make solc warnings filter work correctly --- test/sources/function/multiple.sol | 2 +- test/sources/statements/compilation-error.sol | 6 ++++++ test/statements.js | 12 ++++++++++++ test/util/util.js | 12 ++++++++---- 4 files changed, 27 insertions(+), 5 deletions(-) create mode 100644 test/sources/statements/compilation-error.sol diff --git a/test/sources/function/multiple.sol b/test/sources/function/multiple.sol index 089eefe..bae4f07 100644 --- a/test/sources/function/multiple.sol +++ b/test/sources/function/multiple.sol @@ -8,7 +8,7 @@ contract Test { function f2(uint x){ x = 2; } address a; - + function f3(uint y){ y = 1; } diff --git a/test/sources/statements/compilation-error.sol b/test/sources/statements/compilation-error.sol new file mode 100644 index 0000000..42a4030 --- /dev/null +++ b/test/sources/statements/compilation-error.sol @@ -0,0 +1,6 @@ +pragma solidity ^0.4.3; + +contract Test { + address a; + address a; +} \ No newline at end of file diff --git a/test/statements.js b/test/statements.js index f2277f3..b620920 100644 --- a/test/statements.js +++ b/test/statements.js @@ -52,6 +52,18 @@ describe('generic statements', () => { util.report(output.errors); }); + it('should NOT pass tests if the contract has a compilation error', () => { + const contract = util.getCode('statements/compilation-error.sol'); + const info = getInstrumentedVersion(contract, filePath); + const output = solc.compile(info.contract, 1); + try { + util.report(output.errors); + assert.fail('WRONG'); // We shouldn't hit this. + } catch (err) { + (err.actual === 'WRONG') ? assert(false): assert(true); + } + }); + it('should cover a statement following a close brace', done => { const contract = util.getCode('statements/post-close-brace.sol'); const info = getInstrumentedVersion(contract, filePath); diff --git a/test/util/util.js b/test/util/util.js index f17b162..36b82ee 100644 --- a/test/util/util.js +++ b/test/util/util.js @@ -11,8 +11,12 @@ module.exports.getCode = function getCode(_path) { }; module.exports.report = function report(errors) { - - if (errors && errors.indexOf('Warning:') !== -1) { - throw new Error(`Instrumented solidity invalid: ${errors}`); - } + + if (errors){ + errors.forEach(error => { + if (error.indexOf('Warning') === -1) { + throw new Error(`Instrumented solidity invalid: ${errors}`); + } + }); + } }; From 3e2a2bbaf092897434749632099b0e061bd22583 Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Tue, 25 Jul 2017 13:03:07 +0100 Subject: [PATCH 5/7] Refactor duplicate code to do with branches --- lib/instrumenter.js | 32 +++++++------------------------- 1 file changed, 7 insertions(+), 25 deletions(-) diff --git a/lib/instrumenter.js b/lib/instrumenter.js index 389549e..840dffd 100644 --- a/lib/instrumenter.js +++ b/lib/instrumenter.js @@ -186,7 +186,7 @@ instrumenter.instrumentFunctionDeclaration = function instrumentFunctionDeclarat } }; -instrumenter.instrumentAssertOrRequire = function instrumentAssertOrRequire(contract, expression){ +instrumenter.addNewBranch = function addNewBranch(contract, expression) { contract.branchId += 1; const startline = (contract.instrumented.slice(0, expression.start).match(/\n/g) || []).length + 1; const startcol = expression.start - contract.instrumented.slice(0, expression.start).lastIndexOf('\n') - 1; @@ -210,38 +210,20 @@ instrumenter.instrumentAssertOrRequire = function instrumentAssertOrRequire(cont }, }], }; +}; + +instrumenter.instrumentAssertOrRequire = function instrumentAssertOrRequire(contract, expression) { + instrumenter.addNewBranch(contract, expression); createOrAppendInjectionPoint(contract, expression.start, { type: 'callAssertPreEvent', branchId: contract.branchId, }); createOrAppendInjectionPoint(contract, expression.end + 1, { type: 'callAssertPostEvent', branchId: contract.branchId, }); -} +}; instrumenter.instrumentIfStatement = function instrumentIfStatement(contract, expression) { - contract.branchId += 1; - const startline = (contract.instrumented.slice(0, expression.start).match(/\n/g) || []).length + 1; - const startcol = expression.start - contract.instrumented.slice(0, expression.start).lastIndexOf('\n') - 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, - }, - }], - }; + instrumenter.addNewBranch(contract, expression); if (expression.consequent.type === 'BlockStatement') { createOrAppendInjectionPoint(contract, expression.consequent.start + 1, { type: 'callBranchEvent', branchId: contract.branchId, locationIdx: 0, From 57ffe3ead4b9b7499fc97140cd843d845a250e38 Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Tue, 25 Jul 2017 13:46:52 +0100 Subject: [PATCH 6/7] Clarify comment explaining what test is doing --- test/function.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/function.js b/test/function.js index 2deb735..dff9745 100644 --- a/test/function.js +++ b/test/function.js @@ -94,8 +94,8 @@ describe('function declarations', () => { const info = getInstrumentedVersion(contract, filePath); const coverage = new CoverageMap(); coverage.addContract(info, filePath); - // The vm runs out of gas here - but we can verify line / statement / fn - // coverage is getting mapped. + // We try and call a contract at an address where it doesn't exist and the VM + // throws, but we can verify line / statement / fn coverage is getting mapped. vm.execute(info.contract, 'a', []).then(events => { const mapping = coverage.generate(events, pathPrefix); assert.deepEqual(mapping[filePath].l, { From d5d86be4b7eb57a09bf4f9fb5899ccede0426b29 Mon Sep 17 00:00:00 2001 From: Alex Rea Date: Tue, 25 Jul 2017 14:10:25 +0100 Subject: [PATCH 7/7] Add documentation about assert/require branches --- README.md | 1 + docs/faq.md | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/README.md b/README.md index 0279234..70a56c1 100644 --- a/README.md +++ b/README.md @@ -92,6 +92,7 @@ Solutions to common issues people run into using this tool: + [Running out of time (in mocha)](https://github.com/sc-forks/solidity-coverage/blob/master/docs/faq.md#running-out-of-time-in-mocha) + [Using alongside HDWalletProvider](https://github.com/sc-forks/solidity-coverage/blob/master/docs/faq.md#using-alongside-hdwalletprovider) + [Integrating into CI](https://github.com/sc-forks/solidity-coverage/blob/master/docs/faq.md#continuous-integration-installing-metacoin-on-travisci-with-coveralls) ++ [Why are asserts and requires highlighted as branch points?](https://github.com/sc-forks/solidity-coverage/blob/master/docs/faq.md#why-has-my-branch-coverage-decreased-why-is-assert-being-shown-as-a-branch-point) ### Example reports + [metacoin](https://sc-forks.github.io/metacoin/) (Istanbul HTML) diff --git a/docs/faq.md b/docs/faq.md index d58d6e7..040bdf0 100644 --- a/docs/faq.md +++ b/docs/faq.md @@ -152,3 +152,20 @@ And set up an npm script to run the coverage tool like this: "coverage": "SOLIDITY_COVERAGE=true ./node_modules/.bin/solidity-coverage" }, ``` + +### Why has my branch coverage decreased? Why is assert being shown as a branch point? + +`assert` and `require` check whether a condition is true or not. If it is, they allow execution to proceed. If not, they throw, and all changes are reverted. Indeed, prior to [Solidity 0.4.10](https://github.com/ethereum/solidity/releases/tag/v0.4.10), when `assert` and `require` were introduced, this functionality was achieved by code that looked like + +``` +if (!x) throw; +``` +rather than + +``` +require(x) +``` + +Clearly, the coverage should be the same in these situations, as the code is (functionally) identical. Older versions of solidity-coverage did not treat these as branch points, and they were not considered in the branch coverage filter. Newer versions *do* count these as branch points, so if your tests did not include failure scenarios for `assert` or `require`, you may see a decrease in your coverage figures when upgrading `solidity-coverage`. + +If an `assert` or `require` is marked with an `I` in the coverage report, then during your tests the conditional is never true. If it is marked with an `E`, then it is never false.