[CBR-371] unit tests for listing addresses

[kderme]

Description

This PR adds tests for listing addresses:

• the results are always deterministic (in the sense that doing the same request 2 times yields the same response)
• paginating the results, yields no duplicate results.
• paginating the results may bring them in a different order (e.g. requesting 10 + 10 results in two pages will bring the same results but in different order than if we requested 20 in one page).
• The last point does not apply if there is a single account. In that case results are always in the same order.

UPDATE: a very small bug affected pagination when there are multiple accounts (see the diff in wallet-new/src/Cardano/Wallet/WalletLayer/Kernel/Addresses.hs). Now order is consistent always.

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CBR-371

Type of change

• 🐞 Bug fix (non-breaking change which fixes an issue)
• 🛠 New feature (non-breaking change which adds functionality)
• ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
• 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
• 🔨 New or improved tests for existing code
• ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

• I have read the style guide document, and my code follows the code style of this project.
• If my code deals with exceptions, it follows the guidelines.
• I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
• CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

• I have added tests to cover my changes.
• All new and existing tests passed.

QA Steps

Screenshots (if available)

[adinapoli-iohk]

Great job @kderme ! We are getting there, but I think we can do slightly better in the code design space. I have jotted down some ideas. Let me know what you think!

[adinapoli-iohk]

I would recommend flipping the logic and doing something like:

prepareAddressFixture acn adn = 
    -- We recompute the values to account for the fact that creating a new wallet
    -- @always@ come with a fresh account, and this account always have a fresh address
    -- associated with it.
    let requestedAccounts = max 0 (acn - 1)
    let requestedAddresses = max 0 (adn - 1)

Then, you can use those two let bindings all over the place without having to worry about +1/-1 arithmetic, both in the fixtures and in the tests. Furthermore, you segregate the comment on why we do this exactly in one point in the code, without this assumption being scattered all over the place.

[kderme]

I'm not sure I understand what are the semantics of requestedAddresses. Some accounts will have requestedAddresses addresses, while the first account will have requestedAddresses+1. Also requestedAccounts should never be 0, as one account is created by default.

[adinapoli-iohk]

I think if you bake the logic inside the fixture, you might be able to remove this.

[adinapoli-iohk]

Yes, unfortunately if you create multiple accounts for the same wallet those won't come with an extra associated address. But ideally, if you can bake this inside the fixture creation, test logic should hopefully simplify 😉

[kderme]

I should delete this comment.

[adinapoli-iohk]

It's a bit dense for the reader to try to figure out why you do the quot expectedTotal dance, but hopefully we can simplify this one.

[adinapoli-iohk]

Idea: considering is very error prone to go for this style of foo, foo', bar, bar', why don't you write something like this?

                        res <- runExceptT $ runHandler' $ do
                            r1 <- Handlers.listAddresses layer (RequestParams pp)
                            r2 <- Handlers.listAddresses layer (RequestParams pp)
                            pure (r1 === r2)
                        res1 <- runExceptT $ runHandler' $ do
                            r1 <- Handlers.listAddresses layer (RequestParams pp1)
                            r2 <- Handlers.listAddresses layer (RequestParams pp1)
                            pure (r1 === r2)
                       assert $ conjoin [res, res1]

Even better, I think you can simplify this even further by noticing how this is just iterating over [pp,pp1,pp2,pp3] but executing mechanical actions, so you can write something which runs a forM, calls internally replicateM 2 , yields a Property and conjoin everything at the end.

[adinapoli-iohk]

This is a minor thing, but these tests feels very imperative rather than declarative. I don't know what we can do about it, but I suspect we might come up with some combinators which factor away duplication and mechanical reshuffling of the data and leave the logic of the tests exposes. I guess what we aim is something like:

myTest = do
    -- Initialisation
    -- Execution
    -- Checking of properties

Which is exactly what your tests are also doing, but each of these steps takes a lot of lines of code and is hard to track down the spirit of the test 😉

[adinapoli-iohk]

We discussed this with @kderme , and we agreed he will open a separate ticket for the cleanup as requested by my original review.

[kderme]

cleanup ticket at https://iohk.myjetbrains.com/youtrack/issue/CBR-435