-
Notifications
You must be signed in to change notification settings - Fork 9
sdk/agent: add a first happy path test #255
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is awesome, lgtm 🙌 just had 1 non blocking 💭
took me a minute to understand what the func types were doing, TIL functions can satisfy interfaces
"github.com/stretchr/testify/require" | ||
) | ||
|
||
type sequenceNumberCollector func(accountID *keypair.FromAddress) (int64, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ what's the reason for re-creating these functions instead of using the exported ones?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm defining a type, which can be a function, that satisfies each of the interfaces so that in tests I can write quick little functions inline for them. This is a common pattern most known for its use in the stdlib's http package, namely the http.HandleFunc
type that is a function that implements the http.Handle
interface.
There are a couple proposals being debated at the moment to make it possible in Go to convert a function to a single-function interface: golang/go#21670, golang/go#$47487. If either proposal ever gets accepted it'll make it unnecessary to define these types in tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops I asked that and then figured it out and thought I deleted the question. Thanks for the additional info though makes sense!
SequenceNumberCollector: sequenceNumberCollector(func(accountID *keypair.FromAddress) (int64, error) { | ||
return 1, nil | ||
}), | ||
BalanceCollector: balanceCollectorFunc(func(accountID *keypair.FromAddress, asset state.Asset) (int64, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 thoughts on returning the passed in int64
? Or possibly another source instead of hardcoding 🤔 may come in handy on future tests where the balance will change and we want test that
also could be a change in the PR that requires that, so maybe not this one more I think about it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can implement that in whatever test needs it. The way I've implement that is to set a new BalanceCollector
at each point I want a new value.
I'd do that either by setting a new function that returns a new hardcoded value, or by changing the balanceCollectorFunc
to a fixedBalanceCollector
that is an int64 that returns itself. Either way works.
What
Add a first happy path test that exercises two agents that are connected to each other and successfully open, pay, and close.
Why
The agent code was ported over from the console example. The console example was really written to be throw away code, but the agent code within it was well formed enough to be something to build on. It's important we add tests early for the agent before we build too much more into it. This is a first step to closing #225.