Should there be more unit tests and fewer integration tests?

[mds1]

The test architecture in this repo is very clean and well thought out. As I was reviewing it to extract some ideas and best practices, one thing that confused me is that the test/unit directory has very few unit tests.

• It has some tests for Adminable, Comptroller, and NFTDescriptor tests
• It has no tests for everything else in src/, and there's a lot there! The dynamic and linear lockup contracts, various abstract contracts, and libraries all have no unit tests.

I was expecting the unit test folder to have at least concrete/ unit tests and .tree files for everything in src/. Instead, it seems a lot of these instead are in the integration folder, but they still look like unit tests to me.

The line between unit and integration tests can certainly be blurry, and I'd be interested to here the definitions you used here / how you decided what was a unit vs. integrations. But it still feels like there is a lack of unit tests based on the current architecture, which threw me off a bit. Per @PaulRBerg's suggestion, I'm starting this discussion to better understand the rationale here and hopefully come away with some broader takeaways other projects can use.

[PaulRBerg]

Thanks for your feedback.

The reason why Lockup Linear, Lockup Dynamic, and the abstract contracts have no unit tests is that all functions contained in them require interaction with other contracts. Even the constructors contain a reference to the NFT descriptor.

It would be helpful to clarify how we define "integration": it's any test that involves at least one other contract. It doesn't matter that the other contract is a measly ERC-20 token - it's still an external contract, and so it leads to an integration of multiple contracts.

The advantage of this definition is that it is deterministic - we never quibble about whether a test constitutes is unit or integration (bar for when we make mistakes, as per the next paragraph).

With that said, there are cases where we didn't apply this rule consistently. For instance, these integration tests should absolutely be converted to unit:

• flashFee
• setFlashFee

This is because they don't involve an interaction with any other contract.

[PaulRBerg]

FWIW, we ended up turning the flashFee and the setFlashFee tests to unit tests, as discussed above.

#626