Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client: Improve testing and increase coverage #636

Merged
merged 16 commits into from
Nov 28, 2024
Merged

client: Improve testing and increase coverage #636

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
56b1f15
client: Increase coverage for `Dial` method
cthulhu-rider Nov 18, 2024
1a3a6e5
client: Do not lose some URI parsing failure reasons on dial
cthulhu-rider Nov 18, 2024
e3cc3b8
client: Place URI parsing function from neofs-api-go module internally
cthulhu-rider Nov 18, 2024
ce6872c
internal/util: Drop wrong statement from `ParseURI` func docs
cthulhu-rider Nov 18, 2024
92b9745
client: Do not try to dial an invalid server endpoint
cthulhu-rider Nov 18, 2024
ae384ed
client: Unify homogeneous internal call mechanics
cthulhu-rider Nov 19, 2024
d109f2d
client: Return error from unimplemented test methods
cthulhu-rider Nov 19, 2024
ee40390
client: Drop overriding of session and netmap calls via global vars
cthulhu-rider Nov 19, 2024
93b61f8
client: Unify the override of all operations
cthulhu-rider Nov 19, 2024
b75db12
client: Inline gRPC mechanics from neofs-api-go lib
cthulhu-rider Nov 20, 2024
e94cd49
client: Drop debug printline
cthulhu-rider Nov 21, 2024
0de224e
client: Increase test coverage of `BalanceGet` method
cthulhu-rider Nov 21, 2024
756cd3f
client: Clarify `Client.BalanceGet` unit test names
cthulhu-rider Nov 22, 2024
4269131
client: Increase `Client.BalanceGet` stat test coverage
cthulhu-rider Nov 22, 2024
614a5a7
client: Disable contextcheck linter for `Dial`
cthulhu-rider Nov 22, 2024
195a8f2
client: Fix duration in operation statistics
cthulhu-rider Nov 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 17 additions & 11 deletions client/accounting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,16 @@

import (
"context"
"time"

v2accounting "github.com/nspcc-dev/neofs-api-go/v2/accounting"
protoaccounting "github.com/nspcc-dev/neofs-api-go/v2/accounting/grpc"
"github.com/nspcc-dev/neofs-api-go/v2/refs"
rpcapi "github.com/nspcc-dev/neofs-api-go/v2/rpc"
"github.com/nspcc-dev/neofs-api-go/v2/rpc/client"
"github.com/nspcc-dev/neofs-sdk-go/accounting"
"github.com/nspcc-dev/neofs-sdk-go/stat"
"github.com/nspcc-dev/neofs-sdk-go/user"
)

var (
// special variable for test purposes, to overwrite real RPC calls.
rpcAPIBalance = rpcapi.Balance
roman-khimov marked this conversation as resolved.
Show resolved Hide resolved
)

// PrmBalanceGet groups parameters of BalanceGet operation.
type PrmBalanceGet struct {
prmCommonMeta
Expand All @@ -41,9 +36,12 @@
// - [ErrMissingAccount]
func (c *Client) BalanceGet(ctx context.Context, prm PrmBalanceGet) (accounting.Decimal, error) {
var err error
defer func() {
c.sendStatistic(stat.MethodBalanceGet, err)()
}()
if c.prm.statisticCallback != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely you can just defer c.sendStatistic(...)() and handle statisticCallback and startTime internally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if if is inevitable, then it is better to make a conditional defer imo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but why? you turned out the func that is commonly used and made more lines, no? i think it was planned as defer c.sendStatistic(...)() but smth went wrong, why make things harder?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not doing a defer is obviously more efficient, but for these (network) operations the difference is so negligible that I'd rather opt for simpler code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've done this not just for negligible optimization but for more clear code. Previously, i saw unconditional deferred stats, making me think that some stats are always tracked while sendStatistics encapsulates the logic that there can be no stats at all! Also, seeing defer func() { c.method()() }() blows my mind, i definitely cannot agree this is a simpler code

finally, if no one has any categorical objections, i'd leave the fixed version

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any categorical objections

mine is many repetitive lines for every method. also, you still use defer so why not, at least i would try to inline time calculation: time.Since(time.Now())

seeing defer func() { c.method()() }() blows my mind

agree but once you get it you just see in every method, it is our general approach

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at least i would try to inline time calculation: time.Since(time.Now())

hm, how would it be?

Copy link
Member

@carpawell carpawell Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i meant start time can be an arg of the deferred func, provided wrong example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

startTime := time.Now()
defer func() {
c.sendStatistic(stat.MethodBalanceGet, time.Since(startTime), err)
}()
}

switch {
case prm.account.IsZero():
Expand Down Expand Up @@ -74,7 +72,15 @@
cc.meta = prm.prmCommonMeta
cc.req = &req
cc.call = func() (responseV2, error) {
return rpcAPIBalance(&c.c, &req, client.WithContext(ctx))
resp, err := c.accounting.Balance(ctx, req.ToGRPCMessage().(*protoaccounting.BalanceRequest))
if err != nil {
return nil, rpcErr(err)
}
var respV2 v2accounting.BalanceResponse
if err = respV2.FromGRPCMessage(resp); err != nil {
return nil, err
}

Check warning on line 82 in client/accounting.go

View check run for this annotation

Codecov / codecov/patch

client/accounting.go#L81-L82 

Added lines #L81 - L82 were not covered by tests
return &respV2, nil
}
cc.result = func(r responseV2) {
resp := r.(*v2accounting.BalanceResponse)
Expand Down
Loading
Loading