-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Retention: Implement Filter: Keep Latest K #8255
Retention: Implement Filter: Keep Latest K #8255
Conversation
Signed-off-by: Nathan Lowe <public@nlowe.me>
Pull Request Test Coverage Report for Build 13562
💛 - Coveralls |
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.
Commented
"github.com/stretchr/testify/require" | ||
) | ||
|
||
func TestEvaluator_New(t *testing.T) { |
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.
Can we use the github.com/stretchr/testify
testing framework to drive the test cases as I did in the or/processor_test.go
?
i = len(artifacts) | ||
} | ||
|
||
return artifacts[:i], nil |
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.
The input list does not guarantee the order. I think you need to do sort first and then return the k ones.
…r/testify/suite Signed-off-by: Nathan Lowe <public@nlowe.me>
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
This ports my WIP implementation to match the updated proposal.
Fixes #6660
Signed-off-by: Nathan Lowe public@nlowe.me