Improving and adding CNI unit tests #543

DancingLinks · 2020-04-10T05:49:37Z

Add ipam/pool_test.go
Add network/*test.go
Using ginkgo test ipv6ipam

mainred

Left some comments, and I need time for another look later.

mainred · 2020-04-10T07:37:16Z

ipam/pool_test.go

+			})
+
+			Context("Create a new pool ID when the string is too long", func() {
+				It("Should create a pool ID by parsing the string", func() {


should raise an error?

Yes, sorry my mistake

mainred · 2020-04-10T08:06:12Z

ipam/pool_test.go

+				})
+			})
+
+			Context("", func() {


We may forget the context.

it should be "When pool's epoch is less than the space's epoch and the pool is never in use"

mainred · 2020-04-10T08:07:22Z

network/network_test.go

+			})
+		})
+	})
+)


lost a newline.

added, thx :)

mainred · 2020-04-10T08:26:22Z

ipam/pool_test.go

+						Id:     poolId,
+						IsIPv6: false,
+					}
+					ap, err := as.requestPool("", "", nil, true)


Suggested change

ap, err := as.requestPool("", "", nil, true)

ap, err := as.requestPool(poolId, "", nil, true)

This test case is to test requesting an ipv6 pool from an address space which only contain ipv4 pool, by only not assign the pool id it will enter the IP version matching "if branch".

mainred · 2020-04-10T09:04:06Z

ipam/pool_test.go

+				})
+			})
+
+			Context("When as has pools with different addresses", func() {


What about using the struct name addressSpace or addressSpace object instead of the variable name in a context, so that we can understand from the plain text context better? we can discuss here.

Suggested change

Context("When as has pools with different addresses", func() {

Context("When a addressSpace has pools with different number of addresses", func() {

If you agree, please also edit all other affected cases.

agree, I also changed the ar to addressRecord

mainred · 2020-04-10T09:10:01Z

ipam/pool_test.go

+						Id:        "pool2",
+						Addresses: map[string]*addressRecord{},
+					}
+					as.Pools["pool2"].Addresses["ar"] = &addressRecord{}


What about we assign values from real cases, so that:

it may detect more errors or better safeguard our code by walking through the code with real values.

it can help people understand the structure of the code even without running env.

Thx, very helpful suggestion!

mainred · 2020-04-10T09:10:33Z

ipam/pool_test.go

+
+		Describe("Test releasePool", func() {
+			Context("When pool not found", func() {
+				It("Should return error", func() {


Suggested change

It("Should return error", func() {

It("Should return an error", func() {

"Should raise an error?" seems better?

mainred · 2020-04-10T09:15:07Z

ipam/pool_test.go

+			})
+
+			Context("", func() {
+				It("", func() {


We may forget the expectation.

thx, it should be "When pool's epoch is less than the space's epoch and pool is never in use"

mainred · 2020-04-10T09:20:13Z

ipam/pool_test.go

+				})
+			})
+
+			Context("When ar is ubhealthy", func() {


Suggested change

Context("When ar is ubhealthy", func() {

Context("When an addressPool is unhealthy", func() {

maybe "When addressRecords are unhealthy" ?

mainred · 2020-04-13T01:31:51Z

ipam/ipv6Ipam_test.go

+					if runtime.GOOS == windows {
+						kubeConfigPath = defaultWindowsKubeConfigFilePath
+					} else {
+						kubeConfigPath = defaultLinuxKubeConfigFilePath
+					}


The Less, the tidy, and define kubeConfigPath when being used here.

Suggested change

if runtime.GOOS == windows {

kubeConfigPath = defaultWindowsKubeConfigFilePath

} else {

kubeConfigPath = defaultLinuxKubeConfigFilePath

}

kubeConfigPath := defaultLinuxKubeConfigFilePath

if runtime.GOOS == windows {

kubeConfigPath = defaultWindowsKubeConfigFilePath

}

mainred · 2020-04-13T01:34:27Z

ipam/ipv6Ipam_test.go

-		},
-	}
+			Context("When node doesn't have IPv6 subnet", func() {
+				It("Expected to fail IPv6 IP retrieval", func() {


Suggested change

It("Expected to fail IPv6 IP retrieval", func() {

It("Should fail to retrieve the IPv6 address", func() {

mainred · 2020-04-13T01:42:09Z

network/endpoint_test.go

+		})
+
+		Describe("Test updateEndpoint", func() {
+			Context("", func() {


We lost something.

Thx, I forget adding this test case.

mainred · 2020-04-13T01:45:01Z

network/endpoint_test.go

+					testData := map[string]string{
+						"nginx-deployment-5c689d88bb":       "nginx",
+						"nginx-deployment-5c689d88bb-qwq47": "nginx-deployment",
+						"nginx": "nginx",
+					}


Suggested change

testData := map[string]string{

"nginx-deployment-5c689d88bb": "nginx",

"nginx-deployment-5c689d88bb-qwq47": "nginx-deployment",

"nginx": "nginx",

}

testData := map[string]string{

"nginx-deployment-5c689d88bb": "nginx",

"nginx-deployment-5c689d88bb-qwq47": "nginx-deployment",

"nginx" : "nginx",

}

I guess you need a lint tool. Could you please run gofmt or more in the first place?

This is copied from the origin test cases, I'll format it

seems like we need a lint check in CI.

mainred

Thanks for your effort, and the values borrowed from real cases impressed me a lot.

mainred · 2020-04-13T08:24:55Z

ipam/pool_test.go

+				})
+			})
+
+			Context("When IfName is not match", func() {


Suggested change

Context("When IfName is not match", func() {

Context("When the requested interface name does not exist", func() {

can you resolve this also?

DancingLinks · 2020-04-13T08:38:48Z

/azp run

azure-pipelines · 2020-04-13T08:38:54Z

Commenter does not have sufficient privileges for PR 543 in repo Azure/azure-container-networking

tamilmani1989

Thank you so much for this PR. Appreciate your effort. Left some comments

tamilmani1989 · 2020-05-11T00:24:21Z

ipam/ipv6Ipam_test.go

+
+		Describe("Test newIPv6IpamSource", func() {
+
+			Context("When using windows environment", func() {


why context is windows env?

It should be "When creating with current environment", thx

ipam/ipv6Ipam_test.go

tamilmani1989 · 2020-05-11T00:31:19Z

ipam/manager_ipv6Ipam_test.go

 )

+func TestManagerIpv6Ipam(t *testing.T) {
+	RegisterFailHandler(Fail)


where these functions are defined? can you add comment as well about these functions and what it does

Defined in pkg Gpmega. RegisterFailHandler connects Ginkgo to Gomega. When a matcher fails the fail handler passed into RegisterFailHandler is called.

tamilmani1989 · 2020-05-11T00:33:58Z

ipam/manager_ipv6Ipam_test.go

+				})
+			})
+
+			Context("When test with a specified address", func() {


test without requesting address explicitly

tamilmani1989 · 2020-05-11T00:39:38Z

ipam/pool_test.go

+				})
+			})
+
+			Context("Create a new pool ID when the string is too long", func() {


Create a new pool ID when string format is incorrect

Changed, thx

tamilmani1989 · 2020-05-11T01:08:03Z

network/endpoint_test.go

+				})
+			})
+
+			Context("When SandboxKey now in use", func() {


Suggested change

Context("When SandboxKey now in use", func() {

Context("When SandboxKey not in use", func() {

tamilmani1989 · 2020-05-11T01:10:43Z

network/network_test.go

+	_ = Describe("Test Network", func() {
+
+		Describe("Test newExternalInterface", func() {
+			Context("When external interface alread exists", func() {


Suggested change

Context("When external interface alread exists", func() {

Context("When external interface already exists", func() {

tamilmani1989 · 2020-05-11T01:11:12Z

network/manager_test.go

+					Expect(nm.TimeStamp).To(Equal(time.Time{}))
+				})
+			})
+			Context("When store.Write error", func() {


Suggested change

Context("When store.Write error", func() {

Context("When store.Write returns error", func() {

tamilmani1989 · 2020-05-11T01:13:24Z

network/manager_test.go

+				})
+			})
+
+			Context("When ifName is empty", func() {


couldnt understand this? what you mean by ifName is empty? Can you add a proper description of this test case

"When ifName is not specified"

can you update description saying "When ifName not specifed in GetNumberofEndpoints"

tamilmani1989 · 2020-05-11T01:15:24Z

network/network_test.go

+
+		Describe("Test newExternalInterface", func() {
+			Context("When external interface alread exists", func() {
+				It("Should return nil", func() {


have to check if nm.ExternalInterfaces is updated with "subnet" right?

Yes, very good suggestion!

tamilmani1989

i see some comments are unaddressed still..address those

tamilmani1989 · 2020-05-11T20:18:55Z

ipam/pool_test.go

+				})
+			})
+
+			Context("When IfName is not match", func() {


can you resolve this also?

ipam/pool_test.go

tamilmani1989 · 2020-05-11T20:21:06Z

ipam/pool_test.go

+				})
+			})
+
+			Context("When pool's epoch is less than the space's epoch and pool is never in use", func() {


RefCount: 1 so it mean pool in use by one entity and should be deleted. If pool not in use, delete should fail

tamilmani1989 · 2020-05-11T20:24:02Z

network/manager_test.go

+				})
+			})
+
+			Context("When ifName is empty", func() {


can you update description saying "When ifName not specifed in GetNumberofEndpoints"

matmerr · 2020-05-11T22:35:31Z

/azp run

azure-pipelines · 2020-05-11T22:35:42Z

Azure Pipelines successfully started running 1 pipeline(s).

codecov · 2020-05-11T22:41:30Z

Codecov Report

Merging #543 into master will decrease coverage by 10.25%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master     #543       +/-   ##
===========================================
- Coverage   49.22%   38.97%   -10.26%     
===========================================
  Files          28       42       +14     
  Lines        3437     5047     +1610     
===========================================
+ Hits         1692     1967      +275     
- Misses       1455     2811     +1356     
+ Partials      290      269       -21

tamilmani1989 · 2020-05-27T00:12:38Z

/azp run

azure-pipelines · 2020-05-27T00:12:49Z

Azure Pipelines successfully started running 1 pipeline(s).

tamilmani1989

lgtm

* Add ipam/pool_test.go & add network/*test.go * add testing ./network/ in Makefile * fix context

mainred reviewed Apr 10, 2020

View reviewed changes

mainred reviewed Apr 13, 2020

View reviewed changes

DancingLinks force-pushed the networktest branch from ec4095f to 951ad59 Compare April 14, 2020 03:49

matmerr assigned tamilmani1989 and matmerr Apr 15, 2020

tamilmani1989 changed the title ~~Network test~~ Improving and adding CNI unit tests Apr 16, 2020

tamilmani1989 requested changes May 11, 2020

View reviewed changes

DancingLinks force-pushed the networktest branch from 551e2de to 975745d Compare May 11, 2020 11:56

tamilmani1989 requested changes May 11, 2020

View reviewed changes

Add ipam/pool_test.go & add network/*test.go

fc9171f

DancingLinks force-pushed the networktest branch from 975745d to fc9171f Compare May 13, 2020 07:17

DancingLinks added 2 commits May 19, 2020 14:50

add testing ./network/ in Makefile

a62330f

fix context

05955e7

tamilmani1989 approved these changes Jun 26, 2020

View reviewed changes

tamilmani1989 merged commit 7b5e817 into Azure:master Jun 26, 2020

pjohnst5 pushed a commit to pjohnst5/azure-container-networking that referenced this pull request Jul 9, 2020

Improving and adding CNI unit tests (Azure#543)

a22f0cc

* Add ipam/pool_test.go & add network/*test.go * add testing ./network/ in Makefile * fix context

+              			})
+              		})
+              	})
+              )

                
                    No newline at end of file

	ap, err := as.requestPool("", "", nil, true)
	ap, err := as.requestPool(poolId, "", nil, true)

	Context("When as has pools with different addresses", func() {
	Context("When a addressSpace has pools with different number of addresses", func() {

	It("Should return error", func() {
	It("Should return an error", func() {

	Context("When ar is ubhealthy", func() {
	Context("When an addressPool is unhealthy", func() {

	It("Expected to fail IPv6 IP retrieval", func() {
	It("Should fail to retrieve the IPv6 address", func() {

	Context("When IfName is not match", func() {
	Context("When the requested interface name does not exist", func() {


		Describe("Test newIPv6IpamSource", func() {

		Context("When using windows environment", func() {

	Context("When SandboxKey now in use", func() {
	Context("When SandboxKey not in use", func() {

	Context("When external interface alread exists", func() {
	Context("When external interface already exists", func() {

	Context("When store.Write error", func() {
	Context("When store.Write returns error", func() {

Improving and adding CNI unit tests #543

Improving and adding CNI unit tests #543

Uh oh!

Conversation

DancingLinks commented Apr 10, 2020

Uh oh!

mainred left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mainred left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DancingLinks commented Apr 13, 2020

Uh oh!

azure-pipelines bot commented Apr 13, 2020

mainred left a comment •

edited

Loading