Rename shouldFail to expectRevert (#39)

* Rename shouldFail to expectFailure. * Fix exports. * Refactor expectRevert and change API. * Update src/expectRevert.js Co-Authored-By: Francisco Giordano <[email protected]> * Update CHANGELOG.md Co-Authored-By: Francisco Giordano <[email protected]> * Update CHANGELOG.md Co-Authored-By: Francisco Giordano <[email protected]> * Update src/expectRevert.js Co-Authored-By: Francisco Giordano <[email protected]> * Update src/expectRevert.js Co-Authored-By: Francisco Giordano <[email protected]> * Fix output message. * Add assertFailure helper. * Update tests and integration tests to use the new API. * update documentation for expectRevert * Update README.md
OpenZeppelin · May 17, 2019 · 8c13195 · 8c13195
1 parent 91f4022
commit 8c13195
Show file tree

Hide file tree

Showing 16 changed files with 318 additions and 277 deletions.
diff --git a/.eslintrc b/.eslintrc
@@ -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",

diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,10 +6,18 @@
  * `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))
-
+ * Replace `shouldFail` with `expectRevert`, with an improved API. ([#39](https://github.com/OpenZeppelin/openzeppelin-test-helpers/pull/39))
+
 #### How to upgrade from 0.3
 - 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.
+- The `shouldFail` module has been renamed to `expectRevert`, and `reverting.withMessage` is now the main module export.
+
+| 0.3                                | 0.4                          |
+| ---------------------------------- | ---------------------------- |
+| `shouldFail.reverting.withMessage` | `expectRevert`               |
+| `shouldFail.reverting`             | `expectRevert.unspecified`   |
+| `shouldFail.throwing`              | `expectRevert.invalidOpcode` |
+| `shouldFail.outOfGas`              | `expectRevert.outOfGas`      |
 
 ## 0.3.2 (2019-04-10)
  * Update ERC1820Registry address. ([#26](https://github.com/OpenZeppelin/openzeppelin-test-helpers/pull/26))

diff --git a/README.md b/README.md
@@ -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, expectRevert } = require('openzeppelin-test-helpers');
 
 // Import preferred chai flavor: both expect and should are supported
 const { expect } = require('chai');
@@ -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 expectRevert.unspecified(this.erc20.transfer(constants.ZERO_ADDRESS, this.value, { from: sender }));
   });
 
   it('emits a Transfer event on successful transfers', async function () {
@@ -177,14 +177,11 @@ 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.
+### expectRevert
+Collection of assertions for transaction errors (similar to [chai's `throw`](https://www.chaijs.com/api/bdd/#method_throw)).
 
-#### async shouldFail.reverting (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 expectRevert (promise, message)
+This helper asserts that `promise` was rejected due to a reverted transaction, and it will check that the revert reason includes `message`. Use `expectRevert.unspecified` when the revert reason is unknown. For example:
 
 ```solidity
 contract Owned {
@@ -203,7 +200,7 @@ contract Owned {
 
 Can be tested as follows:
 ```javascript
-const { shouldFail } = require('openzeppelin-test-helpers');
+const { expectRevert } = require('openzeppelin-test-helpers');
 
 const Owned = artifacts.require('Owned');
 
@@ -214,19 +211,20 @@ 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 expectRevert(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 expectRevert.unspecified (promise)
+This helper asserts that `promise` was rejected due to a reverted transaction caused by a `require` or `revert` statement.
 
-#### async shouldFail.throwing (promise)
-Only accepts failures due to a failed `assert` (which executes an invalid opcode).
+#### async expectRevert.assertion (promise)
+This helper asserts that `promise` was rejected due to a reverted transaction caused by an `assert` statement or an invalid opcode.
 
-#### async shouldFail.outOfGas (promise)
-Only accepts failures due to the transaction running out of gas.
+#### async expectRevert.outOfGas (promise)
+This helper asserts that `promise` was rejected due to a transaction running out of gas.
 
 ---
 

diff --git a/contracts/Failer.sol b/contracts/Failer.sol
diff --git a/contracts/Reverter.sol b/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);
+        }
+    }
+}
diff --git a/index.js b/index.js
@@ -8,7 +8,7 @@ module.exports = {
   get expectEvent () { return require('./src/expectEvent'); },
   get makeInterfaceId () { return require('./src/makeInterfaceId'); },
   get send () { return require('./src/send'); },
-  get shouldFail () { return require('./src/shouldFail'); },
+  get expectRevert () { return require('./src/expectRevert'); },
   get singletons () { return require('./src/singletons'); },
   get time () { return require('./src/time'); },
 };
diff --git a/src/expectRevert.js b/src/expectRevert.js
@@ -0,0 +1,52 @@
+const { web3 } = require('./setup');
+
+const { expect } = require('chai');
+const colors = require('ansi-colors');
+const semver = require('semver');
+
+async function expectException (promise, expectedError) {
+  try {
+    await promise;
+  } catch (error) {
+    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 \
+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);
+  };
+
+  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.');
+    expectedError = 'revert';
+  } else if (!semver.gte(matches[1], '2.2.0')) {
+    // 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.`);
+    expectedError = 'revert';
+  }
+
+  await expectException(promise, expectedError);
+};
+
+expectRevert.assertion = (promise) => expectException(promise, 'invalid opcode');
+expectRevert.outOfGas = (promise) => expectException(promise, 'out of gas');
+expectRevert.unspecified = (promise) => expectException(promise, 'revert');
+
+module.exports = expectRevert;
diff --git a/src/shouldFail.js b/src/shouldFail.js
diff --git a/test-integration/ganache-core-2.1.x/test/Tested.test.js b/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', async function () {
     beforeEach(async function () {
       this.contract = await Tested.new();
     })
@@ -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(this.contract.failWithRevertReason(), expectedMessage);
       })
     })
   })

diff --git a/test-integration/simple-project-truffle-5.x/test/Tested.test.js b/test-integration/simple-project-truffle-5.x/test/Tested.test.js
@@ -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');
 
@@ -13,7 +13,7 @@ contract('Tested', function (accounts) {
     })
 
     it('detect reverts', async function () {
-      await shouldFail.reverting(this.contract.reverts());
+      await expectRevert.unspecified(this.contract.reverts());
     });
 
     it('accepts calls with non-zero address', async function () {
@@ -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.unspecified(this.contract.nonZeroAddress(constants.ZERO_ADDRESS));
     });
   });
 });
diff --git a/test/helpers/assertFailure.js b/test/helpers/assertFailure.js
@@ -0,0 +1,12 @@
+const { expect } = require('chai');
+
+async function assertFailure (promise) {
+  try {
+    await promise;
+  } catch (error) {
+    return;
+  }
+  expect.fail();
+}
+
+module.exports = assertFailure;