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

fix(sdk): report file upload failures as errors #7196

Merged
merged 25 commits into from
Apr 4, 2024

Conversation

moredatarequired
Copy link
Contributor

@moredatarequired moredatarequired commented Mar 21, 2024

Description

If the retryablehttp.Client completes a request but the status code isn't 200, report the status as an error rather than returning nil.

  • I updated CHANGELOG.md, or it's not applicable

Testing

Added 4 tests for:

  • 404 / File Not Found
  • No server response before timeout
  • Connection refused
  • Connection closed

Copy link

codecov bot commented Mar 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.23%. Comparing base (87b555a) to head (d341f8e).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7196      +/-   ##
==========================================
- Coverage   76.28%   76.23%   -0.05%     
==========================================
  Files         509      509              
  Lines       54174    54180       +6     
==========================================
- Hits        41325    41305      -20     
- Misses      12499    12527      +28     
+ Partials      350      348       -2     
Flag Coverage Δ
func 43.80% <25.00%> (+0.10%) ⬆️
system 64.56% <25.00%> (-0.10%) ⬇️
unit 57.91% <100.00%> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...ore/internal/filetransfer/file_transfer_default.go 65.30% <100.00%> (+2.26%) ⬆️
core/internal/filetransfer/tasks.go 50.00% <ø> (ø)

... and 12 files with indirect coverage changes

@moredatarequired moredatarequired changed the title Add a test that randomly fails uploads in various ways fix(sdk): report upload errors Mar 21, 2024
@moredatarequired moredatarequired changed the title fix(sdk): report upload errors fix(sdk): report file upload failures as errors Mar 21, 2024
@github-actions github-actions bot added cc-fix and removed cc-fix labels Mar 21, 2024
@github-actions github-actions bot added cc-fix and removed cc-fix labels Mar 21, 2024
@moredatarequired moredatarequired marked this pull request as ready for review March 21, 2024 23:01
@moredatarequired moredatarequired requested a review from a team as a code owner March 21, 2024 23:01
Copy link
Contributor

@timoffex timoffex left a comment

Choose a reason for hiding this comment

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

Generally LGTM! Just some comments about test duration; unit tests should avoid sleeping.

CHANGELOG.md Outdated Show resolved Hide resolved
core/internal/filetransfer/file_transfer_default.go Outdated Show resolved Hide resolved
core/internal/filetransfer/file_transfer_default_test.go Outdated Show resolved Hide resolved
core/internal/filetransfer/file_transfer_default_test.go Outdated Show resolved Hide resolved
core/internal/filetransfer/file_transfer_default_test.go Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@moredatarequired moredatarequired merged commit a1fd261 into main Apr 4, 2024
62 of 66 checks passed
@moredatarequired moredatarequired deleted the test-fault-uploads branch April 4, 2024 23:29
if err != nil {
return err
} else if resp == nil || resp.StatusCode < 200 || resp.StatusCode > 299 {
return fmt.Errorf("file transfer: upload: failed to upload: %v", resp.Status)
Copy link
Collaborator

Choose a reason for hiding this comment

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

wait if resp is nil you will have a panic here

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