-
Notifications
You must be signed in to change notification settings - Fork 370
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
Add unit test for pkg/monitor #4370
Conversation
pkg/monitor/controller_test.go
Outdated
|
||
for _, tt := range tc { | ||
t.Run(tt.name, func(t *testing.T) { | ||
generated := controller.syncExternalNode(tt.key) |
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 would suggest to name it err
directly which is easier to understand what's the returned value.
@NamanAg30 any update for this PR? |
Will update this today |
0d69c6a
to
367c59b
Compare
@luolanzone PR updated |
75dd34c
to
defeee0
Compare
Codecov Report
@@ Coverage Diff @@
## main #4370 +/- ##
==========================================
- Coverage 66.50% 64.58% -1.92%
==========================================
Files 402 402
Lines 57247 57227 -20
==========================================
- Hits 38070 36960 -1110
- Misses 16463 17543 +1080
- Partials 2714 2724 +10
|
pkg/monitor/controller_test.go
Outdated
en := | ||
&v1alpha1.ExternalNode{ |
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.
en := | |
&v1alpha1.ExternalNode{ | |
en := &v1alpha1.ExternalNode{ |
pkg/monitor/controller_test.go
Outdated
err := controller.controllerMonitor.deleteAgentCRD(tt.name) | ||
if err != nil { | ||
assert.Equal(t, tt.expectedError, err.Error()) | ||
} else { | ||
assert.Equal(t, tt.expectedError, "") | ||
} |
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.
When deleteAgentCRD is successful, you should check existing agent CRD is no longer existing in the fake client via fakeClient.get(...).
And you can fake the failure like this https://github.com/antrea-io/antrea/blob/main/pkg/apiserver/certificate/cacert_controller_test.go#L83-L87 to cover the case when delete is failed.
agentCRD.ResourceVersion = "0" | ||
} | ||
for _, externalnodes := range tt.externalnodes { | ||
|
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.
Remove empty line.
pkg/monitor/controller_test.go
Outdated
t.Run("1", func(t *testing.T) { | ||
if tt.controllerCRD == "" { | ||
controller.controllerMonitor.syncControllerCRD() | ||
assert.Equal(t, crdName, controller.controllerMonitor.controllerCRD.Name) |
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.
You should check the new created controller CRD is the same as you expected instead of checking the name only.
pkg/monitor/controller_test.go
Outdated
} else { | ||
controller.controllerMonitor.controllerCRD, _ = controller.controllerMonitor.createControllerCRD(tt.controllerCRD) | ||
controller.controllerMonitor.syncControllerCRD() | ||
assert.NotNil(t, controller.controllerMonitor.controllerCRD) |
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.
Ditto, this is partially update, you should check the new updated one is expected one instead of checking if it's nil or not.
pkg/monitor/controller_test.go
Outdated
groupEntityIndex := grouping.NewGroupEntityIndex() | ||
labelIdentityIndex := labelidentity.NewLabelIdentityIndex() | ||
|
||
multiclusterEnabled := features.DefaultFeatureGate.Enabled(features.Multicluster) && true |
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 don't think you need this line, you can just pass false
to NewNetworkPolicyController
directly.
6c8bb60
to
5a76d9a
Compare
5a76d9a
to
f03eb02
Compare
@lanluozone This PR has been updated . Most of the test cases have been covered and I have also added unit tests for pkg/monitor/agent.go. |
f03eb02
to
435b0b7
Compare
435b0b7
to
fd53cb9
Compare
pkg/monitor/agent_test.go
Outdated
for _, tt := range tc { | ||
t.Run(tt.name, func(t *testing.T) { | ||
switch tt.name { | ||
case "Agent CRD exists-updated Agent CRD partially successfully": |
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 don't think it's a good way to run test based on test name. Please add necessary test fields to distinguish your test cases. Please take this https://github.com/antrea-io/antrea/blob/main/pkg/apiserver/certificate/cacert_controller_test.go#L104-L127 or here https://github.com/antrea-io/antrea/blob/main/pkg/controller/externalnode/controller_test.go#L599-L631 to organize your tests.
pkg/monitor/controller_test.go
Outdated
controller := newControllerMonitor(clientset) | ||
initController(controller) | ||
err := controller.controllerMonitor.syncExternalNode(tt.key) | ||
if err != nil { |
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.
Usually we would check like this:
if tt.expectedError != "" {
assert.Equal(t, tt.expectedError, err.Error())
} else {
// do other checks.
}
af24920
to
132fa6f
Compare
Pr updated @luolanzone |
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.
pkg/monitor/controller_test.go
Outdated
} else { | ||
assert.Nil(t, err) | ||
crd, _ := controller.controllerMonitor.client.CrdV1beta1().AntreaAgentInfos().Get(context.TODO(), "TestExternalNode", metav1.GetOptions{}) | ||
assert.Equal(t, tt.expectedAgentCRD, crd) |
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 didn't see any case has tt.expectedAgentCRD
value. I suppose you need one more case to cover 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.
Added required test case
dcc0b96
to
7f75c05
Compare
pkg/monitor/agent_test.go
Outdated
monitor.querier.GetAgentInfo(partiallyUpdatedCRD, true) | ||
monitor.syncAgentCRD() | ||
crd, err := monitor.client.CrdV1beta1().AntreaAgentInfos().Get(context.TODO(), "testAgentCRD", metav1.GetOptions{}) | ||
assert.NoError(t, 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.
this should probably be require.NoError
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.
fixed
pkg/monitor/agent_test.go
Outdated
_, err := monitor.client.CrdV1beta1().AntreaAgentInfos().Update(context.TODO(), existingCRD, metav1.UpdateOptions{}) | ||
assert.Equal(t, "error updating agent crd", err.Error()) |
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 am not 100% sure this is useful (same for tests below). This is not really testing functionality, this is just testing your reactor above. You can keep it, but I would suggest adding a clarifying comment to avoid confusion.
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.
fixed
pkg/monitor/agent_test.go
Outdated
monitor.agentCRD = existingCRD | ||
monitor.querier.GetAgentInfo(partiallyUpdatedCRD, true) | ||
monitor.syncAgentCRD() | ||
crd, err := monitor.client.CrdV1beta1().AntreaAgentInfos().Get(context.TODO(), "testAgentCRD", metav1.GetOptions{}) |
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 suggest declaring ctx := context.Background()
at the top of the test and using that context for all your sub-tests. Same for controller tests.
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.
fixed
pkg/monitor/agent_test.go
Outdated
func newAgentMonitor(crdClient *fakeclientset.Clientset, t *testing.T) *agentMonitor { | ||
client := fake.NewSimpleClientset() | ||
ctrl := gomock.NewController(t) | ||
defer ctrl.Finish() |
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 is a bit weird. It feels like ctrl.Finish()
should be called at the end of each individual test, not when this function returns.
The good news is you don't actually need to call ctrl.Finish
any more. From the gomock documentation:
New in go1.14+, if you are passing a *testing.T into NewController function you no longer need to call ctrl.Finish() in your test methods.
So you should be able to remove this defer statement.
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.
fixed
pkg/monitor/controller_test.go
Outdated
|
||
func initController(controller *fakeController) { | ||
stopCh := make(chan struct{}) | ||
defer close(stopCh) |
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 don't understand this. Do you mean to close stopCh
when this function returns? This doesn't seem correct and I would be surprised if the tests were not flaky. I don't think the informer factories should be stopped before the end of each test. You can pass a stopCh
as a parameter to the function instead.
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.
fixed
pkg/monitor/controller_test.go
Outdated
) | ||
|
||
const ( | ||
informerDefaultResync = 12 * time.Second |
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.
can you add a comment explaining why you are choosing 12 seconds here?
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.
changed to default value
pkg/monitor/controller_test.go
Outdated
if tt.expectedError != "" { | ||
assert.Equal(t, tt.expectedError, err.Error()) | ||
} else { | ||
assert.Nil(t, 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.
you should use NoError
instead of Nil
for readability
pkg/monitor/controller_test.go
Outdated
name: "Invalid key format", | ||
key: "namespace/name/error", | ||
requeueNum: 1, |
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 is a bad example of a transient error. It should not happen and a requeue in this case would not help. A better example would be c.externalNodeLister.ExternalNodes(namespace).Get(name)
returning an error.
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 changed this but I have checked the error of _, _, err := splitKeyFunc(tt.key.(string))...Will that be okay?
pkg/monitor/controller_test.go
Outdated
name: "Invalid key format", | ||
key: "namespace/name/error", | ||
requeueNum: 1, |
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.
same comment as above
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.
Same as above
7f75c05
to
7c21302
Compare
@antoninbas comments have been addressed.....Pr updated |
pkg/monitor/controller_test.go
Outdated
informerDefaultResync = 12 * time.Hour | ||
) | ||
|
||
var ctx = context.Background() |
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.
declare ctx := context.Background()
at the beginning of each test function that requires a context. Don't declare the context as a global variable.
pkg/monitor/controller_test.go
Outdated
ResourceVersion: "", | ||
Generation: 0, | ||
}, | ||
NetworkPolicyControllerInfo: v1beta1.NetworkPolicyControllerInfo{ | ||
NetworkPolicyNum: 0, | ||
AddressGroupNum: 0, | ||
AppliedToGroupNum: 0, |
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.
maybe you could omit all the fields which have a default value (all the fields except for the Name
). It will not impact the test but it will help reduce the verbosity.
pkg/monitor/controller_test.go
Outdated
ResourceVersion: "", | ||
Generation: 0, | ||
}, | ||
NetworkPolicyControllerInfo: v1beta1.NetworkPolicyControllerInfo{ | ||
NetworkPolicyNum: 0, | ||
AddressGroupNum: 0, | ||
AppliedToGroupNum: 0, |
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.
same comment as above
agentCRD := new(v1beta1.AntreaAgentInfo) | ||
agentCRD.Name = crd | ||
agentCRD.ResourceVersion = "0" |
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.
we usually use this syntax:
agentCRD := &1beta1.AntreaAgentInfo{
Name: crd,
ResourceVersion: "0",
}
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 syntax gives error 'unknown field Name in struct literal' & 'unknown field ResourceVersion in struct literal'
pkg/monitor/controller_test.go
Outdated
initController(controller, stopCh) | ||
defer close(stopCh) | ||
controller.controllerMonitor.controllerCRD = existingCRD | ||
controller.controllerMonitor.querier.GetControllerInfo(partiallyUpdatedCRD, true) |
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.
why are you calling GetControllerInfo
yourself in the test, I don't understand? Isn't syncControllerCRD
supposed to call it?
}) | ||
} | ||
|
||
func TestEnqueueNode(t *testing.T) { |
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 don't think these "Enqueue" tests are very useful, this is what the function is like:
func (monitor *controllerMonitor) enqueueNode(obj interface{}) {
node := obj.(*corev1.Node)
key, _ := keyFunc(node)
monitor.nodeQueue.Add(key)
}
And if you really want to write a test for it, it can be much shorter. I don't think you need to call initController
for example.
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 removed init Controller from the two test cases ....Do these need to be shortened further?These functions are not getting covered by other functions , that is why I have included these tests.
pkg/monitor/controller_test.go
Outdated
for _, tt := range tc { | ||
t.Run(tt.name, func(t *testing.T) { | ||
controller.controllerMonitor.externalNodeQueue.Add(tt.key) | ||
controller.controllerMonitor.processNextExternalNodeWorkItem() | ||
if tt.expectedError != "" { | ||
_, _, err := splitKeyFunc(tt.key.(string)) | ||
assert.Equal(t, tt.expectedError, err.Error()) | ||
} else { | ||
assert.Equal(t, tt.requeueNum, controller.controllerMonitor.externalNodeQueue.NumRequeues(tt.key)) | ||
} | ||
}) | ||
} |
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 am not sure the test makes sense any more. requeueNum
is also always 0.
The only reason to keep this test IMO is if you can find a way to have c.externalNodeLister.ExternalNodes(namespace).Get(name)
return an error (different from NotFound). Can you do it with a reactor like in other tests? I'm not sure this is possible though, maybe @tnqn knows.
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 don' think a reactor can be added c.externalNodeLister.Should I keep these tests ,Since these are not covered by other tests ...coverage will decrease
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.
If a test is not useful, it's better to remove it. Otherwise, it's just code bloat.
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.
Removed these tests
7c21302
to
a830397
Compare
a830397
to
e22948b
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.
make sure you run the tests a large number of time (e.g., with -count=300
) to make sure that the new tests are not flaky
pkg/monitor/agent_test.go
Outdated
monitor := NewAgentMonitor(crdClient, querier) | ||
return monitor |
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.
nit: return NewAgentMonitor(crdClient, querier)
is less verbose
pkg/monitor/controller_test.go
Outdated
if tt.expectedAgentCRD == nil { | ||
_, name, _ := splitKeyFunc(tt.key) | ||
_, err := controller.controllerMonitor.client.CrdV1beta1().AntreaAgentInfos().Get(ctx, name, metav1.GetOptions{}) | ||
assert.Equal(t, true, errortesting.IsNotFound(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.
this should be assert.True(t, errortesting.IsNotFound(err))
pkg/monitor/controller_test.go
Outdated
if tt.expectedAgentCRD == nil { | ||
_, name, _ := splitKeyFunc(tt.key) | ||
_, err := controller.controllerMonitor.client.CrdV1beta1().AntreaAgentInfos().Get(ctx, name, metav1.GetOptions{}) | ||
assert.Equal(t, true, errortesting.IsNotFound(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.
same as above
pkg/monitor/controller_test.go
Outdated
} else { | ||
require.NoError(t, err) | ||
_, err = controller.controllerMonitor.client.CrdV1beta1().AntreaAgentInfos().Get(ctx, "testAgentCRD", metav1.GetOptions{}) | ||
assert.Equal(t, "antreaagentinfos.crd.antrea.io \"testAgentCRD\" not found", err.Error()) |
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.
you can use assert.EqualError
actually, can't you use errortesting.IsNotFound
here as well?
pkg/monitor/controller_test.go
Outdated
defer close(stopCh) | ||
err := controller.controllerMonitor.deleteAgentCRD("testAgentCRD") | ||
if tt.expectedError != "" { | ||
assert.Equal(t, tt.expectedError, err.Error()) |
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.
you can use assert.EqualError
pkg/monitor/controller_test.go
Outdated
defer close(stopCh) | ||
err := controller.controllerMonitor.createAgentCRD("testAgentCRD") | ||
if tt.expectedError != "" { | ||
assert.Equal(t, tt.expectedError, err.Error()) |
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.
use assert.EqualError
pkg/monitor/controller_test.go
Outdated
clientset := fakeclientset.NewSimpleClientset() | ||
if tt.existingExternalNode != nil { | ||
clientset = fakeclientset.NewSimpleClientset(tt.existingExternalNode) | ||
} |
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 think it's better to only initialize the client set once, even if it's a bit more verbose:
var clientset *fakeclientset.Clientset
if tt.existingExternalNode != nil {
clientset = fakeclientset.NewSimpleClientset(tt.existingExternalNode)
} else {
clientset = fakeclientset.NewSimpleClientset()
}
applies to other tests as well
pkg/monitor/controller_test.go
Outdated
controller.controllerMonitor.deleteStaleAgentCRDs() | ||
crds, err := controller.controllerMonitor.client.CrdV1beta1().AntreaAgentInfos().List(ctx, metav1.ListOptions{ResourceVersion: "0"}) | ||
if tt.expectedError != "" { | ||
assert.Equal(t, tt.expectedError, err.Error()) |
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.
same as above
e22948b
to
16ebd4c
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.
LGTM
@luolanzone do you want to take another look at this before we merge? |
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.
LGTM overall, a few nits, @NamanAg30 please replace the year with 2023 since it's a new file. And double check all test cases' name format as well. thanks.
pkg/monitor/agent_test.go
Outdated
@@ -0,0 +1,153 @@ | |||
// Copyright 2022 Antrea Authors |
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.
s/2022/2023/
pkg/monitor/controller_test.go
Outdated
@@ -0,0 +1,626 @@ | |||
// Copyright 2022 Antrea Authors |
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.
ditto
pkg/monitor/controller_test.go
Outdated
expectedError: "", | ||
}, | ||
{ | ||
name: "key exists", |
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.
Keep the format consistent if you start it with upper case in other cases' name:
name: "key exists", | |
name: "Key exists", |
pkg/monitor/controller_test.go
Outdated
expectedError: "", | ||
}, | ||
{ | ||
name: "key exists", |
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.
Ditto
Signed-off-by: Naman Agarwal <[email protected]>
16ebd4c
to
45c2f6a
Compare
@antoninbas I have made the changes according to @luolanzone 's recommendations . When I pushed my code it automatically dismissed your review. You can check if the PR can be merged ,Thanks. |
/skip-all |
Signed-off-by: Naman Agarwal <[email protected]>
for #4142
Signed-off-by: Naman Agarwal [email protected]