feat(test): assertApproxEq, assertNotEq, assertFalse #38

OliverNChalk · 2022-04-18T06:25:16Z

Brief

This PR adds the following assertions:

assertApproxEq(a, b, reason?) [uint256, int256]
assertNotEq(a, b, reason?) [address, bytes, int, uint, string]
assertFalse(a, reason?) [bool]

Deliverables

Unsure if this is of interest at this level or should be pitched to ds-test. It however seems that ds-test is not being actively extended at the moment, hence the PR here. We use a forked version of forge-std and find these asserts useful but understand if this is out of scope/not going to be merged.

Would be good to get a decision/some discussion on where most appropriate to extend the current set of assertions which have some gaps imo.

ultrasecreth · 2022-04-18T11:06:42Z

For what is worth, was going to propose/add similar stuff :)

brockelmore · 2022-04-18T13:24:13Z

@transmissions11 curious your thoughts here, I know you were going to maybe do a clean-room rewrite of some existing contracts into forge-std

brockelmore · 2022-04-19T20:29:39Z

Can you just yonk the various methods you want from here? Have permission from t11 to do so. Have received feedback from people that those methods are generally better implementations

OliverNChalk · 2022-04-20T06:53:28Z

Chucked a few more commits on this, largely driven by the suggestion to use @transmissions11's impls. To summarize, the following should be resolved:

assertApproxEq implementation from t11s
assertRelApproxEq implementation from t11s
assertEq(bool, bool) implementation from t11s (note name is diff)
assertEq(bytes, bytes) implementation from t11s (note name is diff)

Areas to cover/discuss:

Naming of assertEq variants (solmate impl has things like assertBoolEq, where I've tried to align with ds-test's assertEq argument overloading approach)
Maintaining error message support (i did this to align with ds-test but I personally rarely use it and would be happy without)
Scope (i have not picked up things like gas metering, fail(), bound) to keep the scope of PR just to enhancing assets)
Log alignment style, mainly did this to sanity check spacing but unsure if this is wanted in here

mds1

Thanks a lot @OliverNChalk!! Looking forward to having these in forge-std

Naming of assertEq variants (solmate impl has things like assertBoolEq, where I've tried to align with ds-test's assertEq argument overloading approach)

Agree with naming the overloads assertEq instead of solmate's assertBoolEq

Maintaining error message support (i did this to align with ds-test but I personally rarely use it and would be happy without)

Supporting error messages for consistency seems like the right approach, though I also rarely use them

Scope (i have not picked up things like gas metering, fail(), bound) to keep the scope of PR just to enhancing assets)

bound is a very popular solmate helper for fuzz tests that is recommended in the docs, so that one is definitely worth upstreaming here. Gas metering and others are more questionable so we can leave them out for now / discuss in a separate issue if people want to consider them

Log alignment style, mainly did this to sanity check spacing but unsure if this is wanted in here

Sorry can you clarify what you mean here?

src/Test.sol

mds1 · 2022-04-20T13:39:46Z

src/Test.sol

+
+    /*//////////////////////////////////////////////////////////////////////////
+                                    REL APPROX EQUAL
+    //////////////////////////////////////////////////////////////////////////*/


Nit, but really just need one section header for all the assertions to differentiate them from the other helper methods. Maybe just called "Assertions"

Happy to collapse into ASSERTIONS section, only hesitation would be if we wanted to properly overload assertApproxEqRel and assertApproxEqAbs. Then we might want to group these guys in their own region, separate to the assertEq region.

For now if we don't plan to fully do all the overloads in this PR, happy to just collapse to one collection and move on

only hesitation would be if we wanted to properly overload assertApproxEqRel and assertApproxEqAbs

By "properly overload" do you mean with types other than int256/uint256? If so, I think just supporting int256/uint256 would cover most cases and is sufficient.

Either way, both a single Assertions section or two sections (one for assertEqs, one for approx equals) is good with me. I don't feel strongly about this, but see the thumbs up from @ZeroEkkusu so will leave it up to you both 😛

It ends up being 4 sections if you group overloaded functions together. This may be excessive and the argument for would be if we expect more overloads to come down the track - then this makes it blindingly obvious where to put them and whether they comply with existing style/format.

Will defer to @ZeroEkkusu to act as tie breaker and move this forward. For reference here is the commit where I group overloaded assertions together: 924456a

The idea I had was to use the big headers to divide the main parts of the contract:
"STD-CHEATS", "STD-ASSERTIONS", "STD-ERRORS", "STD-STORAGE".

I like the order as is, assertEq --> assertApproxEqAbs... It's logical IMO. Just put assertFalse first. I don't think we need any dividers rn; we can always add them later, as that'd be a non-breaking change.

Okay, summary of changes:

Collapse assertion code regions into one: 99a36d8

Re-order assertFalse: 3c15901

Add STD-CHEATS code region: 113b11c

Changes not done:

Did not add STD-ERRORS or STD-STORAGE as these are standalone libraries, not regions within contract Test

src/Test.sol

OliverNChalk · 2022-04-21T00:41:44Z

Log alignment style, mainly did this to sanity check spacing but unsure if this is wanted in here

Sorry can you clarify what you mean here?

To help ensure the error messaging comes out aligned on the console, the following indentation style is used:

