Tests pass with failed assertion in external call

[FrankieIsLost]

Component

Forge

Have you ensured that all of these are up to date?

• Foundry
• Foundryup

What version of Foundry are you on?

forge 0.2.0 (09d53c2 2022-03-31T00:32:45.902442+00:00)

What command(s) is the bug in?

forge test

Operating System

macOS (M1)

Describe the bug

Tests are passing even though assertion fails

How to reproduce

Run forge test -vv with the following test contract:

contract TestMain is DSTest {

    TestUtil util = new TestUtil();

    function testInternalCall() public { 
        shouldFailInternal();
    }

    function shouldFailInternal() public {
        assertTrue(false);
    }

    function testExternalCall() public { 
        util.shouldFailExternal();
    }
}

contract TestUtil is DSTest { 
    function shouldFailExternal() public { 
        assertTrue(false);
    }
}

Expected Result

Both tests should fail

Actual Result

Only the test where the assertion fails in an internal call fails. Test where assertion fails in external call passes.

Severity

This is likely a high sev bug. Code bases with complex tests are likely to refactor their testing into multiple contracts. This bug can cause tests to fail silently.

[onbjerg]

This is not a bug - assertTrue(false) does not revert, it just sets failed = true in the contract. We only check failed on test contracts, not the contracts that it itself deployed.

You should:

• use inheritance to get around this, or
• make TestUtil a library, or
• make TestUtil its own test suite, or
• have your TestUtil contract revert, or
• override failed in your main test

I don't think iterating over all members of the test contract to check failed on every address is something we want to do

• it's expensive and,
• it's likely to produce other weird behavior and,
• it would require every member of the contract to be public (otherwise we're back to square 1), or it would require us to run with tracing always

I also don't think this is how DappTools behaves

[d-xo]

related: dapphub/dapptools#768

[onbjerg]

Interesting solution, addresses my concerns! Thanks for sharing.

I imagine this would require support in https://github.com/dapphub/ds-test/, though?

[gakonst]

Yeah we could have a specific address / storage slot which we treat as the global?

[brockelmore]

dapphub/ds-test#30

implementing a fix upstream that should fix this

[brockelmore]

Fixed upstream :)