Fuzz tests make gas report and gas snapshot non-deterministic

[3esmit]

The necessity or how fuzz tests are made should be discussed, as its making the gas report and gas snapshot change for little amounts in every run.

Ideally these files should be deterministic, and should not change from one run to another.

One option would be to force the fuzz test to run a specific amount of times, not randomly choose a number of calls.

This line https://github.com/logos-co/staking/blob/develop/test/StakeManager.t.sol#L665C77-L665C87

gives a random number of accounts to be tested, which makes the number of calls change drastically.

I believe this is the main reason causing this issue.

I would completely remove this fuzz test, as it does not seem to be generating any improvement to the coverage.

[0x-r4bbit]

Nice find!
I'd rather prefer we keep the fuzz test. In practice, there'll be more scenarios where fuzz tests make sense. We can't stop having fuzz tests, just because the reports are changed.

I think the way forward to be to have the fuzz tests simply be excluded from the reporting. Probably via pattern matching in the gas-report command. To make it easier, we can move our fuzz tests into a dedicated fuzz folder.
What do you think?

[3esmit]

I agree that fuzz tests are good. However, the way this fuzz test we have implemented is done is not helping. I mean, what difference does it make if its creating 2, 10, or whatever amount of accounts? Its doing all the same process over and over many times. The current fuzz test we have is only slowing down the test command and generating reports that are not deterministic.

If it was at least improving the coverage, I would be fine with it.

The only problem its messing with the report is not because the results it test are different, but because the amount of calls change.

What it does in practice is repeat the same thing over and over again, but with different amount of accounts. We could replace the same coverage by having a test with 1 account, another with 5, another with lets say 50 accounts.

We can still have fuzz tests, I can work on that, but using random deposit values, and random gaps in block.timestamp warps before calling executeAccount - that would be indeed useful.

[0x-r4bbit]

Okay fair enough. If you feel like removing it makes more sense, then go for it.

[3esmit]

I`ll fix that test btw, its basically not testing what it should because of this line https://github.com/logos-co/staking/blob/develop/test/StakeManager.t.sol#L674

And I'll implement fuzz tests that make sense. Ill make the changes and do a single PR that improve all this.

[3esmit]

Btw, if that line was not wrong, instead of 1 it being like the actual amount of iteractions to reach cap, and we had fuzz tests at 10000 as it was configured before, this would take weeks to finish, if not months.