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

🐛 bugfix: file download #5673

Conversation

bisgaard-itis
Copy link
Contributor

@bisgaard-itis bisgaard-itis commented Apr 15, 2024

What do these changes do?

  • The file download is broken in the python osparc client.
  • This fixes the openapi.json.

Related issue/s

How to test

  • manually generate osparc client and run tests. These changes to the client will be done after this PR is merged.

Dev-ops checklist

@bisgaard-itis bisgaard-itis requested a review from pcrespov as a code owner April 15, 2024 11:50
@bisgaard-itis bisgaard-itis requested a review from wvangeit April 15, 2024 11:50
@bisgaard-itis bisgaard-itis self-assigned this Apr 15, 2024
@bisgaard-itis bisgaard-itis added the bug buggy, it does not work as expected label Apr 15, 2024
@bisgaard-itis bisgaard-itis added this to the Enchanted Odyssey milestone Apr 15, 2024
Copy link

codecov bot commented Apr 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.9%. Comparing base (cafbf96) to head (a65a53f).
Report is 114 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5673      +/-   ##
=========================================
- Coverage    84.5%   66.9%   -17.6%     
=========================================
  Files          10     630     +620     
  Lines         214   30856   +30642     
  Branches       25     204     +179     
=========================================
+ Hits          181   20670   +20489     
- Misses         23   10134   +10111     
- Partials       10      52      +42     
Flag Coverage Δ
integrationtests 65.0% <ø> (?)
unittests 84.9% <ø> (+0.3%) ⬆️

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

Files Coverage Δ
...src/simcore_service_api_server/api/routes/files.py 68.6% <ø> (ø)

... and 635 files with indirect coverage changes

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

Can you add a test that guarantees that this change actually works. Or if that exists, can you please point which test covers this code?

Copy link
Contributor

@wvangeit wvangeit left a comment

Choose a reason for hiding this comment

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

Thx. Looks good.

@bisgaard-itis
Copy link
Contributor Author

Can you add a test that guarantees that this change actually works. Or if that exists, can you please point which test covers this code?

I added a test which ensures that media type of the content is application/octet-stream when the download file endpoint returns status 200. This of course doesn't guarantee that the client call works, but there are e2e tests which checks this (which is how I discovered the bug).

@bisgaard-itis bisgaard-itis enabled auto-merge (squash) April 16, 2024 07:49
Copy link

sonarcloud bot commented Apr 16, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@bisgaard-itis bisgaard-itis merged commit f4a365c into ITISFoundation:master Apr 16, 2024
56 checks passed
@bisgaard-itis bisgaard-itis deleted the fix-file-download-in-python-osparc-client branch April 16, 2024 08:28
pcrespov pushed a commit to pcrespov/osparc-simcore that referenced this pull request Apr 16, 2024
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Apr 26, 2024
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug buggy, it does not work as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants