-
Notifications
You must be signed in to change notification settings - Fork 15
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
BCDA 7122: add test coverage and cclf files #867
Conversation
@@ -115,43 +133,154 @@ func (s *CCLFTestSuite) TestImportCCLF0_SplitFiles() { | |||
assert.Equal(cclfFileValidator{totalRecordCount: 6, maxRecordLength: 549}, validator["CCLF8"]) | |||
} | |||
|
|||
func (s *CCLFTestSuite) TestImportCCLFDirectoryValid() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Golang has a feature called table driven testing, where you can run boiler plate code with different inputs and expected outcomes/outputs. We use this in some areas of our codebase, like in the fileprocessor.
Table driven testing doesn't necessarily offer any improvements to performance and unless we're testing a handful of different inputs/outputs then it's not terribly useful.
There are some places like TestImportCCLFDirectoryValid
, TestImportCCLFDirectoryInvalid
, TestImportCCLFDirectoryTwoLevels
, and this func where it might come in handy, but that's totally up to you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, Lauren! I'm going to check out table driven testing and see if I can apply it to some of the upcoming test coverage tickets.
cclfDirectory := filepath.Join(s.basePath, constants.CCLFDIR, "emptydir") | ||
_, _, _, err := ImportCCLFDirectory(cclfDirectory) | ||
assert.Nil(err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did this test not work? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one was funky; it passed locally using both go test, as well as the make unit-test, but github actions was having none of it, even after several attempts to mitigate (eg making file paths os-agnostic)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ 🌟
## 🎫 Ticket https://jira.cms.gov/browse/BCDA-7122 ## 🛠 Changes Updated test coverage for cclf.go. Rising from ~65% -> 89.3% statement coverage. Reaching 90+% will require more significant refactoring (as most branches are generally unreachable via normal means, and are dependent upon database failures mid-method / IO errors mid-method.) ## ℹ️ Context for reviewers Several canned files were created in order to fully flesh out some of the test scenarios. ## ✅ Acceptance Validation ![image](https://github.com/CMSgov/bcda-app/assets/120701369/fba416a0-d9ee-4860-ba64-2ac77260c9fb) There is also a commented out test that passes locally but fails the CI flow that adds another 0.5% coverage. ## 🔒 Security Implications - [ ] This PR adds a new software dependency or dependencies. - [ ] This PR modifies or invalidates one or more of our security controls. - [ ] This PR stores or transmits data that was not stored or transmitted before. - [ ] This PR requires additional review of its security implications for other reasons. If any security implications apply, add Jason Ashbaugh (GitHub username: StewGoin) as a reviewer and do not merge this PR without his approval. --------- Co-authored-by: Alex Dzeda <[email protected]>
🎫 Ticket
https://jira.cms.gov/browse/BCDA-7122
🛠 Changes
Updated test coverage for cclf.go. Rising from ~65% -> 89.3% statement coverage. Reaching 90+% will require more significant refactoring (as most branches are generally unreachable via normal means, and are dependent upon database failures mid-method / IO errors mid-method.)
ℹ️ Context for reviewers
Several canned files were created in order to fully flesh out some of the test scenarios.
✅ Acceptance Validation
There is also a commented out test that passes locally but fails the CI flow that adds another 0.5% coverage.
🔒 Security Implications
If any security implications apply, add Jason Ashbaugh (GitHub username: StewGoin) as a reviewer and do not merge this PR without his approval.