-
Notifications
You must be signed in to change notification settings - Fork 101
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
K8 describe test #306
K8 describe test #306
Conversation
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.
Also, you can look into net/http/httptest
if it is of sny use here.
func (m *MockClient) PrependReactor(method string, Name string, reactorFunc func(*http.Request) (*http.Response, error)) { | ||
originalDo := m.Do | ||
|
||
_ = func(req http.Request) (*http.Response, error) { | ||
//call the reactor funtion for every reqeust | ||
resp, err := reactorFunc(&req) | ||
if err != nil || resp != nil { | ||
// If the reactor function returns a response or error, return it immediately | ||
return resp, err | ||
} | ||
// Otherwise, call the original Do() method | ||
return originalDo(req) | ||
} | ||
} |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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 PrependReactor()
returns http Error response, if there are any errors when the DescribeFunc is called with specific options . The code here returns a reactor function for request for the DescribeFunc if the request have errors it returns the error. If it doesnt the originalDo(req)
handles the request and returns a response
if m.DescribeError != nil && strings.Contains(m.RequestUrl, "describe") { | ||
return &http.Response{ | ||
StatusCode: 404, | ||
Body: io.NopCloser(bytes.NewReader(m.Response)), | ||
}, m.DescribeError | ||
} | ||
return &http.Response{ | ||
StatusCode: 404, | ||
Body: io.NopCloser(bytes.NewReader(m.Response)), | ||
}, m.DescribeError |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
ok i see , i would change this
Hey @Philip-21 , I reviewed this and I still feel see that we are not testing our part of the code. You are testing the describe method on Describer using mock describer. But that function is in kubectl and is already been tested there. The part of code we want to test is still not being tested. |
Ok, What Part of the Code if i may ask. |
@Aisuko, please i am also waiting for your review on this. |
You are welcome. I saw you are trying to mock |
@Revolyssup please we are awaiting your review |
@Revolyssup , @Aisuko from this testcase, i created a mockclient which has an Api and i used a mock http response to handle the request from the mockclient , i just want to show you my progress so far. My testcase keeps failing , I feel it has to do with the MockClient I used as a parameter in the Describe() ,please i need assistance |
if err != nil { | ||
t.Errorf("Testcase failed for %s, coudnt get %s got %s, ", tc.Name, tc.ExpectedError, err) | ||
} | ||
|
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.
Please follow the golangci-lint
advice.
|
||
}) | ||
} | ||
|
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.
And here remove the whitespace
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.
sorry, i've just done that
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 saw the CI still gets stuck on the linter below. Maybe you can check it locally, any of go test
or golangci-lint
is ok.
ok github.com/layer5io/meshkit/utils/kubernetes 0.062s
=== RUN TestDescribe
=== RUN TestDescribe/Pod
--- FAIL: TestDescribe (0.00s)
--- FAIL: TestDescribe/Pod (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x29d50ea]
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 ran go test -v locally, it gave me the same error
=== RUN TestDescribe
=== RUN TestDescribe/Pod
--- FAIL: TestDescribe (0.00s)
--- FAIL: TestDescribe/Pod (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xc0000005 code=0x0 addr=0x0 pc=0x239398a]
goroutine 20 [running]:
testing.tRunner.func1.2({0x2596da0, 0x3aea4e0})
C:/Program Files/Go/src/testing/testing.go:1526 +0x24e
testing.tRunner.func1()
C:/Program Files/Go/src/testing/testing.go:1529 +0x39f
panic({0x2596da0, 0x3aea4e0})
C:/Program Files/Go/src/runtime/panic.go:884 +0x213
github.com/layer5io/meshkit/utils/kubernetes/describe.Describe(0x0, {{0x28360da, 0x5}, {0x283476a, 0x4}, 0x0, 0x0, 0x1})
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.
OK, let me check it.
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.
Make sure your parameter is not nil
The panic is happened in here
meshkit/utils/kubernetes/describe/describe.go
Line 100 in d0a5f69
describer, ok := describe.DescriberFor(kind, &client.RestConfig) |
You parameter &client.RestConfig
is nil and it will cause panic.
call describe.DescriberFor(kind, &client.RestConfig)
Unable to evaluate expression: error evaluating "&client.RestConfig" as argument clientConfig in function k8s.io/kubectl/pkg/describe.DescriberFor: client is nil
httpmock.RegisterResponder(tc.clientApi.Method, tc.clientApi.RequestUrl, | ||
httpmock.NewStringResponder(tc.clientApi.ResponseCode, tc.clientApi.Response)) | ||
|
||
output, err := Describe(tc.Client.Client, tc.Options) |
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 meshkitkube Client is not passed in any of the above test cases. You will have to initialise one and pass in each test case. It is initialised with a kubeconfig or when nothing is passed then it detects one from KUBECONFIG env variable. You can create a dummy kubeconfig and pass it to meshkitkube.New.
@Revolyssup @Aisuko i created a yml file and a meshkit.New() client , but when i ran the testcase this was the error i got |
The test kubeconfig looks something like the one given here: https://kubernetes.io/docs/tasks/access-application-cluster/configure-access-multiple-clusters/ |
|
@Revolyssup @Aisuko |
Hang in there, @Philip-21. We're rooting for you. 💪 |
Hi @Philip-21 , it looks like your HTTP server did not give you a response in the limited duration. |
Thanks Lee |
Maybe i would find a way to Increase the timeout for the HTTP client to allow more time for the server to respond |
Yes, that sounds about right. Sleeping is not ideal, but is sometimes the only reasonable approach (without undertaking a whole lot more work). |
0e614a0
to
ea04f38
Compare
ea04f38
to
8648837
Compare
Description
Recreated Unit test for the K8 describe package
This PR fixes #287
Signed commits