Skip to content
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

Closed
wants to merge 0 commits into from
Closed

Conversation

Philip-21
Copy link
Member

@Philip-21 Philip-21 commented Apr 19, 2023

Description
Recreated Unit test for the K8 describe package
This PR fixes #287

Signed commits

  • Yes, I signed my commits.

Copy link
Contributor

@Azanul Azanul left a 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.

Comment on lines 27 to 40
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.

Copy link
Member Author

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

Comment on lines 48 to 57
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.

Copy link
Member Author

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

@Revolyssup
Copy link
Contributor

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.

@Philip-21
Copy link
Member Author

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.

@Philip-21
Copy link
Member Author

@Aisuko, please i am also waiting for your review on this.

@Aisuko
Copy link
Member

Aisuko commented Apr 20, 2023

@Aisuko, please i am also waiting for your review on this.

You are welcome. I saw you are trying to mock api-server response by using http. Maybe we can invite @Revolyssup to give us a review here.

@Philip-21
Copy link
Member Author

@Aisuko, please i am also waiting for your review on this.

@Revolyssup please we are awaiting your review

@Philip-21
Copy link
Member Author

Philip-21 commented Apr 30, 2023

@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

@Aisuko Aisuko self-requested a review May 1, 2023 01:06
if err != nil {
t.Errorf("Testcase failed for %s, coudnt get %s got %s, ", tc.Name, tc.ExpectedError, err)
}

Copy link
Member

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.


})
}

Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

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]

Copy link
Member Author

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})

Copy link
Member

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.

Copy link
Member

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

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)
Copy link
Contributor

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.

@Philip-21
Copy link
Member Author

@Revolyssup @Aisuko i created a yml file and a meshkit.New() client , but when i ran the testcase this was the error i got
error in loading Kube configs for tests no kind "Job" is registered for version "batch/v1" in scheme "pkg/runtime/scheme.go:100" , please i seem not to understand this

@Revolyssup
Copy link
Contributor

@Revolyssup @Aisuko i created a yml file and a meshkit.New() client , but when i ran the testcase this was the error i got error in loading Kube configs for tests no kind "Job" is registered for version "batch/v1" in scheme "pkg/runtime/scheme.go:100" , please i seem not to understand this

The test kubeconfig looks something like the one given here: https://kubernetes.io/docs/tasks/access-application-cluster/configure-access-multiple-clusters/
You are passing Kubernetes resources in kubeconfig.

@Philip-21
Copy link
Member Author

Philip-21 commented May 6, 2023

@Revolyssup @Aisuko i created a yml file and a meshkit.New() client , but when i ran the testcase this was the error i got error in loading Kube configs for tests no kind "Job" is registered for version "batch/v1" in scheme "pkg/runtime/scheme.go:100" , please i seem not to understand this

@Philip-21
Copy link
Member Author

Philip-21 commented May 9, 2023

@Revolyssup @Aisuko
i have adjusted the test.yml file it seems to work well , but looking at the the new code i have in the file,
if err != nil {
t.Errorf("Testcase failed for %s, couldn't get %s got %s, ", tc.Name, tc.ExpectedError, err)
}
,
the unit test fails due to this error checked , when running the testcase i got this error message
"Testcase failed for Pod, couldn't get %!s() got Get "https://5.6.7.8/api/v1/namespaces/test/pods/mypod": dial tcp 5.6.7.8:443: connectex: A connection attempt failed because the connected party did not properly respond after a period of time, or established connection failed because connected host has failed to respond.".

@leecalcote
Copy link
Member

Hang in there, @Philip-21. We're rooting for you. 💪

@Aisuko
Copy link
Member

Aisuko commented May 13, 2023

Hi @Philip-21 , it looks like your HTTP server did not give you a response in the limited duration.

@Philip-21
Copy link
Member Author

Hang in there, @Philip-21. We're rooting for you. 💪

Thanks Lee

@Philip-21
Copy link
Member Author

Hi @Philip-21 , it looks like your HTTP server did not give you a response in the limited duration.

Maybe i would find a way to Increase the timeout for the HTTP client to allow more time for the server to respond

@leecalcote
Copy link
Member

leecalcote commented May 20, 2023

Hi @Philip-21 , it looks like your HTTP server did not give you a response in the limited duration.

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).

@Philip-21 Philip-21 closed this May 20, 2023
@Philip-21 Philip-21 deleted the k8-describe-test branch May 20, 2023 18:24
@Philip-21 Philip-21 restored the k8-describe-test branch May 20, 2023 18:43
@Philip-21 Philip-21 mentioned this pull request May 20, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Writing Unit tests For k8 in Utils package
5 participants