Add IPAM unit tests && Using ginkgo replace the origin go test #508

DancingLinks · 2020-02-21T10:09:32Z

What this PR does / why we need it:

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

DancingLinks · 2020-03-03T08:56:10Z

@tamilmani1989, @matmerr, @jaer-tsun. Hi all, I'm an SWE Intern to ASK China team. My leader and mentor want me to be familiar with Azure CNI, and add some unit tests for it. So I mentioned this PR after learning the code of Azure CNI.

This PR mainly adds some unit tests of IPAM to ensure that each function can get the expected results. To make the test code look clearer, I used ginkgo as the unit test framework.

I hope you can spare time to review my PR. If there is any problem, I will correct it in time. You can also connect me on Teams, my alias is t-yuqian :)

At present, I am adding unit tests for the network package.

jaer-tsun · 2020-03-04T22:13:37Z

/azp run

azure-pipelines · 2020-03-04T22:13:48Z

Azure Pipelines successfully started running 1 pipeline(s).

jaer-tsun · 2020-03-04T22:16:32Z

ipam/null.go


 	// Set the local address space as active.
-	s.sink.setAddressSpace(local)
+	if err := s.sink.setAddressSpace(local); err != nil {


return s.sink.setAddressSpace(local)

jaer-tsun · 2020-03-04T22:21:45Z

cni/ipam/ipam_test.go

+	"github.com/Azure/azure-container-networking/platform"
+	cniSkel "github.com/containernetworking/cni/pkg/skel"
+	cniTypesCurr "github.com/containernetworking/cni/pkg/types/current"
+	. "github.com/onsi/ginkgo"


we don't use submodules in this repo so please vendor via go-dep

I'm now using go-dep instead, adding github.com/onsi/ginkgo, github.com/onsi/gomega and gopkg.in/fsnotify.v1

jaer-tsun · 2020-03-04T22:22:41Z

cni/ipam/ipam_test.go

-	testAgent, err := common.NewListener(u)
+func parseResult(stdinData []byte) (*cniTypesCurr.Result, error) {
+	result := &cniTypesCurr.Result{}
+	err := json.Unmarshal(stdinData, result)


for single returns, current style is to keep it on the same line

Get, thanks!

jaer-tsun · 2020-03-04T22:27:05Z

cni/ipam/ipam_test.go

+	_ = Describe("Test IPAM", func() {
+
+		var (
+			config common.PluginConfig


looks like linter didn't work here

I will move config to the Context where it works

cni/ipam/ipam_test.go

jaer-tsun · 2020-03-04T22:37:37Z

ipam/azure_test.go

+
+func parseResult(stdinData []byte) (*cniTypesCurr.Result, error) {
+	result := &cniTypesCurr.Result{}
+	err := json.Unmarshal(stdinData, result)


current style is to merge with if statement for single returns
e.g.

if err := json.Marshal...; err != nil {
...
}

OK, thanks!

jaer-tsun · 2020-03-04T22:40:38Z

ipam/fileIpam_test.go

+			Scope: scope,
+			Pools: make(map[string]*addressPool),
+		}, nil
 	} else {


no need for else block

I'll remove it

jaer-tsun · 2020-03-04T22:41:26Z

ipam/fileIpam_test.go

-	options[common.OptEnvironment] = common.OptEnvironmentFileIpam
-	fileIpam, _ := newFileIpamSource(options)
+type addressManagerMock struct {
+	newAddressSpaceSucc bool


type out Succ

jaer-tsun · 2020-03-04T22:41:49Z

ipam/fileIpam_test.go

+func (sink *addressManagerMock) setAddressSpace(*addressSpace) error {
+	if sink.setAddressSpaceSucc {
+		return nil
+	} else {


no need for else block

I'll remove it

ipam/manager_test.go

tamilmani1989

Thanks for the PR. Reviewed half. will review remaining later. One concern is that description of each test case. its little confusing if its pool or address

tamilmani1989 · 2020-03-05T19:11:44Z

cni/ipam/ipam_test.go

-	}
+func getStdinData(cniversion, ifname, subnet, ipAddress string) []byte {
+	stdinData := fmt.Sprintf(
+`{


space damage

tamilmani1989 · 2020-03-05T19:13:35Z

cni/ipam/ipam_test.go

-		fmt.Printf("Failed to create IPAM plugin, err:%v.\n", err)
-		return
-	}
+func getIfName() (string, error) {


why do we need this ? What this function expected to return? I think this is for pod interface name right?

Yes I copy this code from network.go, for getting the interface name of the test environment.

What about just using a fake interface name, say eth0, for simplicity? We'll not setup and configure an interface, anyway.

In cni/network/network.go 145, it will detect the in used interface name. For network.go get the same interface name as the test code, I added this function...

the problem is if there are multiple interfaces, if it returns lo adapter or docker0 bridge as 1st interface, then this will break. for simplicity you can keep it as eth0

Thank @DancingLinks for your clarification.

the problem is if there are multiple interfaces, if it returns lo adapter or docker0 bridge as 1st interface, then this will break. for simplicity you can keep it as eth0

After re-reading the code, @DancingLinks and I found that we do not have an interface check, so both lo and docker0 will still work. But in practise, lo and docker0 will not be picked for mac address mismatch.

Also, I found that interface name is not specified for the Kubernetes case[1]. How about we first cover the path without an interface name when invoking the IPAM plugin.
If we have to select a real interface name, how about removing lo and docker0 from the interface list and pick one randomly, in case we may have interface names other than eth0, in different OS releases? This seems to be more real.

[1]https://github.com/kubernetes/kubernetes/blob/581d3e26c97ade852b54bc7f102f1a4d16b74437/pkg/kubelet/dockershim/network/cni/cni.go#L353

tamilmani1989 · 2020-03-05T19:18:16Z

cni/ipam/ipam_test.go

+			var result *cniTypesCurr.Result
+
+			Context("When ADD with nothing (request pool)", func() {
+				It("Request pool and ADD successfully", func() {


is this for request pool or request address or for both?

Both. I found that this case may happen when request pool.

Then update description saying this is test for ipam add call

jaer-tsun · 2020-03-06T21:07:20Z

Gopkg.toml

 #   go-tests = true
 #   unused-packages = true

+[[override]]


what is this package for?

When I run dep ensure, an error occurred.
golang/dep#1799
cloudfoundry/disaster-recovery-acceptance-tests@983620d

is this signed off by lca?

jaer-tsun · 2020-03-13T18:01:22Z

/azp run

azure-pipelines · 2020-03-13T18:01:34Z

Azure Pipelines successfully started running 1 pipeline(s).

jaer-tsun

lgtm, waiting on @tamilmani1989 final review

lgtm

mainred

Thanks for your commits. Please remove Submodule.

mainred · 2020-03-30T06:47:21Z

ipam/azure_test.go

+			})
+		})
+	})
+)


We may need a newline here.

Ok, I'll add a newline here :) forgot

mainred · 2020-03-30T14:13:34Z

cni/ipam/ipam_test.go

-		fmt.Printf("Failed to create IPAM plugin, err:%v.\n", err)
-		return
-	}
+func getIfName() (string, error) {


What about just using a fake interface name, say eth0, for simplicity? We'll not setup and configure an interface, anyway.

cni/ipam/ipam_test.go

mainred · 2020-03-30T14:52:33Z

cni/ipam/ipam_test.go

+
+			Context("When address and subnet is given", func() {
+				It("ADD address successfully with the given address", func() {
+					arg.StdinData = getStdinData("0.4.0", ifName, "10.0.0.0/16", "10.0.0.6")


What about using another CIDR instead of the default one 10.0.0.0/16? So that we can test the returned IP address is not from the default CIDR.

cni/ipam/ipam_test.go

mainred · 2020-03-30T15:30:54Z

ipam/manager_test.go

 	"testing"
-
-	"github.com/Azure/azure-container-networking/common"
+	"time"


The following package groups sequence convention is very popular:

standard library

third-party packages

local packages
Could you please adjust your code to comply with this?

CC @tamilmani1989

my only concern with 3rd party is we have to get approval from lca before using that..did we get required approval?

Thanks so much for pointing this out. I am not sure about the newly imported packages in this PR.
I saw from our Golang files that we are using different package group conventions, so I here just want to share my idea to follow one convention.

ipam/null.go

mainred · 2020-03-31T03:43:23Z

cni/ipam/ipam_test.go

+			err error
+		)
+
+		BeforeSuite(func() {


Please correct me if I am wrong, BeforeSuite and AfterSuite is suitable to be outside of the Describe clause as suite-scoped resources.

I'll move them outside of the Describe, thx!

cni/ipam/ipam_test.go

tamilmani1989

Thanks for this PR. Really appreciate the effort. Left some comments

tamilmani1989 · 2020-03-31T08:11:54Z

Gopkg.toml

 #   go-tests = true
 #   unused-packages = true

+[[override]]


is this signed off by lca?

tamilmani1989 · 2020-03-31T08:12:55Z

cni/ipam/ipam_test.go

+	cniSkel "github.com/containernetworking/cni/pkg/skel"
+	cniTypesCurr "github.com/containernetworking/cni/pkg/types/current"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"


are these 3rd party packages signed by microsoft legal (lca)

I am not sure about this, but we have these two packages used in repos like aks-engine already.
https://github.com/search?q=org%3AAzure+github.com%2Fonsi%2Fginkgo&type=Code

tamilmani1989 · 2020-03-31T08:15:23Z

cni/ipam/ipam_test.go

-		fmt.Printf("Failed to create IPAM plugin, err:%v.\n", err)
-		return
-	}
+func getIfName() (string, error) {


the problem is if there are multiple interfaces, if it returns lo adapter or docker0 bridge as 1st interface, then this will break. for simplicity you can keep it as eth0

tamilmani1989 · 2020-03-31T08:21:00Z

cni/ipam/ipam_test.go

+			var result *cniTypesCurr.Result
+
+			Context("When ADD with nothing (request pool)", func() {
+				It("Request pool and ADD successfully", func() {


Then update description saying this is test for ipam add call

cni/ipam/ipam_test.go

tamilmani1989 · 2020-03-31T08:29:31Z

ipam/azure_test.go

+				})
+			})
+
+			Context("When time interval is less", func() {


couldnot understand what this test for..what we are testing here

Neither can I. Let's remove this test unless you can make it more descriptive and please also remember test will also be maintained as part of our code.

I will change the description as "refresh interval is too short". It may be more clearly now.

tamilmani1989 · 2020-03-31T08:30:15Z

ipam/azure_test.go

+
+			Context("When newAddressSpace err", func() {
+				It("Exit with error when refresh", func() {
+					sink := &addressManagerMock{false, true}


what this mock does..can you add a comment ..is addressmanagermock defined somewhere

tamilmani1989 · 2020-03-31T08:32:58Z

ipam/manager_test.go

 	"testing"
-
-	"github.com/Azure/azure-container-networking/common"
+	"time"


my only concern with 3rd party is we have to get approval from lca before using that..did we get required approval?

tamilmani1989 · 2020-03-31T08:38:58Z

ipam/manager_test.go

+						RefCount: 	1,
+						Addresses: 	make(map[string]*addressRecord),
+					}
+					ap.Addresses["ar-test"] = &addressRecord{


can we also add some addr and check if addrbyid returns that address...thats the main purpose of addrsbyid

tamilmani1989 · 2020-03-31T08:41:03Z

ipam/manager_test.go

-		t.Errorf("ReleasePool failed, err:%v", err)
-	}
-}
+var (


was unit test for requestpool, requestaddr, releaseddr, releasepool moved somewhere else?

mainred

LGTM from my part, but please let @tamilmani1989 make the decision.

tamilmani1989 · 2020-04-06T20:38:44Z

@DancingLinks Can you rebase and push it again?

mainred · 2020-04-07T01:42:19Z

@DancingLinks, besides rebasing your code, could you please also squash your commits and reword your commit messages to explain your changes neatly before we merge this PR?

DancingLinks · 2020-04-07T05:16:03Z

@tamilmani1989 rebased finished :)

DancingLinks · 2020-04-07T05:17:20Z

@mainred Thanks a lot for your suggestion! I've now merged them in a single commit.

tamilmani1989 · 2020-04-07T18:33:27Z

/azp run

azure-pipelines · 2020-04-07T18:33:38Z

Azure Pipelines successfully started running 1 pipeline(s).

2. Using ginkgo instead of the origin go test

tamilmani1989 · 2020-04-09T03:24:35Z

/azp run

azure-pipelines · 2020-04-09T03:24:47Z

Azure Pipelines successfully started running 1 pipeline(s).

tamilmani1989

lgtm

jaer-tsun requested review from jaer-tsun, matmerr and tamilmani1989 February 28, 2020 21:59

jaer-tsun previously requested changes Mar 4, 2020

View reviewed changes

tamilmani1989 requested changes Mar 5, 2020

View reviewed changes

jaer-tsun reviewed Mar 6, 2020

View reviewed changes

jaer-tsun requested a review from tamilmani1989 March 11, 2020 00:25

jaer-tsun reviewed Mar 17, 2020

View reviewed changes

mainred reviewed Mar 30, 2020

View reviewed changes

mainred reviewed Mar 31, 2020

View reviewed changes

tamilmani1989 requested changes Mar 31, 2020

View reviewed changes

tamilmani1989 self-requested a review March 31, 2020 08:47

mainred previously approved these changes Mar 31, 2020

View reviewed changes

DancingLinks dismissed mainred’s stale review via 00447e2 April 1, 2020 08:47

DancingLinks force-pushed the ipamtest branch from 62cd9b9 to 59655b9 Compare April 7, 2020 05:08

1. Add IPAM unit tests

d5d77cd

2. Using ginkgo instead of the origin go test

DancingLinks force-pushed the ipamtest branch from 2c7310c to d5d77cd Compare April 8, 2020 04:49

fix go mod

6c7abed

tamilmani1989 approved these changes Apr 9, 2020

View reviewed changes

tamilmani1989 merged commit 512ffed into Azure:master Apr 9, 2020

DancingLinks deleted the ipamtest branch April 9, 2020 08:14

+              			})
+              		})
+              	})
+              )

                
                    No newline at end of file

Add IPAM unit tests && Using ginkgo replace the origin go test #508

Add IPAM unit tests && Using ginkgo replace the origin go test #508

Uh oh!

Conversation

DancingLinks commented Feb 21, 2020

Uh oh!

DancingLinks commented Mar 3, 2020

Uh oh!

jaer-tsun commented Mar 4, 2020

Uh oh!

azure-pipelines bot commented Mar 4, 2020

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!

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!

Uh oh!

tamilmani1989 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!

mainred Apr 1, 2020 • 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!

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!

jaer-tsun commented Mar 13, 2020

Uh oh!

azure-pipelines bot commented Mar 13, 2020

mainred Apr 1, 2020 •

edited

Loading