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: add additional logs to DSTest abi #1916

Merged
merged 2 commits into from
Jun 11, 2022

Conversation

mds1
Copy link
Collaborator

@mds1 mds1 commented Jun 11, 2022

In foundry-rs/forge-std#86, assertEq overloads were added for testing array equality. They work by hashing the two arrays, and like other assertion helpers they emit an event with the expected vs. actual values if they don't match.

DSTest's test.sol does not have any events for arrays, therefore right now these new logs in forge-std only show up in traces. This PR adds these events to the DSTest ABI so they now show up in the Logs: section when running tests.

cc @ZeroEkkusu / ref foundry-rs/book#372

@ZeroEkkusu
Copy link
Contributor

I've noticed a comma is printed after Expected and Actual instead of a colon:

    Expected, [0, 58958968456, 0]
      Actual, [0, 0, 0]

@mds1
Copy link
Collaborator Author

mds1 commented Jun 11, 2022

Oops missed that, good catch! Should be fixed in de250bd.

One thing this made me realize: most DSTest logs specify the exact type, whereas these just specify that it's an array. Should we renamed them to log_uint_array, log_int_array, log_named_uint_array, and log_named_int_array to be more verbose/consistent?

Worth considering how either convention would scale in the future too, e.g. if we support decoding bytes32 arrays currently we'd keep it as another log_array overload instead of log_bytes32_array.

I personally prefer not needing to specify a type in the event name, but can see an argument for both.

Copy link
Member

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

I'd like to abuse overrides as much as possible here instead of going for verbose naming, so LGTM

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.

3 participants