From 323d1fa9415695f9132af17a9ebd57642afb7f29 Mon Sep 17 00:00:00 2001 From: Facundo Spagnuolo Date: Thu, 28 Dec 2017 00:39:55 -0300 Subject: [PATCH] Refactor assert revert helper to encapsulate promises (#628) --- test/BasicToken.test.js | 16 +++------------- test/Claimable.test.js | 17 ++++------------- test/DayLimit.test.js | 31 +++++------------------------- test/LimitBalance.test.js | 18 +++--------------- test/Ownable.test.js | 16 +++------------- test/Pausable.test.js | 24 +++++------------------ test/PausableToken.test.js | 18 ++++-------------- test/SafeMath.test.js | 16 +++------------- test/StandardToken.test.js | 37 ++++++------------------------------ test/helpers/assertRevert.js | 10 ++++++++-- 10 files changed, 44 insertions(+), 159 deletions(-) diff --git a/test/BasicToken.test.js b/test/BasicToken.test.js index 67638ea4b..54abc805a 100644 --- a/test/BasicToken.test.js +++ b/test/BasicToken.test.js @@ -1,4 +1,4 @@ -const assertRevert = require('./helpers/assertRevert'); +import assertRevert from './helpers/assertRevert'; var BasicTokenMock = artifacts.require('mocks/BasicTokenMock.sol'); @@ -23,21 +23,11 @@ contract('BasicToken', function (accounts) { it('should throw an error when trying to transfer more than balance', async function () { let token = await BasicTokenMock.new(accounts[0], 100); - try { - await token.transfer(accounts[1], 101); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(token.transfer(accounts[1], 101)); }); it('should throw an error when trying to transfer to 0x0', async function () { let token = await BasicTokenMock.new(accounts[0], 100); - try { - await token.transfer(0x0, 100); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(token.transfer(0x0, 100)); }); }); diff --git a/test/Claimable.test.js b/test/Claimable.test.js index 1004e512c..6683ee6ab 100644 --- a/test/Claimable.test.js +++ b/test/Claimable.test.js @@ -1,5 +1,5 @@ -const assertRevert = require('./helpers/assertRevert'); +import assertRevert from './helpers/assertRevert'; var Claimable = artifacts.require('../contracts/ownership/Claimable.sol'); @@ -24,24 +24,15 @@ contract('Claimable', function (accounts) { }); it('should prevent to claimOwnership from no pendingOwner', async function () { - try { - await claimable.claimOwnership({ from: accounts[2] }); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(claimable.claimOwnership({ from: accounts[2] })); }); it('should prevent non-owners from transfering', async function () { const other = accounts[2]; const owner = await claimable.owner.call(); + assert.isTrue(owner !== other); - try { - await claimable.transferOwnership(other, { from: other }); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(claimable.transferOwnership(other, { from: other })); }); describe('after initiating a transfer', function () { diff --git a/test/DayLimit.test.js b/test/DayLimit.test.js index a02b40f62..8e4d28071 100644 --- a/test/DayLimit.test.js +++ b/test/DayLimit.test.js @@ -2,7 +2,7 @@ import latestTime from './helpers/latestTime'; import { increaseTimeTo, duration } from './helpers/increaseTime'; -const assertRevert = require('./helpers/assertRevert'); +import assertRevert from './helpers/assertRevert'; const DayLimitMock = artifacts.require('mocks/DayLimitMock.sol'); @@ -34,13 +34,7 @@ contract('DayLimit', function (accounts) { await dayLimit.attemptSpend(8); let spentToday = await dayLimit.spentToday(); assert.equal(spentToday, 8); - - try { - await dayLimit.attemptSpend(3); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(dayLimit.attemptSpend(3)); }); it('should allow spending if daily limit is reached and then set higher', async function () { @@ -48,12 +42,7 @@ contract('DayLimit', function (accounts) { let spentToday = await dayLimit.spentToday(); assert.equal(spentToday, 8); - try { - await dayLimit.attemptSpend(3); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(dayLimit.attemptSpend(3)); spentToday = await dayLimit.spentToday(); assert.equal(spentToday, 8); @@ -68,12 +57,7 @@ contract('DayLimit', function (accounts) { let spentToday = await dayLimit.spentToday(); assert.equal(spentToday, 8); - try { - await dayLimit.attemptSpend(3); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(dayLimit.attemptSpend(3)); spentToday = await dayLimit.spentToday(); assert.equal(spentToday, 8); @@ -91,12 +75,7 @@ contract('DayLimit', function (accounts) { let spentToday = await dayLimit.spentToday(); assert.equal(spentToday, 8); - try { - await dayLimit.attemptSpend(3); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(dayLimit.attemptSpend(3)); spentToday = await dayLimit.spentToday(); assert.equal(spentToday, 8); diff --git a/test/LimitBalance.test.js b/test/LimitBalance.test.js index bebab9f98..db9c7d3d3 100644 --- a/test/LimitBalance.test.js +++ b/test/LimitBalance.test.js @@ -1,6 +1,5 @@ - +import assertRevert from './helpers/assertRevert'; var LimitBalanceMock = artifacts.require('mocks/LimitBalanceMock.sol'); -const assertRevert = require('./helpers/assertRevert'); contract('LimitBalance', function (accounts) { let lb; @@ -25,12 +24,7 @@ contract('LimitBalance', function (accounts) { it('shouldnt allow sending above limit', async function () { let amount = 1110; - try { - await lb.limitedDeposit({ value: amount }); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(lb.limitedDeposit({ value: amount })); }); it('should allow multiple sends below limit', async function () { @@ -48,12 +42,6 @@ contract('LimitBalance', function (accounts) { await lb.limitedDeposit({ value: amount }); assert.equal(web3.eth.getBalance(lb.address), amount); - - try { - await lb.limitedDeposit({ value: amount + 1 }); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(lb.limitedDeposit({ value: amount + 1 })); }); }); diff --git a/test/Ownable.test.js b/test/Ownable.test.js index e1920602e..226e68bbf 100644 --- a/test/Ownable.test.js +++ b/test/Ownable.test.js @@ -1,5 +1,5 @@ -const assertRevert = require('./helpers/assertRevert'); +import assertRevert from './helpers/assertRevert'; var Ownable = artifacts.require('../contracts/ownership/Ownable.sol'); @@ -27,21 +27,11 @@ contract('Ownable', function (accounts) { const other = accounts[2]; const owner = await ownable.owner.call(); assert.isTrue(owner !== other); - try { - await ownable.transferOwnership(other, { from: other }); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(ownable.transferOwnership(other, { from: other })); }); it('should guard ownership against stuck state', async function () { let originalOwner = await ownable.owner(); - try { - await ownable.transferOwnership(null, { from: originalOwner }); - assert.fail(); - } catch (error) { - assertRevert(error); - } + await assertRevert(ownable.transferOwnership(null, { from: originalOwner })); }); }); diff --git a/test/Pausable.test.js b/test/Pausable.test.js index d061a8fe2..b5cd63694 100644 --- a/test/Pausable.test.js +++ b/test/Pausable.test.js @@ -1,5 +1,5 @@ -const assertRevert = require('./helpers/assertRevert'); +import assertRevert from './helpers/assertRevert'; const PausableMock = artifacts.require('mocks/PausableMock.sol'); contract('Pausable', function (accounts) { @@ -19,24 +19,14 @@ contract('Pausable', function (accounts) { let count0 = await Pausable.count(); assert.equal(count0, 0); - try { - await Pausable.normalProcess(); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(Pausable.normalProcess()); let count1 = await Pausable.count(); assert.equal(count1, 0); }); it('can not take drastic measure in non-pause', async function () { let Pausable = await PausableMock.new(); - try { - await Pausable.drasticMeasure(); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(Pausable.drasticMeasure()); const drasticMeasureTaken = await Pausable.drasticMeasureTaken(); assert.isFalse(drasticMeasureTaken); }); @@ -64,12 +54,8 @@ contract('Pausable', function (accounts) { let Pausable = await PausableMock.new(); await Pausable.pause(); await Pausable.unpause(); - try { - await Pausable.drasticMeasure(); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + + await assertRevert(Pausable.drasticMeasure()); const drasticMeasureTaken = await Pausable.drasticMeasureTaken(); assert.isFalse(drasticMeasureTaken); diff --git a/test/PausableToken.test.js b/test/PausableToken.test.js index 443b7eda9..3fe723094 100644 --- a/test/PausableToken.test.js +++ b/test/PausableToken.test.js @@ -1,7 +1,7 @@ 'user strict'; -const assertRevert = require('./helpers/assertRevert'); -var PausableTokenMock = artifacts.require('mocks/PausableTokenMock.sol'); +import assertRevert from './helpers/assertRevert'; +var PausableTokenMock = artifacts.require('./mocks/PausableTokenMock.sol'); contract('PausableToken', function (accounts) { let token; @@ -53,21 +53,11 @@ contract('PausableToken', function (accounts) { it('should throw an error trying to transfer while transactions are paused', async function () { await token.pause(); - try { - await token.transfer(accounts[1], 100); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(token.transfer(accounts[1], 100)); }); it('should throw an error trying to transfer from another account while transactions are paused', async function () { await token.pause(); - try { - await token.transferFrom(accounts[0], accounts[1], 100); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(token.transferFrom(accounts[0], accounts[1], 100)); }); }); diff --git a/test/SafeMath.test.js b/test/SafeMath.test.js index f0f3b3697..a1150ffd9 100644 --- a/test/SafeMath.test.js +++ b/test/SafeMath.test.js @@ -1,4 +1,4 @@ -const assertRevert = require('./helpers/assertRevert'); +import assertRevert from './helpers/assertRevert'; const assertJump = require('./helpers/assertJump'); var SafeMathMock = artifacts.require('mocks/SafeMathMock.sol'); @@ -49,22 +49,12 @@ contract('SafeMath', function (accounts) { it('should throw an error on addition overflow', async function () { let a = 115792089237316195423570985008687907853269984665640564039457584007913129639935; let b = 1; - try { - await safeMath.add(a, b); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(safeMath.add(a, b)); }); it('should throw an error on multiplication overflow', async function () { let a = 115792089237316195423570985008687907853269984665640564039457584007913129639933; let b = 2; - try { - await safeMath.multiply(a, b); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(safeMath.multiply(a, b)); }); }); diff --git a/test/StandardToken.test.js b/test/StandardToken.test.js index fdc7c31ff..10eb30a7e 100644 --- a/test/StandardToken.test.js +++ b/test/StandardToken.test.js @@ -1,5 +1,5 @@ -const assertRevert = require('./helpers/assertRevert'); +import assertRevert from './helpers/assertRevert'; var StandardTokenMock = artifacts.require('mocks/StandardTokenMock.sol'); @@ -36,12 +36,7 @@ contract('StandardToken', function (accounts) { it('should throw an error when trying to transfer more than balance', async function () { let token = await StandardTokenMock.new(accounts[0], 100); - try { - await token.transfer(accounts[1], 101); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(token.transfer(accounts[1], 101)); }); it('should return correct balances after transfering from another account', async function () { @@ -61,23 +56,13 @@ contract('StandardToken', function (accounts) { it('should throw an error when trying to transfer more than allowed', async function () { await token.approve(accounts[1], 99); - try { - await token.transferFrom(accounts[0], accounts[2], 100, { from: accounts[1] }); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(token.transferFrom(accounts[0], accounts[2], 100, { from: accounts[1] })); }); it('should throw an error when trying to transferFrom more than _from has', async function () { let balance0 = await token.balanceOf(accounts[0]); await token.approve(accounts[1], 99); - try { - await token.transferFrom(accounts[0], accounts[2], balance0 + 1, { from: accounts[1] }); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(token.transferFrom(accounts[0], accounts[2], balance0 + 1, { from: accounts[1] })); }); describe('validating allowance updates to spender', function () { @@ -107,22 +92,12 @@ contract('StandardToken', function (accounts) { it('should throw an error when trying to transfer to 0x0', async function () { let token = await StandardTokenMock.new(accounts[0], 100); - try { - await token.transfer(0x0, 100); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(token.transfer(0x0, 100)); }); it('should throw an error when trying to transferFrom to 0x0', async function () { let token = await StandardTokenMock.new(accounts[0], 100); await token.approve(accounts[1], 100); - try { - await token.transferFrom(accounts[0], 0x0, 100, { from: accounts[1] }); - assert.fail('should have thrown before'); - } catch (error) { - assertRevert(error); - } + await assertRevert(token.transferFrom(accounts[0], 0x0, 100, { from: accounts[1] })); }); }); diff --git a/test/helpers/assertRevert.js b/test/helpers/assertRevert.js index 2e44d775f..866e211d9 100644 --- a/test/helpers/assertRevert.js +++ b/test/helpers/assertRevert.js @@ -1,3 +1,9 @@ -module.exports = function (error) { - assert.isAbove(error.message.search('revert'), -1, 'Error containing "revert" must be returned'); +export default async promise => { + try { + await promise; + assert.fail('Expected revert not received'); + } catch (error) { + const revertFound = error.message.search('revert') >= 0; + assert(revertFound, `Expected "revert", got ${error} instead`); + } };