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

Tests FHIRResource and its extensions #28

Merged
merged 9 commits into from
Jan 28, 2025
Merged

Conversation

jdisho
Copy link
Member

@jdisho jdisho commented Jan 3, 2025

Write tests for FHIRResource and its extensions.

♻️ Current situation & Problem

The FHIRResource class and its extensions currently have 0% test coverage. We should increase the code coverage as much as possible, to ensure long-term maintainability.

⚙️ Release Notes

  • This PR doesn't change any features. It adds tests for the FHIRResource, FHIRResource+Search, and ResourceProxy+DisplayName.
  • For the FHIRResource+Category tests, it's best to wait until PR FHIRStore actor isolation #26 is merged into the main branch, avoiding any potential conflicts or inconsistencies.

📚 Documentation

Comments are added throughout the code.

✅ Testing

Aim for >90% code coverage, but >80% is acceptable in this case.

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

@jdisho jdisho added the enhancement New feature or request label Jan 3, 2025
@jdisho jdisho mentioned this pull request Jan 3, 2025
4 tasks
@jdisho jdisho force-pushed the test-fhirresource branch 2 times, most recently from b71333d to d607628 Compare January 3, 2025 15:49
@jdisho jdisho force-pushed the test-fhirresource branch from d607628 to 2fa405c Compare January 3, 2025 15:51
@jdisho jdisho changed the title Write tests for FHIRResource and its extensions. Tests FHIRResource and its extensions. Jan 3, 2025
@jdisho jdisho changed the title Tests FHIRResource and its extensions. Tests FHIRResource and its extensions Jan 3, 2025
@jdisho jdisho assigned jdisho and unassigned philippzagar and LeonNissen Jan 4, 2025
Copy link
Member

@PSchmiedmayer PSchmiedmayer left a comment

Choose a reason for hiding this comment

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

Amazing; thank you for all the additional tests and extensions, amazing @jdisho! 🚀

Would be amazing to resolve some of the current failing CI builds and get this merged to get closer to #27, thank you for looking into this! 👍

@jdisho
Copy link
Member Author

jdisho commented Jan 20, 2025

I was waiting for #26 to be merged so I could test FHIRResource+Category. On it right now.

Copy link
Member

@philippzagar philippzagar left a comment

Choose a reason for hiding this comment

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

Thanks @jdisho for the extensive tests, they were quite tedious to write for sure but it helps us to push the test coverage! 🚀

Had some minor comments, mostly regarding formatting and styling. Please resolve all outstanding Linter warnings and compile errors, then we should be good to merge!

@jdisho
Copy link
Member Author

jdisho commented Jan 20, 2025

Thanks for the feedback @philippzagar
I've just committed the missing tests. I'll review your comments in detail a bit later and make the PR ready.

@PSchmiedmayer
Copy link
Member

Thank you @jdisho & @philippzagar!

@jdisho jdisho force-pushed the test-fhirresource branch from 1032031 to 69b63b3 Compare January 21, 2025 10:09
@jdisho jdisho force-pushed the test-fhirresource branch from 69b63b3 to 9e470ca Compare January 21, 2025 10:28
@jdisho jdisho marked this pull request as ready for review January 21, 2025 10:46
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.67%. Comparing base (aa1f1b5) to head (9e470ca).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##             main      #28       +/-   ##
===========================================
+ Coverage   34.97%   54.67%   +19.71%     
===========================================
  Files          21       21               
  Lines        1467     1467               
===========================================
+ Hits          513      802      +289     
+ Misses        954      665      -289     

see 11 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa1f1b5...9e470ca. Read the comment docs.

@jdisho
Copy link
Member Author

jdisho commented Jan 21, 2025

This is ready but let me know if there is anything else.

@jdisho
Copy link
Member Author

jdisho commented Jan 27, 2025

@philippzagar - Any new input from your side or should I merge it?

Copy link
Member

@philippzagar philippzagar left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks @jdisho! 🚀
At some point soon, we'll lift all of our Spezi modules to swift-testing, we'll then also do it for this code!

@philippzagar
Copy link
Member

@jdisho One more thing, please ensure that all commits on the respective branch are properly signed. The later commits on here are signed, however, the first ones are not.
For now, I'll bypass this requirement via my admin rights and merge the PR!

@philippzagar philippzagar merged commit 0f89a3d into main Jan 28, 2025
10 checks passed
@philippzagar philippzagar deleted the test-fhirresource branch January 28, 2025 08:29
@jdisho
Copy link
Member Author

jdisho commented Jan 28, 2025

Thank you 🫶 @philippzagar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants