Modifiers are confusing if staying empty.

[novaknole]

Assume that I want to have the following .tree

WhenTest
└── given blax1 and blax2 are not met
    ├── when blax3 is not met
    │   └── it shouldn't be advanceable
    └── given blax3 is met
        ├── when blax4 is met
        │   └── it shouldn't be advanceable
        └── when blax4 is not met
            └── it should be advanceable

This provides a great structure. Though, after using bulloak, this puts modifiers on my test functions. The problem is I can't actually do anything inside the modifier, because at that time(inside modifier), I don't have the contract and I can only access it inside my test function. So Not sure what to do. Modifiers actually help with readability of the test function description which is great, but If I can't do anything inside modifier, then whatever message modifier has, I have to do it inside the test, which is misleading for sure. Hope I am making sense.

I tried not emitting modifiers(by -m flag), I also tried removing all given keywords inside .tree, but it still puts modifiers on the functions. Even if there's a way not to put the modifier on the function, then test function loses the description.

[alexfertel]

I don't have the contract and I can only access it inside my test function

This is a bit surprising to me, I would expect this to be the case. You can see examples of modifiers here. I would have to delegate on test organization to @PaulRBerg or @smol-ninja.

I tried not emitting modifiers(by -m flag), I also tried removing all given keywords inside .tree, but it still puts modifiers on the functions.

So you'd like a flag where we don't generate modifiers in test signatures as well? Would that resolve your use case?

Even if there's a way not to put the modifier on the function, then test function loses the description.

What do you mean by "test function loses the description"?

[novaknole]

hey @alexfertel Thanks for the quick answer.

Basically, in my opinion, the whole idea of .tree file and modifiers is to give your tests better descriptions. Without this, I end up with test names such as: test_RevertwhenApprovalandMinAdvanceAreMetAndDurationIsNotPassed which is a nonsense name. In the javascript, it's easier because you can have describe blocks and put the descriptions there and then create another describe inside the parent describe and add more descriptions. With bulloak, this is done with .tree file which is great. Though, here is the actual problem:

It generated the following inside the contract:

function test_whenMinAdvanceIsPassed() external givenApprovalIsPassed {

}

This is good, because modifier now adds more descriptions to the test, so I don't have to have such a huge name as above stated for the test. But the actual problem is that that modifier can't have any code - why ? it's because modifier is called at first, but it doesn't have the records since the records are created in the test itself, so modifier description loses its advantage at all - what's the point of having empty modifiers if it doesn't do what it's called - in this case, it's called givenApprovalIsPassed, but at that time, I can't make sure that approvalIsPassed and I can't write any code inside that modifier to make approval passed since records are created when the test runs.

You might say to execute modifier after the test is executed, but that's problem as well because this test's responsibility is to check things only when approval is passed.

So you'd like a flag where we don't generate modifiers in test signatures as well? Would that resolve your use case?

That's better, but doesn't solve the case, because my tests would still have huge names otherwise where would you put the description ? as a comment inside the test ? not fully ideal. I liked the modifiers option but as I said, I don't see the advantage of using bulloak in my scenarios unless I am doing something wrong.

[alexfertel]

I think I understand what you're saying, but I think it has more to do with test organization and BTT than with bulloak.

in this case, it's called givenApprovalIsPassed, but at that time, I can't make sure that approvalIsPassed and I can't write any code inside that modifier to make approval passed since records are created when the test runs.

I don't think I understand what you mean here. You should be able to do whatever you want inside a modifier. In particular, you should be able to approve whatever you need to approve. If it's too much in your opinion or too complex, maybe that shows that it would be better to use a separate setUp in a different test contract.

That's better, but doesn't solve the case, because my tests would still have huge names otherwise where would you put the description ? as a comment inside the test ? not fully ideal. I liked the modifiers option but as I said, I don't see the advantage of using bulloak in my scenarios unless I am doing something wrong.

I mean, thank you for sharing your thoughts, but I can't do much if you don't like BTT, hehe. You're free to use whatever practice feels better to you!

If you have meaningful suggestions to improve bulloak itself lmk, otherwise, I think we should close this issue or move it to a GitHub Discussion, since this is not really a bug or a feature.

[novaknole]

@alexfertel

Definitely not the issue with bulloak. I was curious about the best practice. Just so you fully understand, here is the idea.

