test: add e2e tests for Target Group Policy CRD #434

scottlaiaws · 2023-10-12T17:10:09Z

What type of PR is this? test

Which issue does this PR fix: #379

What does this PR do / Why do we need it: add e2e tests for Target Group Policy CRD

If an issue # is not available please add repro steps and logs from aws-gateway-controller showing the issue:

Testing done on this change:

make e2e-test outputs:

Ran 38 of 38 Specs in 2715.256 seconds
SUCCESS! -- 38 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestIntegration (2716.14s)
PASS
ok      github.com/aws/aws-application-networking-k8s/test/suites/integration   2716.173s

Automation added to e2e: add test/suites/integration/target_group_policy_test.go

Will this PR introduce any new dependencies?: No

Will this break upgrades or downgrades. Has updating a running cluster been tested?: N/A

Does this PR introduce any user-facing change?: No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

coveralls · 2023-10-12T17:12:21Z

Pull Request Test Coverage Report for Build 6498774292

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 45.149%

Totals
Change from base Build 6489472856:	0.0%
Covered Lines:	3825
Relevant Lines:	8472

💛 - Coveralls

test/pkg/test/framework.go

zijun726911 · 2023-10-13T23:11:52Z

test/suites/integration/target_group_policy_test.go

+
+		testFramework.ExpectUpdated(ctx, policy)
+
+		httpsTG := testFramework.GetTargetGroupWithProtocol(ctx, service, "https", "http1")


Verify old tg is deleted?

I would rather not mix use cases. Let deletion check happens in another spec.

zijun726911 · 2023-10-13T23:18:52Z

test/suites/integration/target_group_policy_test.go

+		Expect(*httpsTG.Protocol).To(Equal(vpclattice.TargetGroupProtocolHttps))
+	})
+
+	It("Delete Target Group Policy reset health check config for HTTP and HTTP1 Target Group", func() {


That can be a better name : "Delete Target Group Policy will reset health check config to default value" ?

No, since for other type of Target Group (e.g. HTTPS or HTTP2), when we delete the Target Group Policy, the controller will create new Target group since those fields are create-only

zijun726911 · 2023-10-13T23:24:16Z

test/suites/integration/target_group_policy_test.go

+		Expect(updatedTG.Config.HealthCheck.Port).To(BeNil())
+	})
+
+	It("Delete Target Group Policy create HTTP and HTTP1 Target Group", func() {


A better name ? "Delete Target Group Policy fallback to default TargetGroupProtocol and TargetGroupProtocolVersion"

zijun726911 · 2023-10-13T23:26:33Z

test/suites/integration/target_group_policy_test.go

+		})
+
+		httpRoute = testFramework.NewHttpRoute(testGateway, service, service.Kind)
+


Do we really need to create httpRoute, deployment, service 3 times? can these 3 test cases reuse one httpRoute, deployment, service to save some time?

test/pkg/test/framework.go

mikhail-aws · 2023-10-14T01:57:06Z

test/suites/integration/target_group_policy_test.go

+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+	appsv1 "k8s.io/api/apps/v1"
+	v1 "k8s.io/api/core/v1"


mikhail-aws · 2023-10-14T01:59:02Z

test/suites/integration/target_group_policy_test.go

+
+		testFramework.ExpectUpdated(ctx, policy)
+
+		httpsTG := testFramework.GetTargetGroupWithProtocol(ctx, service, "https", "http1")


I would rather not mix use cases. Let deletion check happens in another spec.

mikhail-aws · 2023-10-14T02:00:38Z

test/suites/integration/target_group_policy_test.go

+		testFramework.ExpectUpdated(ctx, policy)
+
+		httpsTG := testFramework.GetTargetGroupWithProtocol(ctx, service, "https", "http1")
+


you can remove empty lines

mikhail-aws · 2023-10-14T02:01:55Z

test/suites/integration/target_group_policy_test.go

+		Expect(*updatedTG.Config.HealthCheck.Enabled).To(BeTrue())
+		Expect(*updatedTG.Config.HealthCheck.Path).To(Equal("/"))
+		Expect(*updatedTG.Config.HealthCheck.HealthCheckIntervalSeconds).To(BeEquivalentTo(30))
+		Expect(*updatedTG.Config.HealthCheck.HealthCheckTimeoutSeconds).To(BeEquivalentTo(5))
+		Expect(*updatedTG.Config.HealthCheck.HealthyThresholdCount).To(BeEquivalentTo(5))
+		Expect(*updatedTG.Config.HealthCheck.UnhealthyThresholdCount).To(BeEquivalentTo(2))
+		Expect(*updatedTG.Config.HealthCheck.Protocol).To(Equal(vpclattice.TargetGroupProtocolHttp))
+		Expect(*updatedTG.Config.HealthCheck.ProtocolVersion).To(Equal(vpclattice.TargetGroupProtocolVersionHttp1))
+		Expect(*updatedTG.Config.HealthCheck.Matcher.HttpCode).To(Equal("200"))
+		Expect(updatedTG.Config.HealthCheck.Port).To(BeNil())


Can you compare structs rather than each field of struct? Also when we reset health check config there should be some DefaultHealthCheckConfig struct in source code that we should compare with, otherwise we declare default behavior in 2 different places source and test.

