-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
sdk/log/logtest: Add RecordBuilder #5234
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5234 +/- ##
=====================================
Coverage 84.6% 84.6%
=====================================
Files 258 259 +1
Lines 17349 17428 +79
=====================================
+ Hits 14678 14749 +71
- Misses 2360 2368 +8
Partials 311 311
|
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.
I do not have a strong opinion on my comments.
sdk/log/logtest/builder.go
Outdated
} | ||
|
||
// TODO: comment. | ||
func (b Builder) Record() sdklog.Record { |
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.
func (b Builder) Record() sdklog.Record { | |
func (b Builder) Build() sdklog.Record { |
Builder builds. However, golang uses String
for strings.Builder
.
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.
That is why I decided to name it Record to follow the strings.Builder
Making it an "actual PR" (not just a spike) based on initial feedback. |
…try-go into sdklogtest-2
Closing per #5258 |
Fixes #5192.
My goal was to design the builder in a way to make it as much usable for exporter and processor testing as possible.
See test example for usage.
See also alternative design: #5258