[sql-4] accounts: remove last usages of UpdateAccount
#941
Conversation
ellemouton
added
sql-ize
no-changelog
ellemouton
force-pushed
the
sql4Accounts4
branch
4 times, most recently
from
026cff8 to
3472b00
Compare
ellemouton
force-pushed
the
sql4Accounts4
branch
from
3472b00 to
de02304
Compare
accounts/store_test.go
Outdated
| @@ -96,8 +128,8 @@ func assertEqualAccounts(t *testing.T, expected, | |||
| actual.LastUpdate = time.Time{} | |||
|
|
|||
| require.Equal(t, expected, actual) | |||
| require.Equal(t, expectedExpiry.UnixNano(), actualExpiry.UnixNano()) | |||
| require.Equal(t, expectedUpdate.UnixNano(), actualUpdate.UnixNano()) | |||
| require.Equal(t, expectedExpiry.Unix(), actualExpiry.Unix()) | |||
There was a problem hiding this comment.
I think this one is not needed, the other one could be made stricter with the next PR where the clock is used
There was a problem hiding this comment.
not sure what you mean by "this one" and "the other one"
There was a problem hiding this comment.
reverted it for now. But i think we miiiight need to change it back to Unix() when we have the sqlite backend cause i dont think it does nanosecond precision. But will leave that for that PR 👍
There was a problem hiding this comment.
not sure what you mean by "this one" and "the other one"
for this, I referred to the marked line, because the expectedExpiry had the exact same timestamp, so Equal would have still worked, just the updated timestamp had a small diff
|
thanks @bitromortac ! gonna put it in "ready for review" since it is acked so that i can address all review comments in 1 go :) |
| // Update the balance and expiry. | ||
| err = store.UpdateAccountBalanceAndExpiry( | ||
| ctx, acct1.ID, fn.Some(int64(-500)), fn.Some(now), | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| // Add 2 payments. | ||
| _, err = store.UpsertAccountPayment( | ||
| ctx, acct1.ID, lntypes.Hash{12, 34, 56, 78}, 123456, | ||
| lnrpc.Payment_FAILED, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = store.UpsertAccountPayment( | ||
| ctx, acct1.ID, lntypes.Hash{34, 56, 78, 90}, 789456123789, | ||
| lnrpc.Payment_SUCCEEDED, | ||
| ) | ||
| require.NoError(t, err) | ||
|
|
||
| // Add 2 invoices. | ||
| err = store.AddAccountInvoice( | ||
| ctx, acct1.ID, lntypes.Hash{12, 34, 56, 78}, | ||
| ) | ||
| require.NoError(t, err) | ||
| err = store.AddAccountInvoice( | ||
| ctx, acct1.ID, lntypes.Hash{34, 56, 78, 90}, | ||
| ) | ||
| require.NoError(t, err) |
There was a problem hiding this comment.
nit: would personally put these after updating the in-memory acct1, as that makes more logical sense, and would also then change it to something like:
for paymentHash, entry := range acct1.Payments {
_, err = store.UpsertAccountPayment(
ctx, acct1.ID, paymentHash, entry.FullAmount,
entry.Status,
)
require.NoError(t, err)
}
for paymentHash := range acct1.Invoices {
err = store.AddAccountInvoice(ctx, acct1.ID, paymentHash)
require.NoError(t, err)
}
To not have to repeat hashes and amounts.
There was a problem hiding this comment.
To not have to repeat hashes and amounts.
I kind of like keeping tests very explicit. Else a bug in one version can be transferred to the other version.
For example, if you are testing an func Add(x, y int) int function, then one should test explicitly like:
x := 1
y := 2
expected := 3
result := Add(x, y)
require.Equal(3, result)
instead of:
x := 1
y := 2
expected := x+y
result := Add(x, y)
require.Equal(expected, result)
In our mission to replace UpdateAccount, we need UpdatAccountBalanceAndExpiry to take a negative balance so that we can use it to replace the behaviour of UpdateAccount as it stands today.
ellemouton
force-pushed
the
sql4Accounts4
branch
2 times, most recently
from
cf15ebd to
9e45401
Compare
Remove the call to UpdateAccount from the TestAccountStore test and instead replace it with all the other calls we have added.
We remove the last few calls to UpdateAccount from the TestAccountService test. Previously, the UpdateAccount call was used in this test to also _insert_ new accounts and to force the account ID to be set to specific values. So here we need to instead call the store's NewAccount method to insert a new account, then we get the AccountID from the returned value and we need to then use this returned ID (which is no longer forceable) in the remainder of the tests.
In the case where no startup error is expected, we should assert that no error is returned from Start.
ellemouton
force-pushed
the
sql4Accounts4
branch
from
9e45401 to
ef78bc4
Compare
With this PR, we complete the
Prepare the accounts Store interface to be ready for a SQL impl&Prep unit tests to be ready to pass against a different backendsteps in #917 .We do 2 things here:
we revert one of the changes from a previous PR where we change the balance type of
UpdateAccountBalanceAndExpiryfrom an int64 to a more strict lnwire.Millisatoshi type. We revert that here though so that we can use theUpdateAccountBalanceAndExpiryin a unit test that usesUpdateAccountto set the balance to a negative value (which currently is allowed).The rest of the PR removes the last couple of usages of
UpdateAccountfrom tests.