-
Notifications
You must be signed in to change notification settings - Fork 21
Reduce technical debt of code duplication in automated tests #69
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
Conversation
8a8984d
to
24ca7e8
Compare
c8b3ebd
to
4614cb8
Compare
ea09312
to
60e9f6f
Compare
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.
Thanks for getting this in so early @arrestle.
I've skimmed the refactoring in the _test.go
and utils files, and that looks good. Just a couple of points there. I also ran the acceptance tests locally before/after and confirmed the test coverage has not decreased. I would appreciate if others could weigh in as well on the refactored logic.
I'm requesting changes on two specific areas:
- There are changes in the README and Makefile around what to run to support the acceptance tests. I think we'll be addressing that in a future ticket around contribution guidelines, so I don't think we're ready for this change and my request is that we hold these back.
- There are additions to
GetWithStatus
that enhance error checking. I had some specific feedback on individual checks as well and some general refactoring feedback. The ask here is to review these checks and see what can be refactored, removed, or changed so that details aren't lost.
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.
Lets collapse the Makefile target into one and use environment variables to set it up.
Co-authored-by: Dan Leehr <[email protected]>
e031d36
to
183c71b
Compare
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.
Thanks for addressing the feedback. LGTM.
Uh oh!
There was an error while loading. Please reload this page.