refactor(test): assertApproxEq{Rel,Abs} unit tests, improvements. & fixes

[OliverNChalk]

Brief

This PR adds unit tests for the recently added methods:

• assertFalse(bool)
• assertEq(bool,bool)
• assertEq(bytes,bytes)
• assertApproxEqAbs(uint256,uint256,uint256)
• assertApproxEqAbs(int256,int256,uint256)
• assertApproxEqRel(uint256,uint256,uint256)
• assertApproxEqRel(int256,int256,uint256)

In the quest to unit test these components, the following changes were also made:

• Introduced and tested deltaMaths library (used in relative and absolute approximation)
• Aligned visibility of asserts on internal virtual (2 funcs didn't have virtual)
• BREAKING: assertApproxEq{Abs,Rel}(int256,int265,int256) -> assertApproxEq{Abs,Rel}(int256,int265,uint256), this is because a delta cannot be negative (i think :P)

Next Steps

Hopefully there isn't anything too controversial in here, so should just be a matter of code reviewing & merging once happy. @ZeroEkkusu I did wanna call out that for the code blocks I use the following thought process (excerpt from a personal repo), let me know if you agree/disagree (saw you re-align some of the previous blocks):

    // NB: code blocks extend from 4 to col 80, in-line with markdown standards
    // NB: code block name starts at 37:
    //
    //     length = (80 - 4)
    //     offset = 4
    //     text_len = 12
    //
    //     start_index = offset + length / 2 - text_len / 2
    //     start_index = 36
    //
    //     due to how editors count columns (starting at 1), this means the
    //     first character will be placed at col 37, i.e. col 36 will still be
    //     whitespace.
    //
    //     if your title is less than 12 chars, it will be left of center
    //     if your title is more than 12 chars, it will be right of center
    //     this is fine, do not attempt to center your title, just start typing
    //     at col 37 and beyond

    /*//////////////////////////////////////////////////////////////////////////
                                    CONSTANTS
    //////////////////////////////////////////////////////////////////////////*/

Resolves

Closes: #49

[OliverNChalk]

CC: @ZeroEkkusu @mds1

[mds1]

Thanks a lot, looks great! Most comments are nits on style, and one where I realized we're checking relative tolerance incorrectly

BREAKING: assertApproxEq{Abs,Rel}(int256,int265,int256) -> assertApproxEq{Abs,Rel}(int256,int265,uint256), this is because a delta cannot be negative (i think :P)

Yes good call here, uint256 is the right choice 👌

[mds1]

I think the assertEq overloads are simple enough that we don't really need tests for them as they may add more clutter than value—even the original ones don't have tests 😅

But you can stack expect emits like this:

vm.expectEmit(true, true, true, true);
emit MyFirstEvent(val1);
vm.expectEmit(true, true, true, true);
emit MySecondEvent(val2);

// This will look for both events in the below call
myContract.myCall();

[OliverNChalk]

Thanks for the expectEmit answer! Re testing assertEq im flexible, it was quite easy to just template all these tests and fill them in (some of it was even macro-able). If you think their value as regression guards is outweighed by the clutter (given it would be impressive to regress assertEq()), then okay for them to be dropped

[mds1]

Hmm, paging @ZeroEkkusu for a second opinion here since I don't feel too strongly

[ZeroEkkusu]

I think we should follow the convention from DSTest for now

[OliverNChalk]

DSTest's convention being to not have a corresponding test file for any assertions?

[ZeroEkkusu]

Hey @OliverNChalk, great additions! I think Forge Std should provide things that are often needed when testing but not natively available in Solidity. I'd do these manually, so a math lib is great.

Agree with Matt on following the DSTest conventions for now.
That being said, we can also reduce the amount of code if we do assertions overriding like this. Let me know what you think.

The test are pretty comprehensive, love it.

I'll leave more comments in conversations.

Edit: I don't mind the code block style! Didn't know it was intentional.

[OliverNChalk]

That being said, we can also reduce the amount of code if we do assertions overriding like this. Let me know what you think.

Do we want to follow DSTest convention here? I feel like we should, I can call the inner assertEq() from assertEq(string memory err)

EDIT: I have optimistically included the change proposed by @ZeroEkkusu, feel free to dispute: 3b3e26b

[ZeroEkkusu]

@OliverNChalk, I'll leave other comments in the convos.

[ZeroEkkusu]

STD-MATH

[OliverNChalk]

Done

[ZeroEkkusu]

Oops looks like there's a bug in my GH UI and the comments were not posted.

Regarding the assertions tests, I wrote:
"I meant the simple ones, assertFalse and assertEq. But totally don't mind if you leave it as is (for more coverage 😀)."

If you don't feel to strongly about either, let's leave them and cover everything in Forge Std from now on. It's the standard testing library for Foundry, after all.

[ZeroEkkusu]

Otherwise, lgtm; @mds1 can take a final look and merge once ready.

[mds1]

LGTM, thanks a lot @OliverNChalk!

[ernestognw]

Just letting you guys know: #53