Default Health Check config is different based on the Target Group Protocol (it's a private method that takes in TargetGroupProtocol)

ok, you can make it public, also dont compare each field of struct but compare structs

Since each field of a struct is a pointer, I can't use Equal to compare them.

Also for the default health check config method, it returned a struct with a "reset" value of 0 for each field instead the actual value, it's not possible to use that.

e.g. Our UpdateTargetGroup operation takes the following request:

{ "healthCheck": { "healthCheckIntervalSeconds": 0 } }

and then GetTargetGroup operation on the same target group will return

{ "healthCheck": { "healthCheckIntervalSeconds": 5 } }

Since each field of a struct is a pointer, I can't use Equal to compare them.

Are you sure? assert.Equals de-reference pointers. Works fine for me

type A struct { i int s string b *B } type B struct { i int s string } func TestA(t *testing.T) { a1 := &A{ i: 10, s: "10", b: &B{ i: 20, s: "20", }, } a2 := &A{ i: 10, s: "10", b: &B{ i: 20, s: "20", }, } assert.Equal(t, a1, a2) } m

I'm using gomega.Equal which uses reflect.DeepEqual under the hood, ~~and that doesn't dereference the pointers~~. But even if that issue is resolved, I still have the other issue of the GetDefaultHealthCheck config method returning different values than the GetTargetGroup call

Wait nvm, I just tested again, reflect.DeepEqual did dereference pointers.

reflect.DeepEqual de-reference pointers https://pkg.go.dev/reflect#DeepEqual

Pointer values are deeply equal if they are equal using Go's == operator or if they point to deeply equal values.

Code above also resolves into True, with reflect.DeepEqual(a1, a2)

mikhail-aws · 2023-10-14T02:06:57Z

test/suites/integration/target_group_policy_test.go

+
+		testFramework.ExpectDeletedThenNotFound(ctx, policy)
+
+		time.Sleep(2 * time.Second)


magic 2 seconds?

mikhail-aws · 2023-10-26T21:20:19Z

test/suites/integration/target_group_policy_test.go

+
+			tg := testFramework.GetFullTargetGroupFromSummary(ctx, tgSummary)
+
+			g.Expect(*tg.Config.HealthCheck).To(Equal(vpclattice.HealthCheckConfig{


scottlaiaws force-pushed the 379 branch from 6568cd0 to 5c269ad Compare October 12, 2023 17:28

zijun726911 reviewed Oct 13, 2023

View reviewed changes

mikhail-aws suggested changes Oct 14, 2023

View reviewed changes

scottlaiaws force-pushed the 379 branch from 5c269ad to 5780f6b Compare October 26, 2023 21:16

test: add e2e tests for Target Group Policy CRD

34eb39c

scottlaiaws force-pushed the 379 branch from 5780f6b to 34eb39c Compare October 26, 2023 21:19

mikhail-aws approved these changes Oct 26, 2023

View reviewed changes

scottlaiaws merged commit ad00db0 into aws:main Oct 26, 2023
5 checks passed

scottlaiaws deleted the 379 branch October 26, 2023 22:34

solmonk mentioned this pull request Nov 17, 2023

[More e2etest cases] TargetGroupPolicy test cases #379

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: add e2e tests for Target Group Policy CRD #434

test: add e2e tests for Target Group Policy CRD #434

scottlaiaws commented Oct 12, 2023

coveralls commented Oct 12, 2023 •

edited

Loading

zijun726911 Oct 13, 2023

mikhail-aws Oct 14, 2023

zijun726911 Oct 13, 2023

scottlaiaws Oct 16, 2023

zijun726911 Oct 13, 2023

zijun726911 Oct 13, 2023

mikhail-aws Oct 14, 2023

mikhail-aws Oct 14, 2023

mikhail-aws Oct 14, 2023

mikhail-aws Oct 14, 2023

scottlaiaws Oct 16, 2023

mikhail-aws Oct 18, 2023

scottlaiaws Oct 24, 2023

mikhail-aws Oct 25, 2023

scottlaiaws Oct 25, 2023 •

edited

Loading

mikhail-aws Oct 25, 2023

mikhail-aws Oct 14, 2023

mikhail-aws Oct 26, 2023


		testFramework.ExpectUpdated(ctx, policy)

		httpsTG := testFramework.GetTargetGroupWithProtocol(ctx, service, "https", "http1")

		})

		httpRoute = testFramework.NewHttpRoute(testGateway, service, service.Kind)


		testFramework.ExpectDeletedThenNotFound(ctx, policy)

		time.Sleep(2 * time.Second)


		tg := testFramework.GetFullTargetGroupFromSummary(ctx, tgSummary)

		g.Expect(*tg.Config.HealthCheck).To(Equal(vpclattice.HealthCheckConfig{

test: add e2e tests for Target Group Policy CRD #434

test: add e2e tests for Target Group Policy CRD #434

Conversation

scottlaiaws commented Oct 12, 2023

coveralls commented Oct 12, 2023 • edited Loading

Pull Request Test Coverage Report for Build 6498774292

💛 - 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

Choose a reason for hiding this comment

scottlaiaws Oct 25, 2023 • edited Loading

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

coveralls commented Oct 12, 2023 •

edited

Loading

scottlaiaws Oct 25, 2023 •

edited

Loading