sivasathyaseeelan
commented
2 weeks ago I don't really see a point in these tests. These functions are called so many times in every test that something was wrong there, it would be immediately obvious.
I like unit tests that test something complex + fragile + important in isolation. Unit-tests are great, when possible to use it. But asserting that duration since epoch is positive ... is just not far from
assert_eq!(1 + 1, 2);. Sure it better be true, but is there any chance it will ever actually catch this not being the case?Unless there are some existing issues about missing tests, I think your time (and time of anyone willing to improve our testing posture), would be to go through higher level tests and look for some glaring gaps.
E.g. our passphrase based backup & recovery was very basic IIRC.
Maybe check the past issues and PRs for bugfixes and see if you were to revert the fix, would any test catch the problem and if not - add the missing test.
Another way - try introducing random mistake in some code (add +1, turn
<into>or<=, etc.) and check if any tests catches it and if not think about why is it and what kind of test would do it.
Thank you @dpc for your detailed feedback on the unit tests. I understand your point about the current tests not being as effective in catching potential issues, especially considering their simplicity.
I'll take your advice and shift my focus to higher-level tests, as you suggested. I'll look into areas like passphrase-based backup and recovery, as well as review past issues and PRs to identify any gaps that need addressing.
dpc
Fix
This PR adds unit test fin fedimint/fedimint-core/src/time.rs and closes #5119