Singletone test extras and review

[andreivladbrg]

As we discussed on Slack, this PR runs some of the integration tests only on dynamic, as it was redundant to run them for all distribution modes. We now have this:

Category	Functions
Functions that are run on each distribution model	createWithTimestamps, createWithDurations, withdraw, cancel, renounce, streamedAmountOf, refundableAmountOf, withdrawableAmountOf
Functions that are run only on default Dynamic	burn, "multiple" functions, getters, statusOf, transferFrom, withdrawHooks, withdrawMax, withdrawMaxAndTransfer

Also, this PR addresses some of my feedback left on #1077

Changes

• test: don't run tests on all distribution model
• test: test only cancel withdraw renounce and calculation functions for all distribution models
• test: add getters common test file and run them only for dynamic
• test: add and use expectRevert function in Integration_Test
• test: remove shared dir from integration
• test: remove "multiple" fuzz tests
• test: remove getWithdrawnAmount fuzz tests
• test: rename create dirs to contain the model
• test: add revert test for unique getters
• test: dry fy create with timestamps test with a common file in lockup-base
• test: dry fy tests by adding lockupModel variable in Integration_Test
• test: use only createDefaultStream function where possible
• test: other dry fy and code improvements

[smol-ninja]

Nice work on the PR. Surprised to see such a massive review PR xD

All other functions that are not mentioned in "Functions that are run on each distribution model" are included in "Functions that are run only on default Dynamic", right? Because in the description I see some non-getter function names are missing: setNFTDescriptor, allowToHook etc.

[andreivladbrg]

setNFTDescriptor, allowToHook

these functions are not related to "streams" (you don't pass a streamId), they are independent logic for the admin - there is no need to include them in a list related to the distribution model, they should have been removed from start in initial PR

---

one thing i didn't mention in the OP, i have placed the common function tests for createWithTimestamps although there is not create function in base contract, the problem was that didn't know where it would have been a good place for it, lmk if you have a better idea

[smol-ninja]

I have done partial review and I will do a review again once we have discussed the below comments. Some notes before reviewing the comments below:

On symmetry:

I have requested many changes in favour of symmetry across LL, LD and LT tests. The tests are written keeping in mind that default model used in Base is Dynamic. Lets say if tomorrow, we decide to make Linear as the default model in Base, this would mean a lot of refactor. We can avoid that if we make all tests look symmetric and independent of the knowledge of the underlying default model.

On splitting trees across abstract tests

This is a design choice. I believe trees should always be complete in themselves regardless of whether the test contract has a shared child contract or not. And specially with create tests, I am not in favour of having abstract contract as they are important and imo should have their own complete tests even if its against DRY'ify.

also while reviewing I made some changes which I am keeping in #1085, which if you agree, can marge in this PR.

[smol-ninja]

As mentioned above, I am not in favour of having split trees. I'd rather like to see a full tree here for completeness. But since its a design choice and both are technically correct, we can debate later and continue like this at the moment.

[andreivladbrg]

don't agree, it is not scalable

[smol-ninja]

I don't understand how is it not scalable. There are 6 functions, we still have 6 test files. Each test represent one create function. If we add more create functions, we would still have to add more create tests. So I am not sure how having abstract make it scalable whereas abstract only contains very basic tests.

[andreivladbrg]

abstract only contains very basic tests

bingo, why duplicate it then?

the "scalable" part was mainly due to this explanation: #1083 (comment)

[smol-ninja]

I thought about it more last night. I have come to realize that I am against this mainly because of BTT. If it were not for BTT, I would have supported this refactor.

To be compatible with BTT, this is how tests should be written:

1. Write a test tree first.
2. Generate test contract using bulloak scaffold
3. Remove definitions of modifiers as they are defined in parent contracts but not from the tests
4. Update logic inside the tests

In your child contract of create-with-timestamps, if you use bulloak scaffold it wont be able to add all those modifiers since they do not exist in the tree anymore. bulloak check is not failing because it does not flag additional modifiers. Therefore, creating a test contract from those trees has is no longer automated now because you would have to add modifier by yourself manually. So if you miss a modifier or two, bulloak check wont detect that because they are no longer a part of the tree structure.

I hope I have made myself clear why I am againt this.

[andreivladbrg]

You did and I get this, but as said before, IMO BTT should have inheritance and abstraction concepts.

if BTT technique currently lacks these concepts, it doesn’t mean we shouldn’t use them, as it clearly improves the tests

[smol-ninja]

I finished my review. Two more comments below. I have skipped leaving comments in some of the files in LD and LT as they will just be duplicate of the comments made above.

[smol-ninja]

The comments that we have agreed upon, I have marked them resolved and included those changes through #1085. The comments that are not marked as resolved, I have not made any changes according to them.

[smol-ninja]

Shall I merge it into #1077 @andreivladbrg ? We can do merge request so all commits will be added.