Skip to content

feat(testing): verify indexed keys in event assertions #1401

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

turkaturki
Copy link

@turkaturki turkaturki commented Apr 3, 2025

Fixes #1054

Add verification of indexed keys in event assertions to ensure proper event
structure testing. This change:

  • Adds verify_indexed_keys() function to check event key matching
  • Implements key verification in is_emitted() alongside data matching
  • Updates EventSpyQueue to handle ordered event assertions

PR Checklist

  • Tests
  • Added entry to CHANGELOG.md

@turkaturki
Copy link
Author

Hey @ericnordelo , could you please review this PR when you get a chance?

@turkaturki
Copy link
Author

Hi @ericnordelo , just following up on this. Would appreciate it if you could take a look at the PR when you have some time. Happy to address any feedback. Thanks!

@ericnordelo
Copy link
Member

Hey. Thanks again for taking the time. I will review it it early this week. Thanks for your patience!

- Tests will fail if indexed fields are changed in event definitions
-  Must explicitly specify expected indexed fields
@turkaturki
Copy link
Author

Thanks for the feedback! @ericnordelo I understand your point about that approach potentially being redundant. I've updated the implementation and changed the approach based on your comment. Could you please take a look and let me know if I'm heading in the right direction or if there’s anything else I should adjust?

@turkaturki
Copy link
Author

Thanks for the clarification! I've reverted is_emitted to its original form.

@turkaturki
Copy link
Author

Hi @ericnordelo, just following up on this PR. I’ve implemented the suggested changes as discussed. Could you please take another look when you have a moment? Let me know if anything else needs adjustment. Thanks again!

Copy link
Member

@ericnordelo ericnordelo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function looks good @turkaturki. Note that we still need to use it in our tests to resolve the issue. We should add it to our event helpers with the appropriate keys for each event in each different module.

…luding AccessControl, Ownable, Account, EthAccount, Votes, Pausable, ERC1155, ERC20, ERC4626, and ERC721. Each test verifies the correct indexed keys for events such as role grants, ownership transfers, and token transfers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check indexed keys in event assertions (snforge migration)
2 participants