-
Notifications
You must be signed in to change notification settings - Fork 8
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
tests, zone manager: Adding unit tests for zone manager component #43
Conversation
/release-note-none |
pkg/zonemgr/zone_file_cache_test.go
Outdated
aRecord_nic4_vm1_ns2 = fmt.Sprintf(aRecordFmt, nic4Name, vmi1Name, vmi2Namespace, nic4IP) | ||
) | ||
|
||
validateUpdateFunc := func(newVmiName, newVmiNamespace string, newInterfaces []v1.VirtualMachineInstanceNetworkInterface, |
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 remove the new
prefix from the variable names. It is not necessary new, maybe an existing VMI is updated.
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.
done
pkg/zonemgr/zone_file_cache_test.go
Outdated
When("interfaces records list is empty", func() { | ||
BeforeEach(func() { | ||
zoneFileCache = NewZoneFileCache(nameServerIP, domain) | ||
zoneFileCache.init() |
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 don't invoke private methods from the test.
Maybe it make sense to invoke the init
directly from NewZoneFileCache
.
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.
done
also renamed init() function name
pkg/zonemgr/zone_file_cache_test.go
Outdated
vmi1Name, | ||
vmi1Namespace, | ||
[]v1.VirtualMachineInstanceNetworkInterface{{IPs: []string{nic1IP}, Name: nic1Name}}, | ||
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.
please use a const for Updated, NotUpdated
to make the test more readable.
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.
done
pkg/zonemgr/zone_file_cache_test.go
Outdated
const ( | ||
vmi1Name = "vmi1" | ||
vmi2Name = "vmi2" | ||
vmi1Namespace = "ns1" |
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.
since the namespace is used for both vm1 and vm2 I would change the names to namespace1
and namespace2
.
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.
done
pkg/zonemgr/zone_file_test.go
Outdated
Expect(os.RemoveAll("zones")).To(Succeed()) | ||
}) | ||
|
||
testFileFunc := func(fileContent string) { |
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 change fileContent
to expectedFileContent
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.
done
pkg/zonemgr/zone_manager_test.go
Outdated
zoneMgr = NewZoneManager() | ||
}) | ||
|
||
AfterEach(func() { |
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.
There is no need to add an empty AfterEach
.
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.
done
pkg/zonemgr/zone_manager_test.go
Outdated
|
||
Context("Initialization", func() { | ||
It("should validate input data", func() { | ||
Expect(zoneMgr.UpdateZone(k8stypes.NamespacedName{Name: "vm1"}, nil)).NotTo(Succeed()) |
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 split into two tests. should fail updating a VMI with no name
and should fail updating a VMI with no namespace
.
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.
done
pkg/zonemgr/zone_manager_test.go
Outdated
}) | ||
|
||
It("should set custom data", func() { | ||
Expect(zoneMgr.UpdateZone(k8stypes.NamespacedName{Namespace: "ns1", Name: "vm1"}, nil)).To(Succeed()) |
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.
UpdateZone
is updating the domain
and nameServerIP
? I would expect NewZoneManager
to do 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.
NewZoneManager indeed updates domain and nameServerIP. I use UpdateZone just to check that values are fetched and populated correctly.
I will change the test to be more clear
pkg/zonemgr/zone_manager_test.go
Outdated
|
||
When("zone file already exist", func() { | ||
It("should update cached SOA serial value", func() { | ||
//todo |
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.
TODO can be filtered by the IDE.
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.
done
d0f9205
to
eb3f5c2
Compare
Signed-off-by: Diana Teplits <dteplits@redhat.com>
Signed-off-by: Diana Teplits <dteplits@redhat.com>
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: AlonaKaplan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
Unit tests added for:
Signed-off-by: Diana Teplits dteplits@redhat.com