morpho-org / morpho-utils

Repository gathering useful libraries and contracts.
GNU Affero General Public License v3.0
64 stars 1 forks source link

Reflexion about morpho-utils testing #44

Closed MathisGD closed 1 year ago

MathisGD commented 2 years ago

The following is a general comment, and something I've been thinking about for the past weeks. So don't take it too litterally.

Are the test conditions really useful ? I mean if we rewrite them everytime we change the function, can we really say that it helped us maintain a certain behavior ? It is making development more difficult though, because we have to rewrite this test. I'm leaning more and more towards test that are more focused and durable, leaving behind completeness, as tests cannot be complete anyway.

The general reasoning is that if we can write code more easily, then we can also fix the code more easily. Imagine a scenario for the same PR, but where you had to rewrite 100 tests like these (it would probably take more than a week of work in total). In this case, would you think it would be worth it to change this function at all ?

For this specific test, maybe we can restrict it to arbitrary large values like $10^{36}$

_Originally posted by @QGarchery in https://github.com/morpho-dao/morpho-utils/pull/43#discussion_r951393909_

QGarchery commented 2 years ago

Fair point ; I think it's a consequence of the fact that this test is a unit test whereas integration tests (those you describe) are much more generic I agree it's taking more time to change the implementation of a function tested with unit tests (because of the issue you raised) ; though I think you're forgetting that these very tests are useful during development because they can (not always) shed light on changes and induced edge cases and forces the developer to double-check with unit testing Most of the time, it's just double-checking Sometimes (at least for me), it showed me some edge-cases

_Originally posted by @Rubilmax in https://github.com/morpho-dao/morpho-utils/pull/43#discussion_r951443287_

QGarchery commented 2 years ago

I guess I don't like having too much unit-tests then :sweat_smile:

If we can have tests at a higher level, it is closer to the specification and should reprensent more accurately the expected behavior

MathisGD commented 2 years ago

Or automatic formal proving in CI ? 👀

QGarchery commented 2 years ago

In fact it's more when it is copying the code too much that it seems useless to me. To fix it, you can be tempted to copy-paste the code again in the test, and maybe it even leads to a wrong feeling of safety because the test now goes through

MathisGD commented 2 years ago

Yes, this wrong feeling of safety is the worse thing in the end IMO

Rubilmax commented 2 years ago

Then never feel safe with tests! The only counterpoint to unit tests is that it's harder to maintain. But having them is better than not having them: they are supposed to test each branch (but let's keep in mind that nothing guarantees they test all the branches => don't feel safe)

The ease they provide during development is, IMO, counter-balancing there biggest disadvantage

MerlinEgalite commented 1 year ago

Should we close @MathisGD ?

MathisGD commented 1 year ago

The issue has not been "solved", but it seems that we are fine with it so yes.