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

Reverts Test For Calling Same Function Twice Incorrectly Fails #39

Closed JimLynchCodes closed 3 years ago

JimLynchCodes commented 3 years ago

I am making a voting app where user's can call "castVote". If the same user calls it twice then it should revert on the second call.

it('should allow only one vote per user', async () => {

  await subject.castVote(newCandidate.id, { from: voter1 });

  const shouldRevert = await subject.castVote(newCandidate.id, { from: voter1 });

  await truffleAssert.reverts(shouldRevert);
})

The weird thing is that the function does revert, but the test doesn't pass...

Here's the error message I'm seeing for the failing test:

Error: Returned error: VM Exception while processing transaction: revert User has already voted! -- Reason given: User has already voted!.

I would expect this test to pass since it is reverting, and I am expecting it to revert...

Here's the repo for this code: https://github.com/JimLynchCodes/Voting-Example

Thanks!

rkalis commented 3 years ago

Hey, you need to pass a promise into reverts, which means that you should not await the second castVote.

  const shouldRevert = subject.castVote(newCandidate.id, { from: voter1 });

  await truffleAssert.reverts(shouldRevert);
JimLynchCodes commented 3 years ago

Thanks @rkalis. I tried your suggestion, but it doesn't seem to help the situation at all.

it('should allow only one vote per user', async () => {
    await subject.castVote(newCandidate.id, { from: voter1 })

    const shouldRevert = subject.castVote(newCandidate.id, { from: voter1 });

    truffleAssert.reverts(shouldRevert);

  })

Still fails with the error:

Error: Returned error: VM Exception while processing transaction: revert User has already voted! -- Reason given: User has already voted!.

See code in the should-revert branch here: https://github.com/JimLynchCodes/Voting-Example/blob/should-revert/test/voting.js#L67_L74

Returning or awaiting the "truffleAssert.reverts" does not help either...

JimLynchCodes commented 3 years ago

Possibly there is some subtle bug in truffleAssert that has to do with the fact that I am calling the same function twice, and expecting it to fail on the second call? 🤔

rkalis commented 3 years ago

Hmm interesting. Could you make sure that it is definitely the second call that is failing (and not the first one). e.g. you can use a debugger or you can just write the following in your test to make sure it makes it past the first call:

it('should allow only one vote per user', async () => {
    console.log('Before first call');
    await subject.castVote(newCandidate.id, { from: voter1 });
    console.log('After first call');

    const shouldRevert = subject.castVote(newCandidate.id, { from: voter1 });
    await truffleAssert.reverts(shouldRevert);
})
JimLynchCodes commented 3 years ago

Thanks @rkalis

I just tried this, and I do not see the "After first call" logged anywhere in the output...

rkalis commented 3 years ago

Alright, so then that means that your first castVote is reverting. Most likely this is caused by either a bug in the smart contract logic, or by calling the castVote function earlier in your tests (perhaps you're re-using the same deployed contract in earlier tests). I'd recommend re-deploying a new contract for every test as long as it doesn't degrade performance too much. Otherwise, you should at least be aware that the tests affect each other.

JimLynchCodes commented 3 years ago

@rkalis - Can you please clone the repo and run it?

The contract is only two lines of code, and I am already doing everything you are saying here.

function castVote(uint256 id) public hasntAlreadyVoted {        
     votes[id] = votes[id] + 1;        
     hasVoted[msg.sender] = true;    
}

The contract is also being re-deployed in a beforeEach before every test...

JimLynchCodes commented 3 years ago
modifier hasntAlreadyVoted() {
    require(hasVoted[msg.sender] == false, "User has already voted!");
    _;
}
rkalis commented 3 years ago

Looking at your repository, you have the following code in your beforeEach():

subject = await voting.deployed()

contract.deployed() doesn't deploy a new contract, rather it retrieves the deployed contract from the truffle atrtifact JSON files. So you're not re-deploying before every test. So that means that the castVote call in the 'anyone can cast a vote' test sets hasVoted[voter1] to true, which causes the call in the next test to revert.

To deploy a new contract you need to call await voting.new() instead of .deployed().

JimLynchCodes commented 3 years ago

Thank You!!! 🙏 ❤️ 😭