Add vm.skip(true); to generated tests

alexfertel / bulloak

A Solidity test generator based on the Branching Tree Technique.

Apache License 2.0

225 stars 13 forks source link

Add vm.skip(true); to generated tests #57

Closed matmilbury closed 2 months ago

matmilbury commented 5 months ago

I had the following experience with bulloak recently:

At some point during a protocol development, we started outlining trees for methods. Once finished, we scaffolded test files with over 100 test cases.

When we ran tests, it reported all tests succeed even though majority of them were empty tests generated with bulloak.

We manually added vm.skip(true); to over a 100 test cases to have an accurate reporting on how many tests actually pass and how many are waiting to be implemented (skipped).

Then, as we implemented each test, we removed the vm.skip(true); statement.

I believe it would be cool if bulloak could add vm.skip(true); to all scaffolded tests or at least provide a flag to do that.

alexfertel commented 5 months ago

Yeah, that sounds like a great quality-of-life improvement. I need to think about what the design of this feature will be, but it's probably gonna be either a flag or some sort of "template" that bulloak reads from.

drgorillamd commented 3 months ago

I thought the same and had a bit of time this Sunday, I have a (super rough) implementation, which was meant as personal use but happy to pretty-fy it/make the flag functional/etc @alexfertel https://github.com/drgorillamd/bulloak/tree/feat/vm-skip-flag

alexfertel commented 3 months ago

That looks amazing @drgorillamd ! It's pretty close to what I would have done myself. Are you down to open a PR so I can properly review it? I have a few comments, but it shouldn't be much more work.

drgorillamd commented 3 months ago

Sure! Maybe 2 things I'd like to tidy a bit before:

was just wondering if it would make sense to pave the way to support more expressions - prank caller or fuzzing bounds for instance (then I should remove the StringLiteral hack to use the Expression::MemberAccess, I didn't do it because I'm, well, pretty lazy)
I was thinking of just removing the node with this string literal in the hir tree (makes more sense if/for other expressions too then, I guess), but could instead just skip emitting these (feels hacky tho) - did you have something in mind for this @alexfertel ? If not, I'll go for the former option, as it's cleaner imo

alexfertel commented 3 months ago

I haven't formed an opinion yet because I'm unsure just how much we want to add support for -- I'd rather keep bulloak as lean as possible.

That being said, this seems like quite a useful feature, so I'd say we go with 1, since that feels like it gives us more optionality (i.e. flexibility in the future)

drgorillamd commented 3 months ago

Opened #62 which would need 1 actually, to avoid being hack-bloated at some point