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

feat(cheatcodes): specify mis-matched fields on expectEmit failure #592

Open
EzraWeller opened this issue Jan 26, 2022 · 5 comments
Open
Assignees
Labels
A-cheatcodes Area: cheatcodes C-forge Command: forge Cmd-forge-test Command: forge test D-easy Difficulty: easy good first issue Good for newcomers P-normal Priority: normal T-feature Type: feature T-post-V1 Area: to tackle after V1

Comments

@EzraWeller
Copy link

Component

Forge

Describe the feature you would like

Using the expectEmit function from the vm cheats foundry provides for testing, failures don't provide much information about what was mismatched.

Example:

% forge test -vvvv
compiling...
success.
Running 1 test for RepsTest.json:RepsTest
[FAIL. Reason: Log != expected log] testNewRep():(uint256) (gas: 5917)
Traces:
  [3840356] RepsTest::setUp()
    ├─ → new WETH@0xce71…c246
    │   └─ ← 3486 bytes of code
    ├─ → new Reps@0x185a…1aea
    │   └─ ← 7706 bytes of code
    ├─ [1305] Reps::name()
    │   └─ ← "Test"
    ├─ [1305] Reps::symbol()
    │   └─ ← "TST"
    ├─ [306] Reps::weth()
    │   └─ ← 0xce71065d4017f316ec606fe4422e11eb2c47c246
    ├─ → new CentralizedArbitrator@0xefc5…b132
    │   └─ ← 6101 bytes of code
    └─ ← ()
  [5917] RepsTest::testNewRep()
    ├─ [0] VM::expectEmit(true, true, true, true)
    │   └─ ← ()
    ├─ [2363] Reps::repCount()
    │   └─ ← 0
    └─ ← "Log != expected log"


Failed tests:
[FAIL. Reason: Log != expected log] testNewRep():(uint256) (gas: 5917)

It would be great to get more details about which fields were mismatched in this situation.

Additional context

No response

@EzraWeller EzraWeller added the T-feature Type: feature label Jan 26, 2022
@onbjerg onbjerg added A-cheatcodes Area: cheatcodes Cmd-forge-test Command: forge test C-forge Command: forge good first issue Good for newcomers P-normal Priority: normal D-easy Difficulty: easy labels Jan 26, 2022
@onbjerg onbjerg self-assigned this Mar 29, 2022
@onbjerg onbjerg added this to Foundry Apr 17, 2022
@onbjerg onbjerg moved this to Todo in Foundry Apr 17, 2022
@onbjerg onbjerg added this to the v1.0.0 milestone Jul 1, 2022
@onbjerg onbjerg removed this from the v1.0.0 milestone Aug 23, 2022
@mds1
Copy link
Collaborator

mds1 commented Mar 9, 2023

Closing this in favor of the more generic #928

@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
@mds1 mds1 reopened this Jun 21, 2023
@mds1
Copy link
Collaborator

mds1 commented Jun 21, 2023

@Evalir reopened this as I think it’s a good solution to keeping the current behavior, while also solving #5117 (comment), without having to implement broader improvements/refactors as mentioned in #928

@zerosnacks zerosnacks added this to the v1.0.0 milestone Jul 26, 2024
@zerosnacks zerosnacks changed the title feat: specify mis-matched fields on expectEmit fails feat(cheatcodes): specify mis-matched fields on expectEmit failure Jul 31, 2024
@topocount
Copy link

topocount commented Sep 16, 2024

@zerosnacks @klkvr I'm looking into implementing this (as a follow-up to or as a part of #8686) and am curious what the intended behavior would be for situations where multiple instances of the same event are emitted in a given tx, but none match. Should the params of the events that were emitted but don't quite match be highlighted red?

I think highlighting params of events that match the expected topic 0 but fail another topic or data equivalence check is the way to go, just want to see if there are better alternatives.

edit to add: we should constrain this to an expected address if applicable as well, of course

@topocount
Copy link

topocount commented Sep 16, 2024

Additionally, based on my understanding of the smart contract interfaces, we wouldn't be able to individually highlight data (unindexed) params, right? We'd have to parse and compare the abi-encoded bytestrings to determine which params don't match as well.

@zerosnacks
Copy link
Member

Hi @topocount, great!

Should the params of the events that were emitted but don't quite match be highlighted red?

That would make sense to me, though limited to the mismatch to avoid warning / error fatigue

Additionally, based on my understanding of the smart contract interfaces, we wouldn't be able to individually highlight data (unindexed) params, right? We'd have to parse and compare the abi-encoded bytestrings to determine which params don't match as well.

I believe that to be correct, worst case you could diff the bytestring and just display that

@grandizzy grandizzy removed this from the v1.0.0 milestone Nov 5, 2024
@grandizzy grandizzy added the T-post-V1 Area: to tackle after V1 label Nov 7, 2024
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 Cmd-forge-test Command: forge test D-easy Difficulty: easy good first issue Good for newcomers P-normal Priority: normal T-feature Type: feature T-post-V1 Area: to tackle after V1
Projects
Archived in project
Development

No branches or pull requests

6 participants