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: vm.skip(true) #63

Merged
merged 19 commits into from
Apr 18, 2024
Merged

Conversation

simon-something
Copy link
Contributor

Add a vm.skip(true); at the beginning of each function (closes #57)

Without this forge cheatcode, tests are being run and show as "Pass", even when empty (false positive). With this cheatcode, they would show as "Skipped"

function test_WhenXYZ() external {
    vm.skip(true);
    // It should do the thing
(...)

Implementation

A new node is added in the hir tree itself, after reading an optional bulloak.toml (which should contain a no_skip bool, set to true if this cheatcode should not be included in the final test)

@simon-something
Copy link
Contributor Author

@alexfertel I'm opening this so you can take a quick look (I changed a bit the idea/added an optional toml for this kind of optional setup). Let me know what you think and I'll add docs/natspecs afterward, in the meantime, I'll replace the StringLiteral by the correct solang expression in the sol translator

Copy link
Owner

@alexfertel alexfertel left a comment

Choose a reason for hiding this comment

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

Looking very good!

I see some changes, for which I'm pushing back a bit. But otherwise this is super close and I think could be merged.

Do you mind adding an integration test for this as well?

src/hir/hir.rs Outdated Show resolved Hide resolved
src/hir/translator.rs Outdated Show resolved Hide resolved
src/hir/translator.rs Outdated Show resolved Hide resolved
src/sol/translator.rs Outdated Show resolved Hide resolved
@alexfertel
Copy link
Owner

Yo! If you need any help or anything at all lmk, quite excited about this getting merged!

@simon-something
Copy link
Contributor Author

hmm, one small extra thingy to add: we now need forge-std as an import (but I guess it's not too opiniated to add it when a cheatcode is being inlined too, right @alexfertel ?)

@alexfertel
Copy link
Owner

we now need forge-std as an import (but I guess it's not too opiniated to add it when a cheatcode is being inlined too, right @alexfertel ?)

Yup, it's fine to do so since we are tied to foundry anyway for now.

It should be well-documented both in doc comments and the README.

src/sol/fmt.rs Outdated Show resolved Hide resolved
@simon-something simon-something marked this pull request as ready for review April 16, 2024 22:10
Copy link
Owner

@alexfertel alexfertel left a comment

Choose a reason for hiding this comment

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

Yo! This is looking great! 🚀

I left a few small comments and tweaks that should be quick to resolve. Lmk if you have any thoughts or questions!

src/hir/translator.rs Outdated Show resolved Hide resolved
src/hir/translator.rs Outdated Show resolved Hide resolved
src/hir/translator.rs Outdated Show resolved Hide resolved
src/hir/translator.rs Outdated Show resolved Hide resolved
src/hir/translator.rs Outdated Show resolved Hide resolved
src/sol/translator.rs Outdated Show resolved Hide resolved
src/sol/translator.rs Outdated Show resolved Hide resolved
src/sol/translator.rs Outdated Show resolved Hide resolved
tests/scaffold/basic_vm_skip.t.sol Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
simon-something and others added 2 commits April 17, 2024 12:33
- bool not ref
- typos/naming

Co-authored-by: Alexander González <[email protected]>
Copy link

codecov bot commented Apr 17, 2024

Codecov Report

Attention: Patch coverage is 99.25651% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 93.2%. Comparing base (4c2002d) to head (dda4d18).
Report is 2 commits behind head on main.

Additional details and impacted files
Files Coverage Δ
src/check/context.rs 87.6% <100.0%> (ø)
src/check/violation.rs 89.3% <100.0%> (+<0.1%) ⬆️
src/hir/combiner.rs 95.8% <100.0%> (+0.3%) ⬆️
src/hir/hir.rs 64.0% <ø> (ø)
src/hir/mod.rs 100.0% <100.0%> (ø)
src/hir/translator.rs 98.8% <100.0%> (+0.1%) ⬆️
src/scaffold/emitter.rs 98.0% <100.0%> (+0.2%) ⬆️
src/sol/fmt.rs 69.6% <100.0%> (+5.0%) ⬆️
src/sol/translator.rs 98.8% <100.0%> (+0.8%) ⬆️
src/scaffold/mod.rs 88.2% <75.0%> (+4.0%) ⬆️

@@ -236,6 +238,23 @@ impl<'s> Visitor for EmitterI<'s> {

Ok(emitted)
}

fn visit_statement(
Copy link
Contributor Author

@simon-something simon-something Apr 17, 2024

Choose a reason for hiding this comment

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

good bot. This visitor is actually never reached (tried adding a test here, it's only the fmt which is emitting the statements, which kinda look weird actually, or I misunderstood how the emitter/formatter are working together)

It's the last pending thing iic, rest has been fixed

Copy link
Owner

Choose a reason for hiding this comment

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

Note that emitter is used by bulloak scaffold and the formatter is used by bulloak check. Though I'm not sure which formatter you are referring to.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, wasn't super clear, my bad.

Latest commit fixed what I meant (the emitter wasn't emitting the function call)

Copy link
Owner

@alexfertel alexfertel left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for this wonderful contribution! 🚀

I'll run a few sanity checks locally and make a couple of small adjustments before merging and then cut a release that includes this hopefully tomorrow!

@alexfertel alexfertel merged commit a4cb16a into alexfertel:main Apr 18, 2024
7 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.

Add vm.skip(true); to generated tests
2 participants