Increase test coverage of `accounting` and improve some test packages #590

cthulhu-rider · 2024-06-27T17:58:28Z

No description provided.

It teaches what a regular user should be freed from. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

smallhive · 2024-06-28T03:49:23Z

crypto/test/tests.go

-	require.NoError(tb, err)
+// Signature returns random neofscrypto.Signature.
+func Signature() neofscrypto.Signature {
+	sig := make([]byte, 64)


It was a question which I forget to ask many time. Do we want to move such magic numbers inside packages they are belongs to? I mean this 64 to neofscrypto.SignatureBytesLength for example.

Another example of magic numbers, this 33 would be perfect to has a public const, because in the code in projects we have a lot of pubKey = make([]byte, 33)
pubKey = make([]byte, ecdsa.PublicKeyLength) looks much better for me.

Yes, in some cases it could be inconvenient to export one more package, but in general case it would ok

code should not do pubKey = make([]byte, 33): https://pkg.go.dev/github.com/nspcc-dev/neofs-sdk-go@v1.0.0-rc.12/crypto#PublicKey has method to get len

actually i dont wanna make it const, forgot to add some randomness, thx anyway

accounting/decimal.go

accounting/decimal_test.go

audit/example_test.go

carpawell · 2024-06-28T18:28:26Z

audit/result.go

 		err = oID.ReadFromV2(oidV2)
 		if err != nil {
-			return fmt.Errorf("invalid passed storage group ID: %w", err)
+			return fmt.Errorf("invalid passed storage group ID #%d: %w", i, err)


we have an exact ID we could not read here, why not add it? same below

cuz it can be few MB len. We could make a cut for overlen values abc...def but code'd become more complex

container/id/test/id.go

roman-khimov

I should admit though that audit is less of a problem wrt API changes (nothing outside the node cares).

accounting/decimal.go

audit/example_test.go

roman-khimov · 2024-06-28T19:26:58Z

audit/result.go

+// SetCompleted sets data audit completion flag.
+//
+// See also [Result.Completed].
+func (r *Result) SetCompleted(completed bool) {


Do you have any use case for SetCompleted(false)? Why are we changing the API here?

no more relevant #590 (comment)

- split units tests for value and precision fields - add test for randomizing func Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

Data audit is completely closed in the NeoFS Inner Ring, so there is no point in having its implementation in the SDK. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

carpawell · 2024-07-03T16:31:29Z

@cthulhu-rider, linter died. What about tests btw?

This is very inconvenient. The only current failure cases are private key randomization or signing with it: panic is pretty normal for them. Overall, nobody expects that generating in-memory data could fail. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

It is often needed to get another ID, as well as a list of random ones. With new functions, `cidtest.IDWithChecksum` becomes redundant, so it marked as deprecated. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

Summary: * get rid of `Random` wording, it is implied everywhere; * provide single `Signer` function returning all potentially needed cryptographic components; * do not import `user` package into `neofscryptotest`, there is `usertest` for this; * provide test signer's wrapper which always failing signing. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

cthulhu-rider · 2024-07-03T19:21:56Z

@cthulhu-rider, linter died. What about tests btw?

fixed linter, AIO tests fail for some reason unrelated to the changes. I'll take a closer look, maybe create a separate issue

carpawell · 2024-07-04T16:07:23Z

AIO tests fail for some reason unrelated to the changes. I'll take a closer look, maybe create a separate issue

@cthulhu-rider, yes, have not found issues about it and do not know when they appeared first (to me this PR is the first one), so lets create an issue (if cannot be fixed fast) or fix it here (if possible).

ENV var containing listen endpoint of the REST GW differs b/w two tested versions. The easiest way is to set both. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

codecov · 2024-07-04T16:59:54Z

Codecov Report

Attention: Patch coverage is 73.01587% with 34 lines in your changes missing coverage. Please review.

Project coverage is 68.78%. Comparing base (11ca22b) to head (b069242).
Report is 5 commits behind head on master.

Files	Patch %	Lines
user/test/id.go	75.00%	9 Missing and 1 partial ⚠️
eacl/test/generate.go	0.00%	5 Missing ⚠️
object/id/test/generate.go	86.20%	2 Missing and 2 partials ⚠️
bearer/test/generate.go	0.00%	3 Missing ⚠️
crypto/test/tests.go	85.71%	2 Missing and 1 partial ⚠️
reputation/test/generate.go	0.00%	3 Missing ⚠️
container/id/test/id.go	87.50%	1 Missing and 1 partial ⚠️
container/test/generate.go	0.00%	2 Missing ⚠️
object/test/generate.go	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #590      +/-   ##
==========================================
+ Coverage   68.18%   68.78%   +0.60%     
==========================================
  Files         122      120       -2     
  Lines       10035    10043       +8     
==========================================
+ Hits         6842     6908      +66     
+ Misses       2814     2759      -55     
+ Partials      379      376       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

cthulhu-rider · 2024-07-04T17:42:26Z

AIO tests fail for some reason unrelated to the changes. I'll take a closer look, maybe create a separate issue

@cthulhu-rider, yes, have not found issues about it and do not know when they appeared first (to me this PR is the first one), so lets create an issue (if cannot be fixed fast) or fix it here (if possible).

added fix commit

accounting: Drop redundant example test

9683246

It teaches what a regular user should be freed from. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

cthulhu-rider requested review from roman-khimov, smallhive and carpawell as code owners June 27, 2024 17:58

smallhive reviewed Jun 28, 2024

View reviewed changes

cthulhu-rider force-pushed the accounting branch from 9f3af42 to 6650605 Compare June 28, 2024 11:04

carpawell reviewed Jun 28, 2024

View reviewed changes

roman-khimov reviewed Jun 28, 2024

View reviewed changes

cthulhu-rider mentioned this pull request Jul 2, 2024

audit: Add function to check if node passed the audit #241

Closed

cthulhu-rider force-pushed the accounting branch 2 times, most recently from a736fbd to 64ac78d Compare July 2, 2024 15:04

cthulhu-rider requested a review from carpawell July 2, 2024 15:07

cthulhu-rider added 2 commits July 2, 2024 19:08

accounting: Improve testing

faf6bd4

- split units tests for value and precision fields - add test for randomizing func Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

audit: Purge the package

4af79ee

Data audit is completely closed in the NeoFS Inner Ring, so there is no point in having its implementation in the SDK. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

cthulhu-rider force-pushed the accounting branch from 64ac78d to 16daa0d Compare July 2, 2024 15:09

cthulhu-rider requested a review from smallhive July 2, 2024 15:09

cthulhu-rider added 3 commits July 3, 2024 23:14

test: Add more helper functions for identifiers

377e1d1

It is often needed to get another ID, as well as a list of random ones. With new functions, `cidtest.IDWithChecksum` becomes redundant, so it marked as deprecated. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

cthulhu-rider force-pushed the accounting branch from 16daa0d to e0e7d1b Compare July 3, 2024 19:16

cthulhu-rider changed the title ~~Increase test coverage of accounting+audit and improve some test packages~~ Increase test coverage of accounting and improve some test packages Jul 4, 2024

tests: Fix REST gateway ENV variables in AIO tests

b069242

ENV var containing listen endpoint of the REST GW differs b/w two tested versions. The easiest way is to set both. Signed-off-by: Leonard Lyubich <leonard@morphbits.io>

cthulhu-rider force-pushed the accounting branch from c31ac65 to b069242 Compare July 4, 2024 16:52

cthulhu-rider requested a review from roman-khimov July 4, 2024 17:42

carpawell approved these changes Jul 5, 2024

View reviewed changes

carpawell merged commit 6d3ce31 into master Jul 5, 2024
12 checks passed

carpawell deleted the accounting branch July 5, 2024 10:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase test coverage of `accounting` and improve some test packages #590

Increase test coverage of `accounting` and improve some test packages #590

cthulhu-rider commented Jun 27, 2024

smallhive Jun 28, 2024

cthulhu-rider Jun 28, 2024

carpawell Jun 28, 2024

cthulhu-rider Jul 1, 2024

roman-khimov left a comment

roman-khimov Jun 28, 2024

cthulhu-rider Jul 2, 2024

carpawell commented Jul 3, 2024

cthulhu-rider commented Jul 3, 2024

carpawell commented Jul 4, 2024

codecov bot commented Jul 4, 2024

cthulhu-rider commented Jul 4, 2024

Increase test coverage of accounting and improve some test packages #590

Increase test coverage of accounting and improve some test packages #590

Conversation

cthulhu-rider commented Jun 27, 2024

smallhive Jun 28, 2024

Choose a reason for hiding this comment

cthulhu-rider Jun 28, 2024

Choose a reason for hiding this comment

carpawell Jun 28, 2024

Choose a reason for hiding this comment

cthulhu-rider Jul 1, 2024

Choose a reason for hiding this comment

roman-khimov left a comment

Choose a reason for hiding this comment

roman-khimov Jun 28, 2024

Choose a reason for hiding this comment

cthulhu-rider Jul 2, 2024

Choose a reason for hiding this comment

carpawell commented Jul 3, 2024

cthulhu-rider commented Jul 3, 2024

carpawell commented Jul 4, 2024

codecov bot commented Jul 4, 2024

Codecov Report

cthulhu-rider commented Jul 4, 2024

Increase test coverage of `accounting` and improve some test packages #590

Increase test coverage of `accounting` and improve some test packages #590