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

improve expectRevert output #59

Merged
merged 5 commits into from Jul 26, 2019

Conversation

dotrungkien
Copy link
Contributor

fixes #33

  • use AssertionError instead of plain Error as @frangio suggestion
  • remove noisy Returned error: VM Exception while processing transaction: error substring from error.message

@dotrungkien dotrungkien force-pushed the expect-revert-message branch 2 times, most recently from 1925ada to ec36a94 Compare June 26, 2019 11:20
@frangio
Copy link
Contributor

frangio commented Jun 26, 2019

Nice @dotrungkien! Thanks!

Have you manually tested that this works? It would be good to have automated tests for this, which would also simplify testing it on different setups (like different ganache versions). We can open that as a different issue unless you're interested in tackling it as part of this pull request.

@dotrungkien
Copy link
Contributor Author

@frangio thank you for your comment.
I've tested at local that everything is ok.
Can you write more details which test or setup should be added here ? I think different issue is not necessary.

@frangio
Copy link
Contributor

frangio commented Jun 28, 2019

@dotrungkien It would be a great step to add at least one test that the raised AssertionError has its message in the format that we want. So I think you should add one test in expectRevert.test.js that triggers an assertion error, kinda like this:

it('rejects a failed requirement with incorrect expected reason', async function () {
await assertFailure(expectRevert(this.reverter.revertFromRequireWithReason(), 'Wrong reason'));
});

But instead of simply using assertFailure, catching that assertion error and checking that error.message has the right contents.

It might be a good idea to modify assertFailure to return the caught error.

} catch (error) {
return;

@dotrungkien
Copy link
Contributor Author

dotrungkien commented Jun 28, 2019

@frangio
according to your comment, i've improved the implementation of AssertionError; and since we need to confirm the message is right or not, i've used throw AssertionError({message, expected, actual}) instead of assert.fail([message]) and then check the message in assertFailue.

thus, we not need to edit or add more test into expectRevert.test.js, because pass this case is just enough, i think:

it('rejects a failed requirement with incorrect expected reason', async function () {
await assertFailure(expectRevert(this.reverter.revertFromRequireWithReason(), 'Wrong reason'));
});

all test passed with both ganache version above and bellow 2.2.0.

Copy link
Contributor

@frangio frangio left a comment

Choose a reason for hiding this comment

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

Implemented the missing parts myself. Thanks @dotrungkien!

@frangio frangio merged commit 2522e0b into OpenZeppelin:master Jul 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve expectRevert output
2 participants