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(forge) Native assertion cheatcodes #6803

Merged
merged 19 commits into from
Jan 24, 2024

Conversation

klkvr
Copy link
Member

@klkvr klkvr commented Jan 15, 2024

Motivation

Adds native assert* cheatcodes to Vm

Solution

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

great draft!

one nit re error messages, I think we want real error types via thiserror, so we can test formatting etc

maybe an enum or several standalone error structs

enum AssertErr {
 ....
}

wdyt @DaniPopes

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

good draft! looking good so far imo.

Wonder if we can ship improvements incrementally? so we can get the equivalent native cheats in, and then improve errors and diagnostics in followups

use crate::{Cheatcode, Cheatcodes, Result, Vm::*};

#[derive(Debug, thiserror::Error)]
#[error("Assertion failed")]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#[error("Assertion failed")]
#[error("assertion failed")]

crates/cheatcodes/src/test/assert.rs Outdated Show resolved Hide resolved

impl Cheatcode for assertTrue_0Call {
fn apply(&self, _state: &mut Cheatcodes) -> Result {
Ok(assert_true(self.condition).map_err(|_| "Assertion failed")?)
Copy link
Member

Choose a reason for hiding this comment

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

I believe

Suggested change
Ok(assert_true(self.condition).map_err(|_| "Assertion failed")?)
assert_true(self.condition).map_err(|e| e.to_string().into())

Copy link
Member Author

Choose a reason for hiding this comment

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

This kind of conflicts with your other suggestion to rename error message, by default error on assert is "Assertion failed", and in that case it would be "assertion failed"

which one should I do?

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how they conflict, and error messages should be lowercase

crates/cheatcodes/src/test/assert.rs Outdated Show resolved Hide resolved
@klkvr klkvr force-pushed the klkvr/native-cheatcodes branch from 921f1ab to be2523d Compare January 22, 2024 22:33
@klkvr klkvr marked this pull request as ready for review January 22, 2024 22:52
@klkvr klkvr requested review from mattsse and DaniPopes January 22, 2024 22:53
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this looks great!
This should speed up compilation and finally makes tests fail as soon as an assert fails.

one thing we need to decide is, do we want to include them in the Vm interface or mark them internal and switch out all the assert Code in forge-std so function assertEq just calls vm.assertEq?

wdyt @mds1 @DaniPopes @Evalir

@klkvr klkvr changed the title [WIP] feat(forge) Native assertion cheatcodes feat(forge) Native assertion cheatcodes Jan 23, 2024
Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

This looks awesome to me.

I think we should just take the bandaid off and include them in the vm interface.

@klkvr
Copy link
Member Author

klkvr commented Jan 23, 2024

One thing to consider re compile times is that after this PR we will have enough native cheatcodes to drop DSTest dep, but we still would need StdAssertions for assertEqCall* cheats

@mds1
Copy link
Collaborator

mds1 commented Jan 24, 2024

Per @klkvr's comment, since users won't yet be able to find+replace all assertEq* calls due to StdAssertions, I would include them in the public Vm interface and have forge-std functions call the native cheats under the hood to avoid breaking changes.

Once we also have everything in StdAssertions upstreamed, then we can add a deprecating warnings log to the forge-std methods to start the breaking change process (or just do it all of at once like @Evalir said if v1 is coming soon)

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

sgtm!

pending @DaniPopes


impl Cheatcode for assertTrue_0Call {
fn apply(&self, _state: &mut Cheatcodes) -> Result {
Ok(assert_true(self.condition).map_err(|_| "Assertion failed")?)
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how they conflict, and error messages should be lowercase

@klkvr klkvr requested a review from DaniPopes January 24, 2024 22:33
@mattsse mattsse merged commit 4a64380 into foundry-rs:master Jan 24, 2024
20 checks passed
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.

5 participants