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

Implement Binary Upload (Fixes #2126) #2331

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

allentiak
Copy link
Member

No description provided.

@allentiak allentiak force-pushed the implement-binary-upload branch 4 times, most recently from deb292b to d8b837c Compare January 22, 2025 17:36
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.31%. Comparing base (faea419) to head (6fa4273).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2331      +/-   ##
==========================================
- Coverage   95.32%   95.31%   -0.02%     
==========================================
  Files         381      381              
  Lines       22823    22854      +31     
  Branches      517      518       +1     
==========================================
+ Hits        21756    21783      +27     
- Misses        550      553       +3     
- Partials      517      518       +1     

see 2 files with indirect coverage changes

@allentiak allentiak force-pushed the implement-binary-upload branch 12 times, most recently from d19d3d3 to b83d9e9 Compare January 30, 2025 11:28
@allentiak allentiak force-pushed the implement-binary-upload branch 10 times, most recently from 15c9a5c to 2293cb2 Compare January 30, 2025 22:00
@allentiak allentiak force-pushed the implement-binary-upload branch from b718330 to 78c8bf4 Compare February 3, 2025 18:23
@allentiak allentiak marked this pull request as ready for review February 3, 2025 18:37
@allentiak allentiak marked this pull request as draft February 3, 2025 18:49
Copy link
Member

@alexanderkiel alexanderkiel left a comment

Choose a reason for hiding this comment

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

I did not look at the unit tests, will do that after you worked on the current issues

@allentiak allentiak force-pushed the implement-binary-upload branch 4 times, most recently from 1ffb7bb to 1d299e5 Compare February 5, 2025 14:36
@allentiak allentiak marked this pull request as ready for review February 5, 2025 14:42
@allentiak allentiak force-pushed the implement-binary-upload branch from 1d299e5 to 80ae128 Compare February 5, 2025 14:42
Copy link
Member Author

@allentiak allentiak left a comment

Choose a reason for hiding this comment

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

Could you please review this again?
My only doubt is whether we should keep the debugging code in the integration test. I think it may be useful in the future...

@allentiak allentiak force-pushed the implement-binary-upload branch 4 times, most recently from 0006a11 to 6fa4273 Compare February 6, 2025 13:04
@allentiak allentiak marked this pull request as draft February 11, 2025 15:36
@allentiak allentiak force-pushed the implement-binary-upload branch 4 times, most recently from bf97a8a to 576049a Compare February 19, 2025 10:13
@allentiak allentiak marked this pull request as ready for review February 19, 2025 10:15
Extras:

* Overhaul the unit tests to ease debugging, bug finding, and making
sure we are fully following the spec.

* Fix: add missing status and content type for non-binary responses.
  (This bug was discovered by the new tests.)
@allentiak allentiak force-pushed the implement-binary-upload branch from 576049a to af16b87 Compare February 19, 2025 10:30
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.

None yet

2 participants