Test framework improvement, doing proper clean up in BeforeSuite(), AfterEach(), AfterSuite() #225

zijun726911 · 2023-04-24T18:21:45Z

Changes

Further Test Framework imporvement, add more properly resource clean up logic:
- In BeforeSuite() do CleanServiceNetworkVpcAssociationForCurrentK8sClusterVpc()
- In AfterEach() Clean up previous running test case created k8s resources, in case this test case failed-in-the-middle
- In AfterSuite (), DeleteAllFrameworkTracedServiceNetworks(), Clean all test cases created serviceNetwork's VPC association
Use a more generic interface client.Object to create HttpRoute, so that backendRefObjects could be either Service or Service Import for future use
Add a [load_env_variables.sh] to make golang IDE config easier (https://github.com/aws/aws-application-networking-k8s/compare/main...zijun726911:https-test?expand=1#diff-0921758188af1018497f76b4373865caa0295c375a72b2e7d64cf01d13146912)

Test

make e2etest pass

Ran 3 of 3 Specs in 808.077 seconds
SUCCESS! -- 3 Passed | 0 Failed | 0 Pending | 0 Skipped

Update removed https traffic test in the new revision now it only have 2 test case

Full log: https://gist.github.com/zijun726911/7c8ed592b02f4f9911632c8c6a439a3d

- Add service import and export traffic test - Further Test Framework imporvement - Bug fix for service import and export, when handling Kind: ServiceExport k8s resource, the latticestore should create create a IsServiceImport==true cache for it

zijun726911 · 2023-04-24T18:24:10Z

Closed the previous PR

#224

coveralls · 2023-04-24T18:25:41Z

Pull Request Test Coverage Report for Build 4789745461

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 31.824%

Totals
Change from base Build 4788183481:	0.0%
Covered Lines:	3169
Relevant Lines:	9958

💛 - Coveralls

zijun726911 · 2023-04-24T18:25:52Z

test/suites/integration/https_gateway_listener_test.go

+		clientDeployment, clientService := testFramework.NewHttpApp(test.HTTPAppOptions{Name: "client"})
+		serverDeployment, serverService := testFramework.NewHttpApp(test.HTTPAppOptions{Name: "https-gw-server"})
+		httproute := testFramework.NewPathMatchHttpRoute(gateway, []client.Object{serverService}, "https")
+


to pass "https" in the NewPathMatchHttpRoute(), it will refer the "https" section in the gateway listener

zijun726911 · 2023-04-24T18:36:41Z

Update: removed https traffic test in the new revision now it only have 2 test case

coveralls · 2023-04-24T18:38:26Z

Pull Request Test Coverage Report for Build 4812312590

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 31.824%

Totals
Change from base Build 4788183481:	0.0%
Covered Lines:	3169
Relevant Lines:	9958

💛 - Coveralls

liwenwu-amazon · 2023-04-24T23:03:41Z

scripts/load_env_variables.sh

Will this mess that folks(most of us) doing development on clouddesktop ?

Developer should run (or set it in GoLand manually) load_env_variables.sh manually, and no where refer it.

I have some comments in this script, if it is still confusing, I could remove this script

Today, Makefile takes care of all these environment variable setting. @ellistarn should we merge these into Makefile?

I'm not familiar with golang env vars, but I'd avoid any ide specific configuration. At the end of the day, your team needs to choose the development practices that work best for you. I setup what works for me, and how I run other OSS projects. Feel free to change things.

liwenwu-amazon · 2023-04-24T23:12:06Z

test/pkg/test/http_gateway.go

@@ -5,7 +5,7 @@ import (
 	"sigs.k8s.io/gateway-api/apis/v1beta1"
 )

-func (env *Framework) NewGateway() *v1beta1.Gateway {
+func (env *Framework) NewHttpGateway() *v1beta1.Gateway {


Is there a reason to make this? A gateway can have multiple listener(s), one for HTTP and one for HTTPs. And need a e2e test case to cover a single HTTPRoute can be accessed through HTTP and HTTPs

Maybe put 2 listeners in one GW is better, I will change to that one in the next revision

Spec: v1beta1.GatewaySpec{ GatewayClassName: "amazon-vpc-lattice", Listeners: []v1beta1.Listener{ { Name: "http", Protocol: v1beta1.HTTPProtocolType, Port: 80, }, { Name: "https", Protocol: v1beta1.HTTPSProtocolType, Port: 443, }, }, },

liwenwu-amazon · 2023-04-24T23:26:15Z

test/pkg/test/metadata.go

+		t.SetName(RandomName())
+	}
+	if t.GetNamespace() == "" {
+		t.SetNamespace("default")


Just a note, we also need to test things in non-default namespace, e.g.

httproute

gateway

zmingxi · 2023-04-25T00:06:07Z

Makefile

@@ -2,6 +2,8 @@ export KUBEBUILDER_ASSETS ?= ${HOME}/.kubebuilder/bin
 export CLUSTER_NAME ?= $(shell kubectl config view --minify -o jsonpath='{.clusters[].name}' | rev | cut -d"/" -f1 | rev | cut -d"." -f1)
 export CLUSTER_VPC_ID ?= $(shell aws eks describe-cluster --name $(CLUSTER_NAME) | jq -r ".cluster.resourcesVpcConfig.vpcId")
 export AWS_ACCOUNT_ID ?= $(shell aws sts get-caller-identity --query Account --output text)
+export GATEWAY_API_CONTROLLER_LOGLEVEL ?= dubug


typo: debug?

liwenwu-amazon · 2023-04-25T16:33:43Z

Makefile

@@ -2,6 +2,8 @@ export KUBEBUILDER_ASSETS ?= ${HOME}/.kubebuilder/bin
 export CLUSTER_NAME ?= $(shell kubectl config view --minify -o jsonpath='{.clusters[].name}' | rev | cut -d"/" -f1 | rev | cut -d"." -f1)
 export CLUSTER_VPC_ID ?= $(shell aws eks describe-cluster --name $(CLUSTER_NAME) | jq -r ".cluster.resourcesVpcConfig.vpcId")
 export AWS_ACCOUNT_ID ?= $(shell aws sts get-caller-identity --query Account --output text)
+export GATEWAY_API_CONTROLLER_LOGLEVEL ?= debug


remove these from Makefile

liwenwu-amazon · 2023-04-25T16:33:59Z

Makefile

@@ -2,6 +2,8 @@ export KUBEBUILDER_ASSETS ?= ${HOME}/.kubebuilder/bin
 export CLUSTER_NAME ?= $(shell kubectl config view --minify -o jsonpath='{.clusters[].name}' | rev | cut -d"/" -f1 | rev | cut -d"." -f1)
 export CLUSTER_VPC_ID ?= $(shell aws eks describe-cluster --name $(CLUSTER_NAME) | jq -r ".cluster.resourcesVpcConfig.vpcId")
 export AWS_ACCOUNT_ID ?= $(shell aws sts get-caller-identity --query Account --output text)
+export GATEWAY_API_CONTROLLER_LOGLEVEL ?= debug
+export REGION ?= us-west-2


remove these from Makefile

liwenwu-amazon · 2023-04-25T16:35:30Z

scripts/load_env_variables.sh

@@ -0,0 +1,46 @@
+#!/bin/sh


Can you update docs/developer.md and mentioned that this can be used for when u want to run it inside golang IDE locally

zijun726911 · 2023-04-25T16:46:01Z

test/pkg/test/framework.go

@@ -82,10 +83,20 @@ func NewFramework(ctx context.Context) *Framework {

 	SetDefaultEventuallyTimeout(180 * time.Second)
 	SetDefaultEventuallyPollingInterval(10 * time.Second)
-	AfterEach(func() { framework.ExpectToBeClean(ctx) })
+	BeforeSuite(func() {


Will change to:

BeforeEach((func(){ExpectToBeClean()})

and remove current BeforeSuite()

zijun726911 · 2023-04-25T16:53:01Z

test/pkg/test/framework.go

+		}
+		framework.TestCasesCreatedK8sResource = nil
+		framework.CleanServiceNetworkVpcAssociationForCurrentK8sClusterVpc()
+


In AfterEach do framework.CleanTestEnvironment(ctx) to clean all pevious test created k8s resource and vpcLattice resource

zijun726911 · 2023-04-26T03:23:04Z

New test result, make e2etest still passed https://gist.github.com/zijun726911/3240c8e9a6990db894237015a31753bb

Do CleanTestEnvironment() in AfterEach()

liwenwu-amazon · 2023-04-26T05:57:45Z

docs/developer.md

+Pass `FOCUS` environment variable to run some specific test cases based on filter condition:
+```bash
+export REGION=us-west-2
+export FOCUS="HTTPRoute should support multiple path matches" 


Please add a note on how to find out the this FOCUS string to a particular test. thanks

Ok, Added more description in the next revision

…n` in AfterEach

liwenwu-amazon · 2023-04-26T17:03:31Z

test/pkg/test/framework.go

+		Logger(ctx).Infof("Deleting k8s resource %s %s/%s", reflect.TypeOf(object), object.GetNamespace(), object.GetName())
+		env.Delete(ctx, object)
+		//Ignore resource-not-found error here, as the test case logic itself could already clear the resources
+	}

 	//Theoretically, Deleting all k8s resource by `env.ExpectDeleteAllToSucceed()`, will make controller delete all related VPC Lattice resource,
 	//but the controller is still developing in the progress and may leaking some vPCLattice resource, need to invoke vpcLattice api to double confirm and delete leaking resource.
 	env.DeleteAllFrameworkTracedServiceNetworks(ctx)


Let's NOT do these automatically. The test should failed and stopped if this is the case, and let developer debug why it is the case.

Ok, actually now no where invoke the CleanTestEnvironment(), we could remove this method, but maybe we could keep it in case we use it in the future?

liwenwu-amazon · 2023-04-26T17:07:42Z

test/pkg/test/framework.go

@@ -324,7 +320,25 @@ func (env *Framework) DeleteAllFrameworkTracedServiceNetworks(ctx aws.Context) {
 		}
 	}

-	//TODO: check and delete serviceNetwork VPC associations
+	var allServiceNetworkVpcAssociationIdsToBeDeleted []*string


Let's not use test code to cleanup the resource for now. Let developer debug why it is leaked and manually clean them up.

Also, if these are not cleanup, the test should stop

liwenwu-amazon · 2023-04-26T17:10:21Z

test/pkg/test/framework.go

 		podList := &v1.PodList{}
-		g.Expect(env.List(ctx, podList, client.MatchingLabels(deployment.Spec.Selector.MatchLabels))).To(Succeed())
+		expectedMatchingLabels := make(map[string]string, len(deployment.Spec.Selector.MatchLabels))


Is it possible these are the one causing my 2nd test stucked waiting for target?

liwenwu-amazon

LGTM. Thank you!

Zijun Wang added 4 commits April 23, 2023 15:36

Add header match test case

7496e54

put header_match_test and path_match_test in two different files

b78239c

- Add https traffic test

27ef1b8

- Add service import and export traffic test - Further Test Framework imporvement - Bug fix for service import and export, when handling Kind: ServiceExport k8s resource, the latticestore should create create a IsServiceImport==true cache for it

Change service Export unit test

e1eeb0b

zijun726911 requested a review from liwenwu-amazon April 24, 2023 18:21

zijun726911 commented Apr 24, 2023

View reviewed changes

Rollback target_group_synthesizer.go change

d9c26ab

zijun726911 force-pushed the https-test branch from 51849b9 to d9c26ab Compare April 24, 2023 18:34

liwenwu-amazon reviewed Apr 24, 2023

View reviewed changes

liwenwu-amazon requested a review from ellistarn April 24, 2023 23:15

liwenwu-amazon reviewed Apr 24, 2023

View reviewed changes

zijun726911 changed the title ~~Add Https Listener traffic test, Test framework improvement~~ Test framework improvement, doing proper clean up in BeforeSuite(), AfterEach(), AfterSuite() Apr 24, 2023

zmingxi reviewed Apr 25, 2023

View reviewed changes

Add another https listener in the gateway.go

f4d4e60

liwenwu-amazon reviewed Apr 25, 2023

View reviewed changes

zijun726911 force-pushed the https-test branch from b04b535 to a063f69 Compare April 26, 2023 03:17

zijun726911 commented Apr 26, 2023

View reviewed changes

Do ExpectToBeClean in BeforeEach()

344ff26

Do CleanTestEnvironment() in AfterEach()

zijun726911 force-pushed the https-test branch from a063f69 to 344ff26 Compare April 26, 2023 03:29

liwenwu-amazon reviewed Apr 26, 2023

View reviewed changes

Zijun Wang added 2 commits April 26, 2023 09:16

Add more FOCUS environment variable description, do `ExpectToBeClea…

0a6ba84

…n` in AfterEach

remove CleanServiceNetworkVpcAssociationForCurrentK8sClusterVpc

4e03a5e

liwenwu-amazon reviewed Apr 26, 2023

View reviewed changes

Zijun Wang added 2 commits April 26, 2023 11:46

Change back DiscoveryLabel to a non-random name

ea3a4c4

Change developer.md

23d79ee

liwenwu-amazon approved these changes Apr 26, 2023

View reviewed changes

zijun726911 merged commit 0a27c5b into aws:main Apr 26, 2023

zijun726911 deleted the https-test branch September 20, 2023 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test framework improvement, doing proper clean up in BeforeSuite(), AfterEach(), AfterSuite() #225

Test framework improvement, doing proper clean up in BeforeSuite(), AfterEach(), AfterSuite() #225

zijun726911 commented Apr 24, 2023 •

edited

Loading

zijun726911 commented Apr 24, 2023

coveralls commented Apr 24, 2023 •

edited

Loading

zijun726911 Apr 24, 2023

zijun726911 commented Apr 24, 2023

coveralls commented Apr 24, 2023 •

edited

Loading

liwenwu-amazon Apr 24, 2023

zijun726911 Apr 24, 2023

liwenwu-amazon Apr 24, 2023

ellistarn Apr 26, 2023

liwenwu-amazon Apr 24, 2023

zijun726911 Apr 24, 2023

liwenwu-amazon Apr 24, 2023

zmingxi Apr 25, 2023

liwenwu-amazon Apr 25, 2023

liwenwu-amazon Apr 25, 2023

liwenwu-amazon Apr 25, 2023

zijun726911 Apr 25, 2023

zijun726911 Apr 25, 2023

zijun726911 commented Apr 26, 2023

liwenwu-amazon Apr 26, 2023

zijun726911 Apr 26, 2023

liwenwu-amazon Apr 26, 2023

zijun726911 Apr 26, 2023

liwenwu-amazon Apr 26, 2023

liwenwu-amazon Apr 26, 2023

liwenwu-amazon left a comment

Test framework improvement, doing proper clean up in BeforeSuite(), AfterEach(), AfterSuite() #225

Test framework improvement, doing proper clean up in BeforeSuite(), AfterEach(), AfterSuite() #225

Conversation

zijun726911 commented Apr 24, 2023 • edited Loading

Changes

Test

zijun726911 commented Apr 24, 2023

coveralls commented Apr 24, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4789745461

💛 - Coveralls

Choose a reason for hiding this comment

zijun726911 commented Apr 24, 2023

coveralls commented Apr 24, 2023 • edited Loading

Pull Request Test Coverage Report for Build 4812312590

💛 - Coveralls

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zijun726911 commented Apr 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liwenwu-amazon left a comment

Choose a reason for hiding this comment

zijun726911 commented Apr 24, 2023 •

edited

Loading

coveralls commented Apr 24, 2023 •

edited

Loading

coveralls commented Apr 24, 2023 •

edited

Loading