This is somewhat not standard as the ( is aligned on indentation multiples (4 spaces), so wanted to call it out in case people didn't want it in

mds1 · 2022-04-21T00:51:27Z

To help ensure the error messaging comes out aligned on the console, the following indentation style is used:

This is somewhat not standard as the ( is aligned on indentation multiples (4 spaces), so wanted to call it out in case people didn't want it in

Ah I see, this is fine with me, will leave it to anyone else to raise objections! 😅

mds1

LGTM, thanks @OliverNChalk! If you want to add solmate's bound in this PR feel free, otherwise I can PR that in later.

Otherwise @ZeroEkkusu feel free to merge whenever the last open comment is resolved

ZeroEkkusu · 2022-04-21T14:43:16Z

If you include bound in this PR, add it as a std-cheat below deployCode. ^^

OliverNChalk · 2022-04-22T00:39:33Z

If you include bound in this PR, add it as a std-cheat below deployCode. ^^

Have opted to leave this out as it feels most appropriate in a separate PR. I'll throw that up post merge if no one beats me to the punchb

ZeroEkkusu

Added a few comments. Otherwise, looks good!

ZeroEkkusu · 2022-04-22T10:34:08Z

src/Test.sol

+            emit log_named_decimal_int ("     % Delta", percentDelta, 18);
+            fail();
+        }
+    }
 }




We should add a header here before stdError "STD-ERRORS" to separate them from the assertions.

done, can resolve if happy

ZeroEkkusu · 2022-04-22T10:40:05Z

src/Test.sol

@@ -159,7 +359,7 @@ library stdError {
 struct StdStorage {


Same here "STD-STORAGE"

done, can resolve if happy

ZeroEkkusu

Lgtm, thanks!

feat(test): assertApproxEq, assertNotEq, assertFalse

e17d6f0

OliverNChalk added 7 commits April 20, 2022 16:17

refactor: yoink assertApproxEq from t11s

1e3b6a2

feat(test): yoink assertRelApproxEq from t11s

fb089f0

style(test): align multi-line log messages for character alignment

dfaa2d5

fix(test): correct yoink/translation errors

7e73c21

feat(test): add more bool asserts, t11s assertFalse

1a78d1b

feat(test): add assertEq(bool, err)

3e8d693

feat(test): add assertEq(bytes, bytes)

b61c6c4

mds1 reviewed Apr 20, 2022

View reviewed changes

OliverNChalk added 3 commits April 21, 2022 10:27

refactor(test): standardized approx eq assertion naming

51ff84f

refactor(test): drop assertNotEq as it's bad practice

3e0ef00

refactor(test): improve assertEq(bool,bool) error messaging

1f08a76

OliverNChalk force-pushed the master branch from 97f5f77 to 1f08a76 Compare April 21, 2022 00:39

OliverNChalk added 4 commits April 21, 2022 11:25

fix(test): remove duplicate assertEq(bool,bool)

f0e1ba9

fix(test): use log instead of log_named_string

03355c9

feat(test): add int256 impls for assertApproxEqRel

2c23374

style(test): re-order code and group overloads together

924456a

mds1 approved these changes Apr 21, 2022

View reviewed changes

OliverNChalk added 3 commits April 22, 2022 10:32

style(test): use single STD-ASSERTIONS code region

99a36d8

style(test): declare assertFalse before assertEq

3c15901

style(test): add STD-CHEATS code region

113b11c

ZeroEkkusu reviewed Apr 22, 2022

View reviewed changes

style: add STD-ERRORS & STD-STORAGE delimters

b434995

OliverNChalk requested a review from ZeroEkkusu April 22, 2022 13:12

ZeroEkkusu approved these changes Apr 22, 2022

View reviewed changes

ZeroEkkusu merged commit 6329843 into foundry-rs:master Apr 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(test): assertApproxEq, assertNotEq, assertFalse #38

feat(test): assertApproxEq, assertNotEq, assertFalse #38

OliverNChalk commented Apr 18, 2022

ultrasecreth commented Apr 18, 2022

brockelmore commented Apr 18, 2022

brockelmore commented Apr 19, 2022 •

edited

Loading

OliverNChalk commented Apr 20, 2022 •

edited

Loading

mds1 left a comment

mds1 Apr 20, 2022

OliverNChalk Apr 21, 2022 •

edited

Loading

mds1 Apr 21, 2022

OliverNChalk Apr 21, 2022

ZeroEkkusu Apr 21, 2022

OliverNChalk Apr 22, 2022

OliverNChalk commented Apr 21, 2022

mds1 commented Apr 21, 2022

mds1 left a comment

ZeroEkkusu commented Apr 21, 2022

OliverNChalk commented Apr 22, 2022

ZeroEkkusu left a comment

ZeroEkkusu Apr 22, 2022

OliverNChalk Apr 22, 2022

ZeroEkkusu Apr 22, 2022

OliverNChalk Apr 22, 2022

ZeroEkkusu left a comment

feat(test): assertApproxEq, assertNotEq, assertFalse #38

feat(test): assertApproxEq, assertNotEq, assertFalse #38

Conversation

OliverNChalk commented Apr 18, 2022

Brief

Deliverables

ultrasecreth commented Apr 18, 2022

brockelmore commented Apr 18, 2022

brockelmore commented Apr 19, 2022 • edited Loading

OliverNChalk commented Apr 20, 2022 • edited Loading

mds1 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliverNChalk Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OliverNChalk commented Apr 21, 2022

mds1 commented Apr 21, 2022

mds1 left a comment

Choose a reason for hiding this comment

ZeroEkkusu commented Apr 21, 2022

OliverNChalk commented Apr 22, 2022

ZeroEkkusu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZeroEkkusu left a comment

Choose a reason for hiding this comment

brockelmore commented Apr 19, 2022 •

edited

Loading

OliverNChalk commented Apr 20, 2022 •

edited

Loading

OliverNChalk Apr 21, 2022 •

edited

Loading