Implement unit-tests for the codebase #192
Conversation
* This is a exploratory exercise on writing unit-test for data_collector.go and nic_job_list.go * The nic_job_list_test.go works but data_collector_test.go does not * The tests themselves are basic to establish a familiarity in writing tests
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Copilot <[email protected]>
Remove redundant TODO. Co-authored-by: Copilot <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Remove redundant comment Co-authored-by: Copilot <[email protected]>
Removed custom fake types; use fake.NewSimpleClientset instead Co-authored-by: Copilot <[email protected]>
Remove redundant mock-logger code. Co-authored-by: Copilot <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Remove irrelevant comment Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
Implements comprehensive unit tests for the nginx-supportpkg-for-k8s codebase, covering job execution, data collection, and CRD functionality.
- Unit tests for all job lists (NIM, NIC, NGX, NGF, Common)
- Data collector functionality and client initialization tests
- CRD package validation tests
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/data_collector/data_collector.go | Refactors client interface type and method accessibility for testability |
| pkg/data_collector/data_collector_test.go | Tests DataCollector initialization, tarball creation, and namespace validation |
| pkg/jobs/common_job_list.go | Updates deprecated API calls to use Discovery() method |
| pkg/jobs/common_job_list_test.go | Tests common Kubernetes resource collection jobs |
| pkg/jobs/job_test.go | Tests core job execution patterns and edge cases |
| pkg/jobs/nic_job_list_test.go | Tests NGINX Ingress Controller specific job execution |
| pkg/jobs/ngf_job_list_test.go | Tests NGINX Gateway Fabric job functionality |
| pkg/jobs/ngx_job_list_test.go | Tests NGINX specific job execution |
| pkg/jobs/nim_job_list_test.go | Tests NGINX Instance Manager job execution with exclude flags |
| pkg/crds/crd_test.go | Validates CRD list structure and content |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Taking the address of a slice element from a function call creates a pointer to a temporary value. Store the slice in a variable first, then take the address: jobs := CommonJobList(); podListJob = &jobs[i] Co-authored-by: Copilot <[email protected]>
* Introduce a mock form helm, metrics clients
* Also update go.mod and go.sum
* update Makefile for clean and test targets * Fix product_info job which was timing out due to absence of nginx-ingress container
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Great approach with the |
* delete commented out code * delete CRD test as it was not useful
| }, | ||
| } | ||
|
|
||
| client := fake.NewSimpleClientset(objs...) |
There was a problem hiding this comment.
NewSimpleClientset is mark as deprecated.
| client := fake.NewSimpleClientset(objs...) | |
| client := fake.NewClientset(objs...) |
There was a problem hiding this comment.
From what I see, we should iterate over all jobs and verify that:
- The response is not an error
- The job does not time out
- The job returns more than 0 files
- Number of files and names are expected
- File content format (for those that are JSON) is valid
The test TestCommonJobList_SelectedJobsProduceFiles already covers 1 to 3. To cover 4 and 5, I would expand that same test and verify via a switch case statement for each job (compare names) that we get the right files and formats.
Also, we could include a default selector in the switch case statement to catch those cases where the job name is not known. In that case, the test should fail, forcing us to set a new case statement that contemplates the job that is not being covered in the test.
There was a problem hiding this comment.
Same review than in common_job_list_test.go apply here.
There was a problem hiding this comment.
Same review than in common_job_list_test.go apply here.
There was a problem hiding this comment.
Same review than in common_job_list_test.go apply here.
There was a problem hiding this comment.
Same review than in common_job_list_test.go apply here.
Docs say the return of NewMockClient is always non-nil. Therefore, removing redundant check. Co-authored-by: Daniel Aresté <[email protected]>
NewSimpleClientset is mark as deprecated, therefore replacing. Co-authored-by: Daniel Aresté <[email protected]>
2 participants
Proposed changes
This PR address Issue 40 - Create Unit-tests.
Checklist
Before creating a PR, run through this checklist and mark each as complete.