search

talaia-labs / rust-teos

The Eye of Satoshi - Lightning Watchtower
https://talaia.watch
MIT License
128 stars 62 forks source link

Replaces httpmock with mockito #194

Closed sr-gi closed 1 year ago

sr-gi commented 1 year ago

This PR replaces our current mock for the API's HTTP interface (httpmock) by mockito). The main reason for replacing one with the other is that the latter is more actively maintained and developed on, apart from it having some functionality that would be useful for us but the former lacked.

On top of that, httpmock was the root of some issues that we had worked around but were still carrying, such as #119 and #165.

It needs to be noted that, currently, we have replaced some of out time-based wait-and-check for the Retrier with some loops waiting for a condition to be met. This is due to mockito having some unexpected behavior for API delays when tests are run in parallel. Notice how this was also an issue with httpmock, even though we had the timings better nailed down (#193). We may be able to get rid of the loops once https://github.com/lipanski/mockito/issues/160 is fixed, even though it may be worth discussing whether wait-and-test is better than polling for a certain condition.

I've also spent some time redesigning the Retrier and the RetryManager to add some instrumentation points (such as adding a channel between the WTClient and the Retrier so the former can be notified when the state of the latter is changed). However, after several different iterations, I think it may not be worth the additional complexity for something that is merely used in testing (and can be checked with an ugly-but-simple loop)

sr-gi commented 1 year ago

I've added an additional commit improving the design given the devs from mockito added a new way of building mocks considering the request body when building the response (based on this issue I opened when I started working on the revamp: https://github.com/lipanski/mockito/issues/159).

Both commits can be squashed before merging, leaving them separate for now so it is easier to review. The delay issue seems to remain for now. If it happens to get fixed before we merge this I may add a last commit addressing it.

sr-gi commented 1 year ago

This should do. I've ended up going for the loop alternative after seeing that even if the delays are properly working (as for the .with_delay method rejected by mockito) test can sometimes fail. wait-based tests with hardcoded times suck, especially when the underlying function is an exponential backoff. I'd be nice to have with_body_from_fn accept async closures so tokio::time::sleep can be used instead, but I'm honestly not fluent enough with async to do the change (nor do I think it's worth putting the time atm given the usecase).

I've added a macro to do the polling-based wait so things are less verbose.

All commits can be squashed.

sr-gi commented 1 year ago

Addressed all comments but the ones related to https://github.com/talaia-labs/rust-teos/pull/194#discussion_r1129241073