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

Improve test coverage #7

Merged
merged 6 commits into from
Sep 13, 2024
Merged

Conversation

Aratz
Copy link
Contributor

@Aratz Aratz commented Sep 12, 2024

This PR improves the overall test coverage. While working on this, I've fixed the following issues:

  • /runfolders, /runfolder/next and /runfolder/pickup never have to return 404 Not Found. If there is no runfolder to return, they should simply return an empty list or 204 No Content.
  • Since we are checking for run_parameters when initializing a Runfolder, we can assume this attribute exists in all the other methods.
  • The "no metadata" warning message should be logged when none of the metadata fields is found.
  • Add tests for the cli parameters.

There is no reason why this handlers would return 404. If no runfolders
are found, it should just return an empty list. If another exception
pops up, it will be for a different reason and should be converted to an
Internal Error (500) (which aiohttp does automatically).

Same goes for `/runfolders/pickup` and `/runfolders/next`
This warning should be emitted when no metadata is found, this not only
occurs when run_parameters is empty, but also if it does not contain any
of the fields we look for.
This uncovered the following bug: if the instrument is not recognized,
we can't know its name.
@Aratz Aratz requested a review from nkongenelly September 12, 2024 13:23
@Aratz Aratz self-assigned this Sep 12, 2024
Copy link

codecov bot commented Sep 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (a83ca30) to head (3dfd4c6).
Report is 9 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main        #7      +/-   ##
===========================================
+ Coverage   92.59%   100.00%   +7.40%     
===========================================
  Files           8         8              
  Lines         243       230      -13     
===========================================
+ Hits          225       230       +5     
+ Misses         18         0      -18     

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

This runfolder has a run_parameter file, but no complete marker file
@Aratz
Copy link
Contributor Author

Aratz commented Sep 12, 2024

Actually, I know we've discussed that before here, but I think I've changed my mind. I think list_runfolders should ignore all AssertionErrors that come out when initializing a Runfolder and just ignore the associated path. Otherwise, it means that a faulty folder could block the whole service (e.g. if a folder contains a runparameter file but no complete marker). I've updated the tests and fixed the code to illustrate this issue.

Copy link
Contributor

@nkongenelly nkongenelly left a comment

Choose a reason for hiding this comment

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

These are great changes, I think all looks good.

@Aratz Aratz merged commit 31d4e12 into arteria-project:main Sep 13, 2024
3 checks passed
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.

2 participants