confio / tfi

Contracts for Regulated DeFi on the Tgrade blockchain
Apache License 2.0
9 stars 0 forks source link

dso-token tests refactoring and covering whole validation functionality #43

Closed hashedone closed 3 years ago

hashedone commented 3 years ago

Closes #15

Moved tests from contract to separate module, as those are not unit tests.

As first approach I tried to have sending messages in tests itself, but then there was more like 2k LOC in multitest (as every message is send at least twice, which makes a difference), and some tests (send and send_from in particular) were ridiculously long, which made them really exhausting to read (even update and making sure they are correct). Adding helper function which just sends messages (so are basically decorators) improved tests readability to very nice level, I think this is proper direction. To check what exactly is send by function it is always simple to jump to definition, and functions and their arguments are named in way which should not bring any confusion.

Also I decided not to test functionality which is just transparent calls to cw20 contracts, as those should be tested in cw20 itself (I obviously can check everything, including if Vec::push actually pushes anything to vector, but I don't find this helpful).

In terms of refactoring/upgrading code quality of #15 - I find this code fairly simple and readable and don't see urgent need of that. The only improvement I see could be add some abstractions over original cw20 contract, and external cw4 contract (I have even an idea how to approach this), it would be possible to inject mock of them, and as consequence allow testing most test on UT level, but I find this approach unnecessary complicating contract itself. Instead I have another idea how to allow unit testing contract without whole multitest (not instead, but maybe additionally) so internals could be easier/stronger covered (maybe topic for the future).