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.

Sign up for GitHub

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

Jump to bottom

Bring Truffle to 4.0.4 to be able to properly tests asserts on revert. #111

Merged
merged 6 commits into from
Jan 25, 2018
Merged

Bring Truffle to 4.0.4 to be able to properly tests asserts on revert. #111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1e883ce
Brought Truffle to 4.0.4 to be able to properly use asserts on revert.
simondlr Jan 23, 2018
7dccf82
pin package.json versions
simondlr Jan 23, 2018
9ddcfed
Remove reliance on Babel.
simondlr Jan 23, 2018
6416803
Remove babelrc
simondlr Jan 23, 2018
30b2597
Properly remove Babel.
simondlr Jan 23, 2018
b045918
Bumped to Truffle 4.0.5 and removed comment.
simondlr Jan 24, 2018
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
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
},
"homepage": "https://github.com/ConsenSys/Tokens#readme",
"dependencies": {
"truffle": "4.0.1",
"truffle": "4.0.5",
"truffle-hdwallet-provider": "0.0.3"
},
"devDependencies": {
Expand Down
28 changes: 16 additions & 12 deletions test/eip20/eip20.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const { expectThrow } = require('../utils');
const { assertRevert } = require('../helpers/assertRevert');

const EIP20Abstraction = artifacts.require('EIP20');
let HST;
Expand Down Expand Up @@ -38,15 +38,15 @@ contract('EIP20', (accounts) => {
const balanceBefore = await HST.balanceOf.call(accounts[0]);
assert.strictEqual(balanceBefore.toNumber(), 10000);

web3.eth.sendTransaction({ from: accounts[0], to: HST.address, value: web3.toWei('10', 'Ether') }, async (err, res) => {
expectThrow(new Promise((resolve, reject) => {
if (err) reject(err);
await assertRevert(new Promise((resolve, reject) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like using assertRevert on things that return promises, but for things that only take callbacks might it be more legible to just do the check explicitly inside the callback itself? What do you think @maurelian ?

I think this code is right, it's just a little hairy.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I think I've done the same thing in a previous version of the test suite, but it feels like wrapping and unwrapping at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure how to do this without bungling up the usage of async/await?

Here's how I understand it: if it's a callback and not using async/await, then it will resolve in its own asynchronous thread, and then the following lines (49+) will execute independently of the sendTransaction. Won't this break the linear asyncs?

Doesn't this need to be chained as usual then with .then() style promises (or more traditional callback hell)?

Is that the intent? I'm not 100% sure I grok how to fix this?

Copy link
Contributor

@maurelian maurelian Jan 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I didn't consider that there's another await call made after this. I was able to get this work though:

web3.eth.sendTransaction(
  { from: accounts[0], to: HST.address, value: web3.toWei('10', 'Ether') },
  async (err, res) => {
    assert(err);
    assert.strictEqual(res, undefined);

    const balanceAfter = await HST.balanceOf.call(accounts[0]);
    assert.strictEqual(balanceAfter.toNumber(), 10000);
  },
);

And I think one callback is reasonable.

Another option could be to promisify it at the top of the file:

const promiseSend = function (tx) {
  return new Promise((resolve, reject) => {
    web3.eth.sendTransaction(tx, (err, res) => {
      if (err) { reject(err); }
      resolve(res);
    });
  });
};

then the test itself looks like any other one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about it, if we use just the callback, then we have to add a done to this function I think. Because it doesn't return a promise. https://mochajs.org/#asynchronous-code

But the second thing you wrote I think would work @maurelian

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm okay with merging this as-is though and coming back to this question later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think about it, if we use just the callback, then we have to add a done to this function I think. Because it doesn't return a promise.

FWIW, I had tried that, and got a 'resolution method overspecified' error. The sample I shared was working.

web3.eth.sendTransaction({ from: accounts[0], to: HST.address, value: web3.toWei('10', 'Ether') }, (err, res) => {
if (err) { reject(err); }
resolve(res);
}));
});
}));

const balanceAfter = await HST.balanceOf.call(accounts[0]);
assert.strictEqual(balanceAfter.toNumber(), 10000);
});
const balanceAfter = await HST.balanceOf.call(accounts[0]);
assert.strictEqual(balanceAfter.toNumber(), 10000);
});

it('transfers: should transfer 10000 to accounts[1] with accounts[0] having 10000', async () => {
Expand All @@ -55,7 +55,9 @@ contract('EIP20', (accounts) => {
assert.strictEqual(balance.toNumber(), 10000);
});

it('transfers: should fail when trying to transfer 10001 to accounts[1] with accounts[0] having 10000', () => expectThrow(HST.transfer.call(accounts[1], 10001, { from: accounts[0] })));
it('transfers: should fail when trying to transfer 10001 to accounts[1] with accounts[0] having 10000', async () => {
await assertRevert(HST.transfer.call(accounts[1], 10001, { from: accounts[0] }));
});

it('transfers: should handle zero-transfers normally', async () => {
assert(await HST.transfer.call(accounts[1], 0, { from: accounts[0] }), 'zero-transfer has failed');
Expand Down Expand Up @@ -140,16 +142,18 @@ contract('EIP20', (accounts) => {

// FIRST tx done.
// onto next.
expectThrow(HST.transferFrom.call(accounts[0], accounts[2], 60, { from: accounts[1] }));
await assertRevert(HST.transferFrom.call(accounts[0], accounts[2], 60, { from: accounts[1] }));
});

it('approvals: attempt withdrawal from account with no allowance (should fail)', () => expectThrow(HST.transferFrom.call(accounts[0], accounts[2], 60, { from: accounts[1] })));
it('approvals: attempt withdrawal from account with no allowance (should fail)', async () => {
await assertRevert(HST.transferFrom.call(accounts[0], accounts[2], 60, { from: accounts[1] }));
});

it('approvals: allow accounts[1] 100 to withdraw from accounts[0]. Withdraw 60 and then approve 0 & attempt transfer.', async () => {
await HST.approve(accounts[1], 100, { from: accounts[0] });
await HST.transferFrom(accounts[0], accounts[2], 60, { from: accounts[1] });
await HST.approve(accounts[1], 0, { from: accounts[0] });
expectThrow(HST.transferFrom.call(accounts[0], accounts[2], 10, { from: accounts[1] }));
await assertRevert(HST.transferFrom.call(accounts[0], accounts[2], 10, { from: accounts[1] }));
});

it('approvals: approve max (2^256 - 1)', async () => {
Expand Down
11 changes: 11 additions & 0 deletions test/helpers/assertRevert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
module.exports = {
assertRevert: 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`);
}
},
};
12 changes: 0 additions & 12 deletions test/utils.js
View file
Open in desktop

This file was deleted.