-
Notifications
You must be signed in to change notification settings - Fork 16
Add support for backend config in spk infra scaffold
#106
Conversation
@yradsmikham look good business logic wise. What is the plan to get the coverage greater than 44%? |
@andrebriggs, we are definitely keeping an eye on line and branch coverage while writing tests, and the hope is that scaffold would expand a little more in the coming sprints, which implies more unit testing. If you look at the code coverage breakdown, you'll find that the reason why code coverage is "low" is because a lot of the code written for scaffold is reliant on file system (fs). There's certainly no issue with writing "mocks" for code that interacts with file system, but that starts to bridge the conversation around writing integration tests versus unit tests. I find that code coverage statistics are something to consider when writing code, but does not always give an accurate depiction of code quality. |
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.
Lgtm
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.
Running int a small thing where .backip still continues to get appended on an existing directory I am scaffolding. Not sure how we want to handle this in the future or note as a quick bug:
Directory: C:\Users\naros\Desktop\Microsoft\fy20\spk\spk-infra\spk\discovery-service
Mode LastWriteTime Length Name
---- ------------- ------ ----
-a---- 10/29/2019 11:14 AM 1137 definition.json
-a---- 10/29/2019 10:45 AM 2033 main.tf
-a---- 10/29/2019 10:45 AM 698 README.md
-a---- 10/29/2019 10:45 AM 795 terraform.tfvars.backup.backup.backup
-a---- 10/29/2019 10:45 AM 2670 variables.tf
This is resolved in this PR now :) |
Closes microsoft/bedrock#722 and microsoft/bedrock#727.