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

Rename shouldFail to expectRevert #39

Merged
merged 15 commits into from May 17, 2019
3 changes: 2 additions & 1 deletion CHANGELOG.md
Expand Up @@ -6,7 +6,8 @@
* `shouldFail.reverting.withMessage` fails if no error string is provided. ([#28](https://github.com/OpenZeppelin/openzeppelin-test-helpers/pull/28)
* Rename `makeInterfaceId` to `makeInterfaceId.ERC165`, and add `makeInterfaceId.ERC1820`. ([#21](https://github.com/OpenZeppelin/openzeppelin-test-helpers/pull/21)
* Add possibility to configure a custom web3 instance. ([#38](https://github.com/OpenZeppelin/openzeppelin-test-helpers/pull/38))

* Rename `shouldFail` to `expectFailure`. ([#39](https://github.com/OpenZeppelin/openzeppelin-test-helpers/pull/39))
nventuro marked this conversation as resolved.
Show resolved Hide resolved

#### How to upgrade from 0.3
nventuro marked this conversation as resolved.
Show resolved Hide resolved
- Change all occurences of `makeInterfaceId` to `makeInterfaceId.ERC165`.
- Some uses of `shouldFail.reverting.withMessage` may fail now. This means it was being used incorrectly and an error string to match against should be added. Alternatively, if the error message is unknown, use `shouldFail.reverting` instead.
nventuro marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
22 changes: 11 additions & 11 deletions README.md
Expand Up @@ -15,7 +15,7 @@ npm install --save-dev openzeppelin-test-helpers

```javascript
// Import all required modules from openzeppelin-test-helpers
const { BN, constants, expectEvent, shouldFail } = require('openzeppelin-test-helpers');
const { BN, constants, expectEvent, expectFailure } = require('openzeppelin-test-helpers');

// Import preferred chai flavor: both expect and should are supported
const { expect } = require('chai');
Expand All @@ -30,7 +30,7 @@ contract('ERC20', ([sender, receiver]) => {

it('reverts when transferring tokens to the zero address', async function () {
// Edge cases that trigger a require statement can be tested for, optionally checking the revert reason as well
await shouldFail.reverting(this.erc20.transfer(constants.ZERO_ADDRESS, this.value, { from: sender }));
await expectFailure.revert(this.erc20.transfer(constants.ZERO_ADDRESS, this.value, { from: sender }));
});

it('emits a Transfer event on successful transfers', async function () {
Expand Down Expand Up @@ -177,14 +177,14 @@ A chai [should](https://www.chaijs.com/api/bdd/) instance, containing the `bignu

---

### shouldFail
Collection of assertions for failures (similar to [chai's `throw`](https://www.chaijs.com/api/bdd/#method_throw)). `shouldFail` will accept any exception type, but more specific functions exist and their usage is encouraged.
### expectFailure
nventuro marked this conversation as resolved.
Show resolved Hide resolved
Collection of assertions for failures (similar to [chai's `throw`](https://www.chaijs.com/api/bdd/#method_throw)). `expectFailure` will accept any exception type, but more specific functions exist and their usage is encouraged.

#### async shouldFail.reverting (promise)
#### async expectFailure.revert (promise)
Only accepts failures caused due to an EVM revert (e.g. a failed `require`).

#### async shouldFail.reverting.withMessage (promise, message)
Like `shouldFail.reverting`, this helper only accepts failures caused due to an EVM revert (e.g. a failed `require`). Furthermore, it checks whether revert reason string includes passed `message`. For example:
#### async expectFailure.revert.withMessage (promise, message)
nventuro marked this conversation as resolved.
Show resolved Hide resolved
Like `expectFailure.revert`, this helper only accepts failures caused due to an EVM revert (e.g. a failed `require`). Furthermore, it checks whether revert reason string includes passed `message`. For example:

```solidity
contract Owned {
Expand All @@ -203,7 +203,7 @@ contract Owned {

Can be tested as follows:
```javascript
const { shouldFail } = require('openzeppelin-test-helpers');
const { expectFailure } = require('openzeppelin-test-helpers');

const Owned = artifacts.require('Owned');

Expand All @@ -214,18 +214,18 @@ contract('Owned', ([owner, other]) => {

describe('doOwnerOperation', function() {
it('Fails when called by a non-owner account', async function () {
await shouldFail.reverting.withMessage(this.owned.doOwnerOperation({ from: other }), "Unauthorized");
await expectFailure.revert.withMessage(this.owned.doOwnerOperation({ from: other }), "Unauthorized");
});
});
...
```

Use this helper to specify the expected error message, when you're testing a function that can revert for multiple reasons.

#### async shouldFail.throwing (promise)
#### async expectFailure.throw (promise)
Only accepts failures due to a failed `assert` (which executes an invalid opcode).

#### async shouldFail.outOfGas (promise)
#### async expectFailure.outOfGas (promise)
Only accepts failures due to the transaction running out of gas.

---
Expand Down
2 changes: 1 addition & 1 deletion index.js
Expand Up @@ -8,7 +8,7 @@ module.exports = {
expectEvent: require('./src/expectEvent'),
makeInterfaceId: require('./src/makeInterfaceId'),
send: require('./src/send'),
shouldFail: require('./src/shouldFail'),
expectFailure: require('./src/expectFailure'),
singletons: require('./src/singletons'),
time: require('./src/time'),
};
34 changes: 11 additions & 23 deletions src/shouldFail.js → src/expectFailure.js
Expand Up @@ -4,7 +4,7 @@ const { expect } = require('chai');
const colors = require('ansi-colors');
const semver = require('semver');

async function shouldFailWithMessage (promise, message) {
async function expectFailureWithMessage (promise, message) {
try {
await promise;
} catch (error) {
Expand All @@ -19,19 +19,7 @@ async function shouldFailWithMessage (promise, message) {
expect.fail('Expected failure not received');
}

async function reverting (promise) {
await shouldFailWithMessage(promise, 'revert');
}

async function throwing (promise) {
await shouldFailWithMessage(promise, 'invalid opcode');
}

async function outOfGas (promise) {
await shouldFailWithMessage(promise, 'out of gas');
}

async function shouldFail (promise) {
async function expectFailure (promise) {
try {
await promise;
} catch (error) {
Expand All @@ -47,25 +35,25 @@ async function withMessage (promise, message) {
const matches = /TestRPC\/v([0-9.]+)\/ethereum-js/.exec(nodeInfo);
const warn = function (msg) {
console.log(`${colors.white.bgBlack('openzeppelin-test-helpers')} ${colors.black.bgYellow('WARN')} \
shouldFail.reverting.withMessage: ` + msg);
expectFailure.revert.withMessage: ` + msg);
};
if (matches === null || !(1 in matches)) {
// warn users and skip reason check.
warn('revert reason checking only supported on Ganache>=2.2.0');
return shouldFail(promise);
return expectFailure(promise);
} else if (!semver.satisfies(matches[1], '>=2.2.0')) {
// warn users and skip reason check.
warn(`current version of Ganache (${matches[1]}) doesn't return revert reason.`);
return shouldFail(promise);
return expectFailure(promise);
} else {
// actually perform revert reason check.
return shouldFailWithMessage(promise, message);
return expectFailureWithMessage(promise, message);
}
}

shouldFail.reverting = reverting;
shouldFail.reverting.withMessage = withMessage;
shouldFail.throwing = throwing;
shouldFail.outOfGas = outOfGas;
expectFailure.revert = (promise) => expectFailureWithMessage(promise, 'revert');
expectFailure.revert.withMessage = withMessage;
expectFailure.throw = (promise) => expectFailureWithMessage(promise, 'invalid opcode');
expectFailure.outOfGas = (promise) => expectFailureWithMessage(promise, 'out of gas');

module.exports = shouldFail;
module.exports = expectFailure;
6 changes: 3 additions & 3 deletions test-integration/ganache-core-2.1.x/test/Tested.test.js
@@ -1,10 +1,10 @@
const { shouldFail } = require('openzeppelin-test-helpers');
const { expectFailure } = require('openzeppelin-test-helpers');
const { expect } = require('chai');

const Tested = artifacts.require('Tested');

contract('Tested', function (accounts) {
context('shouldFail.reverting.withMessage', async function () {
context('expectFailure.revert.withMessage', async function () {
beforeEach(async function () {
this.contract = await Tested.new();
})
Expand All @@ -25,7 +25,7 @@ contract('Tested', function (accounts) {
}
// With that said, following revert should be accepted without regard to the specified
// reason message.
await shouldFail.reverting.withMessage(this.contract.failWithRevertReason(), expectedMessage);
await expectFailure.revert.withMessage(this.contract.failWithRevertReason(), expectedMessage);
})
})
})
Expand Down
@@ -1,4 +1,4 @@
const { BN, constants, expectEvent, shouldFail } = require('openzeppelin-test-helpers');
const { BN, constants, expectEvent, expectFailure } = require('openzeppelin-test-helpers');

const Tested = artifacts.require('Tested');

Expand All @@ -13,7 +13,7 @@ contract('Tested', function (accounts) {
})

it('detect reverts', async function () {
await shouldFail.reverting(this.contract.reverts());
await expectFailure.revert(this.contract.reverts());
});

it('accepts calls with non-zero address', async function () {
Expand All @@ -22,7 +22,7 @@ contract('Tested', function (accounts) {
});

it('reverts with calls with non-zero address', async function () {
await shouldFail.reverting(this.contract.nonZeroAddress(constants.ZERO_ADDRESS));
await expectFailure.revert(this.contract.nonZeroAddress(constants.ZERO_ADDRESS));
});
});
});
28 changes: 14 additions & 14 deletions test/src/expectEvent.test.js
@@ -1,7 +1,7 @@
const { BN } = require('../../src/setup');
const { expect } = require('chai');
const expectEvent = require('../../src/expectEvent');
const shouldFail = require('../../src/shouldFail');
const expectFailure = require('../../src/expectFailure');

const EventEmitter = artifacts.require('EventEmitter');
const IndirectEventEmitter = artifacts.require('IndirectEventEmitter');
Expand Down Expand Up @@ -30,13 +30,13 @@ describe('expectEvent', function () {
});

it('throws if a correct JavaScript number is passed', async function () {
await shouldFail(
await expectFailure(
expectEvent.inConstruction(this.emitter, 'ShortUint', { value: this.constructionValues.uint })
);
});

it('throws if an incorrect value is passed', async function () {
await shouldFail(expectEvent.inConstruction(this.emitter, 'ShortUint', { value: 23 }));
await expectFailure(expectEvent.inConstruction(this.emitter, 'ShortUint', { value: 23 }));
});
});

Expand All @@ -46,7 +46,7 @@ describe('expectEvent', function () {
});

it('throws if an incorrect value is passed', async function () {
await shouldFail(expectEvent.inConstruction(this.emitter, 'Boolean',
await expectFailure(expectEvent.inConstruction(this.emitter, 'Boolean',
{ value: !this.constructionValues.boolean }
));
});
Expand All @@ -58,12 +58,12 @@ describe('expectEvent', function () {
});

it('throws if an incorrect string is passed', async function () {
await shouldFail(expectEvent.inConstruction(this.emitter, 'String', { value: 'ClosedZeppelin' }));
await expectFailure(expectEvent.inConstruction(this.emitter, 'String', { value: 'ClosedZeppelin' }));
});
});

it('throws if an unemitted event is requested', async function () {
await shouldFail(expectEvent.inConstruction(this.emitter, 'UnemittedEvent'));
await expectFailure(expectEvent.inConstruction(this.emitter, 'UnemittedEvent'));
});
});

Expand Down Expand Up @@ -386,25 +386,25 @@ describe('expectEvent', function () {
});

it('throws if an unemitted event is requested', async function () {
await shouldFail(expectEvent.inTransaction(this.txHash, EventEmitter, 'UnemittedEvent',
await expectFailure(expectEvent.inTransaction(this.txHash, EventEmitter, 'UnemittedEvent',
{ value: this.value }
));
});

it('throws if an incorrect string is passed', async function () {
await shouldFail(expectEvent.inTransaction(this.txHash, EventEmitter, 'String',
await expectFailure(expectEvent.inTransaction(this.txHash, EventEmitter, 'String',
{ value: 'ClosedZeppelin' }
));
});

it('throws if an event emitted from other contract is passed', async function () {
await shouldFail(expectEvent.inTransaction(this.txHash, EventEmitter, 'IndirectString',
await expectFailure(expectEvent.inTransaction(this.txHash, EventEmitter, 'IndirectString',
{ value: this.value }
));
});

it('throws if an incorrect emitter is passed', async function () {
await shouldFail(expectEvent.inTransaction(this.txHash, IndirectEventEmitter, 'String',
await expectFailure(expectEvent.inTransaction(this.txHash, IndirectEventEmitter, 'String',
{ value: this.value }
));
});
Expand All @@ -418,25 +418,25 @@ describe('expectEvent', function () {
});

it('throws if an unemitted event is requested', async function () {
await shouldFail(expectEvent.inTransaction(this.txHash, IndirectEventEmitter, 'UnemittedEvent',
await expectFailure(expectEvent.inTransaction(this.txHash, IndirectEventEmitter, 'UnemittedEvent',
{ value: this.value }
));
});

it('throws if an incorrect string is passed', async function () {
await shouldFail(expectEvent.inTransaction(this.txHash, IndirectEventEmitter, 'IndirectString',
await expectFailure(expectEvent.inTransaction(this.txHash, IndirectEventEmitter, 'IndirectString',
{ value: 'ClosedZeppelin' }
));
});

it('throws if an event emitted from other contract is passed', async function () {
await shouldFail(expectEvent.inTransaction(this.txHash, IndirectEventEmitter, 'String',
await expectFailure(expectEvent.inTransaction(this.txHash, IndirectEventEmitter, 'String',
{ value: this.value }
));
});

it('throws if an incorrect emitter is passed', async function () {
await shouldFail(expectEvent.inTransaction(this.txHash, EventEmitter, 'IndirectString',
await expectFailure(expectEvent.inTransaction(this.txHash, EventEmitter, 'IndirectString',
{ value: this.value }
));
});
Expand Down