Increasing test coverage #401
Conversation
jotak
commented
Mar 7, 2023
jotak
commented
- k8s enrichment
- convert utils
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
@@ Coverage Diff @@
## main #401 +/- ##
==========================================
+ Coverage 61.45% 63.67% +2.21%
==========================================
Files 91 92 +1
Lines 6393 6467 +74
==========================================
+ Hits 3929 4118 +189
+ Misses 2226 2109 -117
- Partials 238 240 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
- k8s enrichment - convert utils
| func (indexMock *IndexerMock) GetByKey(key string) (interface{}, bool, error) { | ||
| args := indexMock.Called(key) | ||
| return args.Get(0), args.Bool(1), args.Error(2) | ||
| } | ||
|
|
||
| func (informerMock *InformerMock) GetIndexer() cache.Indexer { | ||
| args := informerMock.Called() | ||
| return args.Get(0).(cache.Indexer) | ||
| } | ||
|
|
||
| func TestGetInfoPods(t *testing.T) { | ||
| kubeData := KubeData{} | ||
| // pods informer | ||
| pidx := IndexerMock{} | ||
| pidx.On("ByIndex", IndexIP, "1.2.3.4").Return([]interface{}{&Info{ | ||
| func (m *IndexerMock) mockPod(ip, name, namespace, nodeIP string, owner *Owner) { |
There was a problem hiding this comment.
There is inconsistency in the receiver variable name of *IndexerMock. Sometimes it's indexMock and sometimes it's m. I think it'll make sense to rename the instances of indexMock to m (or vice versa).
| @@ -47,50 +47,161 @@ func (indexMock *IndexerMock) ByIndex(indexName, indexedValue string) ([]interfa | |||
| return args.Get(0).([]interface{}), args.Error(1) | |||
| } | |||
There was a problem hiding this comment.
I see there is a kubernetes-mock.go file. Would it make sense to move the mocks defined in this file to that file?
pkg/utils/convert_test.go
Outdated
| }} | ||
| for _, tc := range cases { | ||
| tc := tc | ||
| t.Run(fmt.Sprintf("%t", tc.input), func(t *testing.T) { |
There was a problem hiding this comment.
%t prints the word true or false. Perhaps %T was meant here? (%T prints a Go-syntax representation of the type of the value)
Reference:
https://pkg.go.dev/fmt#hdr-Printing
There was a problem hiding this comment.
oops good catch ... for some reason it wasn't writing true/false, but the some garbage AND the type so I didn't pay much attention
pkg/utils/convert_test.go
Outdated
| "github.com/stretchr/testify/assert" | ||
| ) | ||
|
|
||
| func TestConvertToFloat64(t *testing.T) { |
There was a problem hiding this comment.
Since this tests other functions besides ConvertToFloat64(), I would rename it to something like TestConvertToNumericTypes()
pkg/utils/convert_test.go
Outdated
| wanti: 1, | ||
| }, { | ||
| input: "1", | ||
| wantf64: 1, |
There was a problem hiding this comment.
Why is wantf64 equal to 1 and not equal to 1.0?
pkg/utils/convert_test.go
Outdated
| wanti: 42, | ||
| }} | ||
| for _, tc := range cases { | ||
| tc := tc |
There was a problem hiding this comment.
What does this line contribute?
There was a problem hiding this comment.
it's not really useful unless we make tests run in parallel at some point (it captures the variable in the more narrow scope). I can remove it
|
thanks @ronensc - feedback should be addressed |
jotak
added
no-qe
