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
1 change: 1 addition & 0 deletions .eslintrc
Expand Up @@ -33,6 +33,7 @@
"no-dupe-args": "error",
"no-dupe-keys": "error",
"no-mixed-spaces-and-tabs": ["error", "smart-tabs"],
"no-multi-str": "off",
"no-redeclare": ["error", {"builtinGlobals": true}],
"no-trailing-spaces": ["error", { "skipBlankLines": false }],
"no-undef": "error",
Expand Down
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
34 changes: 0 additions & 34 deletions contracts/Failer.sol

This file was deleted.

38 changes: 38 additions & 0 deletions contracts/Reverter.sol
@@ -0,0 +1,38 @@
pragma solidity ^0.4.24;

contract Reverter {
uint256[] private array;

function dontRevert() public pure {
}

function revertFromRevert() public pure {
revert();
}

function revertFromRevertWithReason() public pure {
revert("Call to revert");
}

function revertFromRequire() public pure {
require(false);
}

function revertFromRequireWithReason() public pure {
require(false, "Failed requirement");
}

function revertFromThrow() public pure {
throw;
}

function revertFromAssert() public pure {
assert(false);
}

function revertFromOutOfGas() public {
for (uint256 i = 0; i < 2**200; ++i) {
array.push(i);
}
}
}
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'),
expectRevert: require('./src/expectRevert'),
singletons: require('./src/singletons'),
time: require('./src/time'),
};
59 changes: 59 additions & 0 deletions src/expectRevert.js
@@ -0,0 +1,59 @@
const { web3 } = require('./setup');

const { expect } = require('chai');
const colors = require('ansi-colors');
const semver = require('semver');

async function expectException (promise, expectedErrors) {
try {
await promise;
} catch (error) {
for (const expectedError of expectedErrors) {
expect(error.message).to.include(expectedError, `Wrong failure type, expected '${expectedError}'`);
}
return;
}

throw Error('Expected failure not received');
}

const expectRevert = async function (promise, expectedError) {
if (!expectedError) {
promise.catch(() => { });
throw Error('No revert reason specified: call expectRevert with the reason string, or use expectRevert.unspecified\
nventuro marked this conversation as resolved.
Show resolved Hide resolved
if your \'require\' statement doesn\'t have one.');
}

// Find out if current version of ganache-core supports revert reason i.e >= 2.2.0.
// https://github.com/trufflesuite/ganache-core/releases/tag/v2.2.0
const nodeInfo = await web3.eth.getNodeInfo();
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')} \
expectRevert: ` + msg);
};

let expectedErrors;

if (matches === null || !(1 in matches)) {
// warn users and skip reason check.
warn('revert reason checking only supported on Ganache v2.2.0 or newer.');
expectedErrors = ['revert'];
} else if (!semver.satisfies(matches[1], '>=2.2.0')) {
nventuro marked this conversation as resolved.
Show resolved Hide resolved
// warn users and skip reason check.
warn(`current version of Ganache (v${matches[1]}) doesn't return revert reason. Use v2.2.0 or newer.`);
expectedErrors = ['revert'];
} else {
// actually perform revert reason check.
expectedErrors = ['revert', expectedError];
nventuro marked this conversation as resolved.
Show resolved Hide resolved
}

await expectException(promise, expectedErrors);
};

expectRevert.assertion = (promise) => expectException(promise, ['invalid opcode']);
frangio marked this conversation as resolved.
Show resolved Hide resolved
expectRevert.outOfGas = (promise) => expectException(promise, ['out of gas']);
expectRevert.unspecified = (promise) => expectException(promise, ['revert']);

module.exports = expectRevert;
71 changes: 0 additions & 71 deletions src/shouldFail.js

This file was deleted.

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 { expectRevert } = require('openzeppelin-test-helpers');
const { expect } = require('chai');

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

contract('Tested', function (accounts) {
context('shouldFail.reverting.withMessage', async function () {
context('expectRevert.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 expectRevert.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, expectRevert } = 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 expectRevert.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 expectRevert.revert(this.contract.nonZeroAddress(constants.ZERO_ADDRESS));
});
});
});