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

Stop reporting assert statements as branches #556

Merged
merged 1 commit into from
Oct 31, 2020
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
14 changes: 7 additions & 7 deletions lib/coverage.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class Coverage {

constructor() {
this.data = {};
this.assertData = {};
this.requireData = {};
}

/**
Expand All @@ -30,7 +30,7 @@ class Coverage {
statementMap: {},
branchMap: {},
};
this.assertData[contractPath] = { };
this.requireData[contractPath] = { };

info.runnableLines.forEach((item, idx) => {
this.data[contractPath].l[info.runnableLines[idx]] = 0;
Expand All @@ -44,7 +44,7 @@ class Coverage {
this.data[contractPath].branchMap = info.branchMap;
for (let x = 1; x <= Object.keys(info.branchMap).length; x++) {
this.data[contractPath].b[x] = [0, 0];
this.assertData[contractPath][x] = {
this.requireData[contractPath][x] = {
preEvents: 0,
postEvents: 0,
};
Expand Down Expand Up @@ -76,19 +76,19 @@ class Coverage {
case 'function': this.data[contractPath].f[id] = hits; break;
case 'statement': this.data[contractPath].s[id] = hits; break;
case 'branch': this.data[contractPath].b[id][data.locationIdx] = hits; break;
case 'assertPre': this.assertData[contractPath][id].preEvents = hits; break;
case 'assertPost': this.assertData[contractPath][id].postEvents = hits; break;
case 'requirePre': this.requireData[contractPath][id].preEvents = hits; break;
case 'requirePost': this.requireData[contractPath][id].postEvents = hits; break;
}
}

// Finally, interpret the assert pre/post events
const contractPaths = Object.keys(this.assertData);
const contractPaths = Object.keys(this.requireData);

for (let contractPath of contractPaths){
const contract = this.data[contractPath];

for (let i = 1; i <= Object.keys(contract.b).length; i++) {
const branch = this.assertData[contractPath][i];
const branch = this.requireData[contractPath][i];

// Was it an assert branch?
if (branch && branch.preEvents > 0){
Expand Down
8 changes: 4 additions & 4 deletions lib/injector.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,8 @@ class Injector {
contract.instrumented = `${start}else { ${injectable}}${end}`;
}

injectAssertPre(contract, fileName, injectionPoint, injection, instrumentation) {
const type = 'assertPre';
injectRequirePre(contract, fileName, injectionPoint, injection, instrumentation) {
const type = 'requirePre';
const id = `${fileName}:${injection.contractName}`;

const {
Expand All @@ -179,8 +179,8 @@ class Injector {
contract.instrumented = `${start}${injectable}${end}`;
}

injectAssertPost(contract, fileName, injectionPoint, injection, instrumentation) {
const type = 'assertPost';
injectRequirePost(contract, fileName, injectionPoint, injection, instrumentation) {
const type = 'requirePost';
const id = `${fileName}:${injection.contractName}`;

const {
Expand Down
4 changes: 2 additions & 2 deletions lib/parse.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,8 @@ parse.FunctionCall = function(contract, expression) {
// This makes sure we don't instrument a chain of expressions multiple times.
if (expression.expression.type !== 'FunctionCall') {
register.statement(contract, expression);
if (expression.expression.name === 'assert' || expression.expression.name === 'require') {
register.assertOrRequire(contract, expression);
if (expression.expression.name === 'require') {
register.requireBranch(contract, expression);
}
parse[expression.expression.type] &&
parse[expression.expression.type](contract, expression.expression);
Expand Down
10 changes: 5 additions & 5 deletions lib/registrar.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ class Registrar {

/**
* Registers injections for branch measurements. This generic is consumed by
* the `assert/require` and `if` registration methods.
* the `require` and `if` registration methods.
* @param {Object} contract instrumentation target
* @param {Object} expression AST node
*/
Expand Down Expand Up @@ -187,25 +187,25 @@ class Registrar {
};

/**
* Registers injections for assert/require statement measurements (branches)
* Registers injections for require statement measurements (branches)
* @param {Object} contract instrumentation target
* @param {Object} expression AST node
*/
assertOrRequire(contract, expression) {
requireBranch(contract, expression) {
this.addNewBranch(contract, expression);
this._createInjectionPoint(
contract,
expression.range[0],
{
type: 'injectAssertPre',
type: 'injectRequirePre',
branchId: contract.branchId,
}
);
this._createInjectionPoint(
contract,
expression.range[1] + 2,
{
type: 'injectAssertPost',
type: 'injectRequirePost',
branchId: contract.branchId,
}
);
Expand Down
17 changes: 8 additions & 9 deletions test/units/assert.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ describe('asserts and requires', () => {
beforeEach(() => coverage = new Coverage());
after(async() => await api.finish());

it('should cover assert statements as `if` statements when they pass', async function() {
// Assert was covered as a branch up to v0.7.11. But since those
// conditions are never meant to be fullfilled (and assert is really for smt)
// people disliked this...
it('should *not* cover assert statements as branches (pass)', async function() {
const contract = await util.bootstrapCoverage('assert/Assert', api);
coverage.addContract(contract.instrumented, util.filePath);
await contract.instance.a(true);
Expand All @@ -25,9 +28,7 @@ describe('asserts and requires', () => {
assert.deepEqual(mapping[util.filePath].l, {
5: 1,
});
assert.deepEqual(mapping[util.filePath].b, {
1: [1, 0],
});
assert.deepEqual(mapping[util.filePath].b, {});
assert.deepEqual(mapping[util.filePath].s, {
1: 1,
});
Expand All @@ -36,9 +37,9 @@ describe('asserts and requires', () => {
});
});

// NB: Truffle replays failing txs as .calls to obtain the revert reason from the return
// NB: truffle/contract replays failing txs as .calls to obtain the revert reason from the return
// data. Hence the 2X measurements.
it('should cover assert statements as `if` statements when they fail', async function() {
it('should *not* cover assert statements as branches (fail)', async function() {
const contract = await util.bootstrapCoverage('assert/Assert', api);
coverage.addContract(contract.instrumented, util.filePath);

Expand All @@ -48,9 +49,7 @@ describe('asserts and requires', () => {
assert.deepEqual(mapping[util.filePath].l, {
5: 2,
});
assert.deepEqual(mapping[util.filePath].b, {
1: [0, 2],
});
assert.deepEqual(mapping[util.filePath].b, {});
assert.deepEqual(mapping[util.filePath].s, {
1: 2,
});
Expand Down