1. In the setUp, contract is deployed.
2. in each test, I create a proposal. contract.createProposal. Until the proposal is created, I can NOT really make sure that approval is passed.

Since modifier is executed first before the test code, inside modifier, sure, I have contract, but proposal is not created yet and it's a really bad thing if I have to do proposal creation inside modifier. Since the modifier is named givenApprovalIsPassed, that means that in the modifier, only thing that should happen is to pass the approvals which I can't.

Do you have any recommendation/best practices of what people name their test functions that are super huge and depends on multiple things ? as in, what would you change test_RevertwhenApprovalandMinAdvanceAreMetAndDurationIsNotPassed to ?

[alexfertel]

Sure, what if you have a givenAProposalIsCreated in which you create the proposal, and then a second, child modifier called givenAnApprovedProposal, in which you approve it. Every test that's a child of the second modifier will have access to both a proposal and ensure stuff is approved, while keeping the name of the test short.

[alexfertel]

Tree looks like this:

ProposalsAreCool
└── given a proposal is created
    ├── when the proposal gets approved
    │   └── it should do this thing
    └── when the proposal is not approved
        └── when calling a function
            └── it should revert

[novaknole]

Thanks @alexfertel so much. Appreciate it so much. This seems like the better option.

One wonder I have is I often have scenarios where 2 tests should have the same name, but their conditions have to be different. i.e assume that modifiers are different but test names are the same - you will also have this situation if you have lots of branching. In such case, the only solution is to create multiple .tree files, otherwise bulloak fails because it complains about multiple tests having the same name and it doesn't generate the sol file at all. I can't do contractName::functionName as you suggest in the documentation, because I am really not testing functions but scenarios.

After splitting as much as I could in multiple .tree files, I end up with the following in one of the tree file:

Blax1
└── given proposal is created
    └── given minAdvance is met and stageDuration is not
        ├── given approvalThreshold is met
        │   ├── when voteThreshold is not met
        │   │   └── it should be advanceable
        │   └── when voteThreshold is met
        │       └── it shouldn't be advanceable
        └── given approvalThreshold is not met
            ├── when voteThreshold is not met
            │   └── it shouldn't be advanceable
            └── when voteThreshold is met
                └── it shouldn't be advanceable

This again fails with bulloak since there're duplicate names which for sure is understandable why bulloak complains. What I am wondering is whether I am making an error in my logic/brain.

How would you write the above so that it still ends up correct ? Also note that I don't like to have when voteThreshold is met, because this should also be a condition as I can use that condition(modifier) for other test functions as well.

I tried to also put given instead of when for voteThreshold is met, but problem is after given, comes it. So it really doesn't make a modifier and puts given in the test name. I think what my actual problems are:

1. I really can't come up with the unique test names for the when keyword.
2. If I change when to given, it then doesn't create a modifier, but puts given in the test name because it comes right away after given.

I was wondering if I am failing to understand something simple and still end up in this horrible mess.

[alexfertel]

Thanks @alexfertel so much. Appreciate it so much. This seems like the better option.

You're welcome!

If I change when to given, it then doesn't create a modifier, but puts given in the test name because it comes right away after given.

This is not true, given and when are equivalent, so using either keyword has no effect in modifier generation.

I really can't come up with the unique test names for the when keyword.

This is a design trade-off I decided to have, because modifier names have to be unique for the Solidity file to compile. An easy fix is just to append a 1 to the condition or change the wording bit. Otherwise you'd have to restructure your tree, which in your example is straightforward, since in the final branch, the voteThreshold doesn't matter:

Blax1
└── given proposal is created
    └── given minAdvance is met and stageDuration is not
        ├── given approvalThreshold is met
        │   ├── when voteThreshold is not met
        │   │   └── it should be advanceable
        │   └── when voteThreshold is met
        │       └── it shouldn't be advanceable
        └── given approvalThreshold is not met
            └── it shouldn't be advanceable

I don't have a good strategy here, and it should be thought about on a case-by-case basis.

---

I'd rather not continue these discussions about design practices and test organization unless you've found a bug or there's a feature you'd like to request, cause bulloak is more about reducing boilerplate and does not have much opinion on these matters. Please, prefer GitHub Discussions for this 🙏 so that the community may help as well! As such, I'm closing this issue. Thanks!