-
Notifications
You must be signed in to change notification settings - Fork 369
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
Fixing search results #863
Fixing search results #863
Conversation
Welcome @wallentx! |
we should probably add a test case here https://github.com/kubernetes-sigs/krew/blob/master/cmd/krew/cmd/search_test.go#L31-L62 with an empty keyword expecting the full set of names. this bug has probably been in krew for some time |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahmetb, wallentx The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
I've made the requested changes to the search_test 👍 |
actually looking into it a bit more, we have this integration test https://github.com/kubernetes-sigs/krew/blob/master/integration_test/search_test.go#L24-L34 but it didnt catch this because it just checks that there are at least 10 lines in the output. wonder if we should update this to an exact number (1 + number of plugins in the cloned index; the 1 being the header line in the search output). |
@chriskim06 I've got |
ill update the integration test in a follow up pr later today. thanks for catching this and fixing! /lgtm |
Fixes #862
I'm not really sure what might could be added to the tests to account for this.