rkalis / truffle-assertions

🛠 Assertions and utilities for testing Ethereum smart contracts with Truffle unit tests
https://kalis.me/check-events-solidity-smart-contract-test-truffle/
MIT License
154 stars 20 forks source link

Revert reason checking is not strict #30

Closed smsunarto closed 5 years ago

smsunarto commented 5 years ago

Describe the bug The reason checking doesn't fully check the string 1:1. If the asserted reason is an incomplete subset of the actual reason it will still pass. For instance:

Asserted Reason: "Pausable: pause" Actual Reason: "Pausable: paused"

This is passed by truffle-assertions. This should have failed because "Pausabled: pause" is not the same as "Pausable: paused".

Example test code

  it('should successfully pause contract', async () => {
    await truffleAssert.passes(instance.pause());
    await truffleAssert.fails(
      instance.splitEth(bob, carol, { sender: alice, value: 2 }),
      truffleAssert.ErrorType.REVERT,
      'Pausable: pause'
    ); // This assertion passes 
  });

  it('should successfully pause contract', async () => {
    await truffleAssert.passes(instance.pause());
    await truffleAssert.fails(
      instance.splitEth(bob, carol, { sender: alice, value: 2 }),
      truffleAssert.ErrorType.REVERT,
      'Pausable: paused'
    ); // This assertion also passes
  });

Expected behavior Should have failed the assertion

Environment Information Truffle version: 5.0.21 Web3 version: 1.0.0-beta.37 truffle-assertions version: 0.9.1

rkalis commented 5 years ago

Hey @smsunarto thanks for opening this issue.

The behaviour has been a conscious decision so that you can run the revert assertions without knowing the full revert reason beforehand (e.g. if the revert message depends on variables that the test doesn't care about).

Are you experiencing issues with this in one of your projects? What is the use case where you want to assert the revert message exactly?

smsunarto commented 5 years ago

Hey @smsunarto thanks for opening this issue.

The behaviour has been a conscious decision so that you can run the revert assertions without knowing the full revert reason beforehand (e.g. if the revert message depends on variables that the test doesn't care about).

Are you experiencing issues with this in one of your projects? What is the use case where you want to assert the revert message exactly?

Ahh I see. My bad then.

There's no specific use case, it's just that I stumbled upon this when I made a typo and it seems a bit odd. Perhaps this feature should be elaborated on the README; now that you've told me about it, I found many cool things I can do with it :D

rkalis commented 5 years ago

@smsunarto Good call to add it to the README. I'll update it.

lastperson commented 4 years ago

I'd like to reopen this one, at least partially. I have 2 test cases:

  1. Revert without any reason.
  2. Revert with particular reason.

In case of 1, I have no way to assert that it reverted without the reason, while I want to make sure it didn't fail because of 2.

Someone might argue that I should add a reason for the 1, but the problem is it is Solidity making a call to address without code, resulting in a revert without the reason. I can of course check that the code is there before making a call, but this will just duplicate Solidity's logic itself.