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

Test FHIRStore #30

Merged
merged 9 commits into from
Feb 3, 2025
Merged

Test FHIRStore #30

merged 9 commits into from
Feb 3, 2025

Conversation

jdisho
Copy link
Collaborator

@jdisho jdisho commented Jan 30, 2025

Test FHIRStore

Part of #27

♻️ Current situation & Problem

This class used to have 0% code coverage 🐒

⚙️ Release Notes

  • FHIRStoreTests: Tests basic CRUD operations, bundle loading, and resource management
  • FHIRStoreObservationChangesTests: Verifies state changes propagate correctly between resource categories
  • The _resources array is marked with @ObservationIgnored to prevent changes to this array from triggering updates to all computed properties.

📚 Documentation

Comments are added throughout the code.

✅ Testing

It's all about the tests.

📝 Code of Conduct & Contributing Guidelines

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

@jdisho jdisho self-assigned this Jan 30, 2025
@jdisho jdisho added the enhancement New feature or request label Jan 30, 2025
Copy link

codecov bot commented Jan 30, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 54.74%. Comparing base (0f89a3d) to head (b059328).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #30      +/-   ##
==========================================
+ Coverage   54.67%   54.74%   +0.07%     
==========================================
  Files          21       21              
  Lines        1467     1467              
==========================================
+ Hits          802      803       +1     
+ Misses        665      664       -1     
Files with missing lines Coverage Δ
Sources/SpeziFHIR/FHIRStore.swift 99.03% <100.00%> (+0.98%) ⬆️

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 0f89a3d...b059328. Read the comment docs.

@jdisho
Copy link
Collaborator Author

jdisho commented Jan 30, 2025

This had started before you introduced swift-testing. If you think it's worth it, I'm ok migrating it. Just fyi @philippzagar

@jdisho jdisho mentioned this pull request Jan 30, 2025
4 tasks
@philippzagar
Copy link
Member

This had started before you introduced swift-testing. If you think it's worth it, I'm ok migrating it. Just fyi @philippzagar

I'm okay with sticking to XCTest for this PR, but feel free to tackle the migration to swift-testing in a separate one! 🚀

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.

Great PR @jdisho, thanks so much for adding all the additional test logic and fixing a small (performance) bug in the current implementation! 🚀
Only had some small comments. The tests in general are often a bit repetitive but as this is testing code I'm fine with that!

@jdisho
Copy link
Collaborator Author

jdisho commented Feb 3, 2025

Thanks for the review @philippzagar! Ready for another look 👀

The tests in general are often a bit repetitive but as this is testing code I'm fine with that!

And yeah, these tests will get a nice cleanup once we bring in swift-testing with parameterized tests. The test in FHIRStoreObservationChangesTests could have been combined with tests in FHIRStoreTests, but I separated the observation part from the insertion and removal part for clearer separation of concerns.

@philippzagar
Copy link
Member

Thanks for the review @philippzagar! Ready for another look 👀

The tests in general are often a bit repetitive but as this is testing code I'm fine with that!

And yeah, these tests will get a nice cleanup once we bring in swift-testing with parameterized tests. The test in FHIRStoreObservationChangesTests could have been combined with tests in FHIRStoreTests, but I separated the observation part from the insertion and removal part for clearer separation of concerns.

Perfect, thanks for adding all the tests, looking forward to the PR that will include swift-testing! 🚀

@jdisho jdisho merged commit 9e275f8 into main Feb 3, 2025
9 checks passed
@jdisho jdisho deleted the test-fhirstore branch February 3, 2025 18:43
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
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants