-
Notifications
You must be signed in to change notification settings - Fork 664
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: added tests for the cmd-filters package #892
base: main
Are you sure you want to change the base?
test: added tests for the cmd-filters package #892
Conversation
2d3afd1
to
cfa05a7
Compare
98dfa72
to
43531d6
Compare
@AlexsJones, I've tried to add some tests to the cmd-filters package and a minor modification in the filters list command. Could you please go through this PR whenever you have a moment? Thanks! |
43531d6
to
3ed5844
Compare
c4dccb1
to
2fd1c46
Compare
@AlexsJones, I've fixed the CI errors. |
ea579a7
to
1a473e7
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.
i have some questions.
overall very good and nice work on sorting filters.
cmd/filters/add_test.go
Outdated
err := addCmd.Args(&cobra.Command{}, []string{"arg1"}) | ||
require.NoError(t, err) | ||
|
||
err = addCmd.Args(&cobra.Command{}, []string{"arg1", "arg2"}) |
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.
what are these arg1, arg2 values? what will actual values for these?
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 thought about it and realized that these are useless, hence, I've got rid of them in the latest version.
|
||
// Initially the list contained 12 filters, after deleting one filter in this | ||
// test, now it should be left with 11 filters only. | ||
require.Equal(t, 11, len(viper.GetStringSlice("active_filters"))) |
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.
how about use length of data[active_filters]. it's more safe later
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.
data
is not up to date with the active filters, its sole purpose was to populate the configuration file.
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 see. but we can do this. len(data['active_filters']) - 1
since we remove one filter
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.
@VaibhavMalik4187 why length of active filter is 11? i think it should be 10 (11 - 1)
This commit adds new tests for the `cmd/filters` package. As a result, the code coverage of the package has increased from 0% to almost 51% This also includes a minor adjustment in the list command. Now, the list of the filters will always be in sorted order for better consistency. Signed-off-by: VaibhavMalik4187 <[email protected]>
1a473e7
to
5b8ef5d
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.
@k8sgpt-ai/k8sgpt-maintainers can someone also review?
// Set the configuration file in viper | ||
configFileName := "delete-config.json" | ||
data := map[string]interface{}{ | ||
"active_filters": []string{ |
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.
this has 11 filters not 12
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.
it's pretty old pr.
i left a question on test
|
||
// Initially the list contained 12 filters, after deleting one filter in this | ||
// test, now it should be left with 11 filters only. | ||
require.Equal(t, 11, len(viper.GetStringSlice("active_filters"))) |
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.
@VaibhavMalik4187 why length of active filter is 11? i think it should be 10 (11 - 1)
📑 Description
This commit adds new tests for the
cmd/filters
package. As a result,the code coverage of the package has increased from 0% to almost 75%
This also includes a minor adjustment in the list command. Now, the list
of the filters will always be in sorted order for better consistency.
✅ Checks