-
Notifications
You must be signed in to change notification settings - Fork 779
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: vendor in testify (again) #2374
Conversation
Signed-off-by: Alex Pana <8968914+acpana@users.noreply.github.com>
Codecov ReportBase: 53.47% // Head: 53.64% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #2374 +/- ##
==========================================
+ Coverage 53.47% 53.64% +0.17%
==========================================
Files 117 117
Lines 10230 10230
==========================================
+ Hits 5470 5488 +18
+ Misses 4341 4325 -16
+ Partials 419 417 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
minor typo, otherwise LGTM
Thanks for adding this @acpana! Is the thought that we would start using testify for new unit tests? Is there a need to move existing tests over? when should we use testify vs existing framework? |
Co-authored-by: Sertaç Özercan <852750+sozercan@users.noreply.github.com> Signed-off-by: alex <8968914+acpana@users.noreply.github.com>
We could! I am a bit opinionated in that I like the expressivity of a
I don't think there's a need to embark on a wholesale refactor. But I can be convinced otherwise. I've seen code bases that adopted
I think it depends on you (the maintainers) and if you want to encourage the usage. Personally, I'd use |
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
Signed-off-by: Alex Pana 8968914+acpana@users.noreply.github.com
What this PR does / why we need it:
This patch vendors in:
Also the patch uses
testify/require
to add some low level unit tests. (I indiscriminately looked for one func/ package w low coverage).Special notes for your reviewer:
testify
as per Remove unused stretchr/testify from go.mod #1330 but we may have not been actively using itWhy?
legend: + =
pro
; - =con
+ I think
testify/require
andtestify/assert
are the more expressive frameworks used in OSS go development for testing. eg1.+ IMO having this dep and using it actively lowers the barrier to entry for contributions when it comes to testing code.
-
testify
can be a bit heavy (since it contains a few packages)- it's not without quirks; for instance: https://tychoish.com/post/against-testify/