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.

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

Test coverage for Client ops #641

Merged
merged 9 commits into from
Dec 12, 2024
Merged

Test coverage for Client ops #641

merged 9 commits into from
Dec 12, 2024

Conversation

cthulhu-rider
Copy link
Contributor

@cthulhu-rider cthulhu-rider commented Nov 27, 2024

Copy link

codecov bot commented Nov 27, 2024

Codecov Report

Attention: Patch coverage is 82.41758% with 16 lines in your changes missing coverage. Please review.

Project coverage is 60.92%. Comparing base (591dd25) to head (09b291d).
Report is 28 commits behind head on master.

Files with missing lines Patch % Lines
client/object_get.go 76.47% 12 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #641      +/-   ##
==========================================
+ Coverage   58.77%   60.92%   +2.15%     
==========================================
  Files         165      165              
  Lines       22751    22817      +66     
==========================================
+ Hits        13372    13902     +530     
+ Misses       8940     8548     -392     
+ Partials      439      367      -72     

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

This:
 - groups with wrong length case;
 - mentions the correct version.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
Copy link
Contributor Author

that's it for now 😅

test code may look bulky, but now it's quite detective. The maintenance is expected to be easier in the future

testing allowed to revise many things. I've changed/fixed some of them right away. For less obvious and/or requiring more changes I opened several issues

@cthulhu-rider cthulhu-rider marked this pull request as ready for review December 10, 2024 17:32
Copy link
Member

@roman-khimov roman-khimov left a comment

Choose a reason for hiding this comment

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

I can't review test changes here, sorry. Functional ones are OK.

client/object_hash.go Show resolved Hide resolved
container/container.go Outdated Show resolved Hide resolved
Regression from b75db12. Since then, no
error were submitted to stat handler of following `Client` ops:
 - `NetmapSnapshot`;
 - `ObjectDelete`;
 - `ObjectHash`.

Now unit tests detect such things.

Signed-off-by: Leonard Lyubich <[email protected]>
Previously, client did not check split info from server responses to
`Head` / `Get` and `GetRange` requests. In addition to accepting a buggy
or malicious response, this also resulted in undefined data in the
method returns.

Now split info field is strictly controlled according to the protocol.

Signed-off-by: Leonard Lyubich <[email protected]>
Previously, `ObjectGetInit` and `ObjectHead` methods of the `Client` did
not check whether required response fields (object header, signature
and, for GET, ID) are set or not. This was incorrect since these fields
are required, missing any of them makes response inappropriate.

Now `Client` returns `MissingResponseFieldErr` in this case.
Corresponding unit tests PASS now.

Signed-off-by: Leonard Lyubich <[email protected]>
Previous 'empty attribute value K' text could confuse because K is not
empty.

For network info, 'attribute' is also renamed to 'parameter'.

Signed-off-by: Leonard Lyubich <[email protected]>
`Client` must not attempt to send obviously invalid requests. Moreover,
pre-check already existed for PUT, and these operations are similar in
this sense.

Signed-off-by: Leonard Lyubich <[email protected]>
`Client` signs not just payload but request parts. So, to be more
precise what's going on, it's worth to narrow generic 'calculate
signature' text. Moreover, PUT already results with 'calculate container
signature' in the same case.

Signed-off-by: Leonard Lyubich <[email protected]>
Previously, on the problem with the 1st GET stream message, `Client`
returned 'header: ...' error. Such an error led to the question: header
what?

Now 'read header: ...' is returned.

Signed-off-by: Leonard Lyubich <[email protected]>
@cthulhu-rider
Copy link
Contributor Author

I can't review test changes here, sorry

np, can be done at any time

Copy link
Member

@carpawell carpawell left a comment

Choose a reason for hiding this comment

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

Test commit was not viewed carefully.

@cthulhu-rider cthulhu-rider merged commit 0b9bb74 into master Dec 12, 2024
10 checks passed
@cthulhu-rider cthulhu-rider deleted the client-fin-tests branch December 12, 2024 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants