Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inline / asymmetric matchers #512

Closed
fubhy opened this issue Jan 19, 2022 · 7 comments
Closed

inline / asymmetric matchers #512

fubhy opened this issue Jan 19, 2022 · 7 comments
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge T-feature Type: feature

Comments

@fubhy
Copy link
Contributor

fubhy commented Jan 19, 2022

For comparing concrete values as part of an assertion, sometimes you'll not be able to reasonably predict the exact value in which case a concept like "asymmetric matchers" (Jest / Jasmine) would come in handy. This can be a concern particularly in integration tests and/or if a value depends on external factors like e.g. block time (rebasing tokens come to mind).

Would it make sense to attempt something along the lines of:

vm.expectEmit();
emit Transfer(address(0xbeef), address(0), vm.lowerThan(12345));

?

@mds1
Copy link
Collaborator

mds1 commented Jan 19, 2022

I've had similar issues, but would prefer to solve it by enabling event emission to come anytime after the call, that way you can just read/compute the expected event data from the new state. For example:

Currently, with the below snippet, I must either (1) implement interest accrual calculations so I know what newInterestRate should be, or (2) just don't test values

vm.expectEmit(false,false,false,false);
emit AccrueInterest(newInterestRate);
contract.accrueInterest();

Instead it'd be nice to do something like:

vm.expectEmit(true,true,true,true);
contract.accrueInterest();
uint256 newInterestRate = contract.interestRate();
emit AccrueInterest(newInterestRate);

Are there cases you can think of where that wouldn't work, and you'd still want to test approximate values instead of exact ones?

@onbjerg onbjerg added A-cheatcodes Area: cheatcodes T-feature Type: feature C-forge Command: forge labels Jan 19, 2022
@fubhy
Copy link
Contributor Author

fubhy commented Jan 20, 2022

I've had similar issues, but would prefer to solve it by enabling event emission to come anytime after the call, that way you can just read/compute the expected event data from the new state.

@mds1 That's a good idea and would indeed likely solve the this particular case.

Slightly off-topic (not relating to inline / asymmetric matchers): What about e.g obscure rounding / off-by-one errors where you want to assert the result of a view function based on previously set fixed input values / fixtures or values obtained from other view functions. Would you then just do something like this:

uint expectedValue = 123;
assertLt(returnedValue, expectedValue + 1);
assertGt(returnedValue, expectedValue - 1);

Or should there be specialized assertion functions like e.g.

assertAround(int a, int b, int margin)

(Just wondering about ergonomics around asserting less deterministic values in tests)

@onbjerg
Copy link
Member

onbjerg commented Jan 20, 2022

@fubhy The assertions are in https://github.com/dapphub/ds-test but really you can use whatever you like: if the test reverts, it fails. If the test does not revert, it passes. So, you can implement more assertions if you want, and even publish a library with the extra ones added if you think it would be useful 🙂

@fubhy
Copy link
Contributor Author

fubhy commented Jan 20, 2022

@fubhy The assertions are in https://github.com/dapphub/ds-test but really you can use whatever you like: if the test reverts, it fails. If the test does not revert, it passes. So, you can implement more assertions if you want, and even publish a library with the extra ones added if you think it would be useful slightly_smiling_face

Yeah I know. I also looked at https://github.com/brockelmore/forge-std already. Just wondering about ergonomics here and what such a std-library should maybe / maybe not contain :-)

(@mds1's suggestion fixes the problem I raised initially in this issue)

@mds1
Copy link
Collaborator

mds1 commented Jan 20, 2022

@fubhy Solmate has assertApproxEq and assertRelApproxEq for comparing values within an absolute or relative tolerance, respectively. These and Solmate's bound method feel like good candidates to add to forge-std if @transmissions11 and @brockelmore have no objections

And regarding the original issue scope:

by enabling event emission to come anytime after the call, that way you can just read/compute the expected event data from the new state

@brockelmore IIRC you mentioned this would be hard to implement, but personally I keep finding myself wanting this, so perhaps worth revisiting?

@fubhy
Copy link
Contributor Author

fubhy commented Jan 20, 2022

@fubhy Solmate has assertApproxEq and assertRelApproxEq for comparing values within an absolute or relative tolerance, respectively. These and Solmate's bound method feel like good candidates to add to forge-std if @transmissions11 and @brockelmore have no objections

Ah nice. I wasn't aware of DSTestPlus. Thanks for the link :-).

Anyways, sorry for going off-topic, back on track: I haven't had a chance to dive deeper into the code base yet so I cannot judge how hard it would be to implement your suggestion @mds1 but I also keep finding myself wanting this. I've migrated a couple of tests over the past few days and ran into this several times. So far, the solution for us has been to just not test the emitted values (only topic0), but that's obviously not ideal.

What about enabling this in two steps? ​

vm.expectEmit("AccrueInterest(uint)");
contract.accrueInterest();

vm.expectPreviousEmit();
emit AccrueInterest(contract.interestRate());

Where expectEmit would store all matching events (just via topic0) emitted in the subsequent call and then expectPreviousEmit would re-check that list of emitted events for its concrete values?

@mds1
Copy link
Collaborator

mds1 commented Mar 9, 2023

Going to close this since there are approximate assertion helpers in forge-std, and you can use vm.snapshot along with vm.revertTo if you can't precompute values so need to extract values from the tx result, revert to a prior state, then expect an emit with those value

@mds1 mds1 closed this as completed Mar 9, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Foundry Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge T-feature Type: feature
Projects
Archived in project
Development

No branches or pull requests

3 participants