parallel e2e testing with different kubeconfigs by bartam1 · Pull Request #1020 · banzaicloud/koperator

bartam1 · 2023-07-21T13:48:32Z

Description

New tests package with Test, TestCase, K8sCluster, TestPool, ClusterInfo structures are added
Add new method to handle tests with the help of tests package
Add test configuration
Calculated test duration
Using different test strategies
Create test report file
Ginkgo Labels can be used for selecting specific test(s) from TestPool
Add test information into ginkgo test header
Unit tests for test strategies
Downgrade k8s.io/apimachinery to the same version that is used in Koperator
Add mock tests
Using TestCase for Koperator all-test test case
Add common and configure as separate package
Tests capable to run parallel with different kubeconfigs
Makefile modifications to run tests with ginkgo (needed for parallelization)
Makefile modification to run e2e test framework unit tests whith "make test"
Makefile modification go fmt and go vet runs on tests/e2e module
Add test report github action
Add entry into test output about test progress "passed(TOTAL:45 PROGRESS/PROC:2/0/0 PID: 1)"

Type of Change

New Feature

Checklist

I have read the contributing guidelines
Existing issues have been referenced (where applicable)
I have verified this change is not present in other open pull requests
All code style checks pass
All new and existing tests pass

…n module

author marbarta <MARBARTA@cisco.com> 1690214601 +0200 committer marbarta <MARBARTA@cisco.com> 1690470420 +0200 create test report

panyuenlau

Didn't get to go through every single line just yet but went through most of them - looking sharp in general.

Also left some comments that are mostly about formatting

tests/e2e/test_alltestcases.go

tests/e2e/pkg/tests/tests.go

tests/e2e/pkg/tests/mocks.go

tests/e2e/helm.go

tests/e2e/test_alltestcases.go

Kuvesz

Left some comments, overall doesn't look too bad, it's just very confusing to me in some ways, I really was hoping a simpler solution is possible to achieve this.

Otherwise I would only ask that we try to refrain from these very long PRs (I know it's not always possible, this is just a reminder) when we can, I'm pretty sure I missed a lot of stuff in there because I can't pay attention long enough to review something like this properly.

.github/actions/kind-create/action.yaml

Kuvesz · 2023-08-01T09:09:56Z

.github/workflows/e2e-test.yaml

+          kind_k8s_version: v1.24.13
+          kubeconfig_dir_path: tests/e2e/kubeconfigs
+
+      # TODO: ZookeeperCluster create time out always on the second kind cluster


Question: Is it an actual timeout issue or can Zookeeper not start for some reason and we retry until we reach a timeout?

Here you can see the problem: https://github.com/banzaicloud/koperator/runs/15416787902
It would be nice to get more information why is this fail (like pod status)

Because in local environment all of the tests passed when multiple k8s cluster is used in parallel way I suspect that the problem is with timeouts or something else on github

Please test the PR with multicluster input on your local.
This PR is not about to run tests on github on multiple kind cluster.

.github/workflows/e2e-test.yaml

tests/e2e/const.go

tests/e2e/pkg/tests/mocks.go

tests/e2e/pkg/tests/tests.go

Kuvesz · 2023-08-01T09:41:51Z

tests/e2e/pkg/tests/tests_test.go

Is this WIP? A lot of unit tests seem to be missing from this still.

I made unit test for the complex function.
Can you see another function with complex logic that need to be unit tested?

What I mean is we only have unit tests here for one struct's functions and nothing for any of the other functions. I know not everything can be unit tested easily but it would be nice to add most we can do.

I added multiple unit tests for functions where there are some logic.

Kuvesz · 2023-08-07T09:26:51Z

tests/e2e/const.go

+	kafkaClusterCreateTimeout              = 600 * time.Second
 	kafkaClusterResourceCleanupTimeout     = 120 * time.Second
 	kcatDeleetionTimeout                   = 40 * time.Second
 	zookeeperClusterCreateTimeout          = 7 * time.Minute // Increased from 4 to 7 for multiple kind


Same as on line 56, I don't think we need this comment.

Can you please explain why should we remove this?

I'm really not sure what sense it makes to have the explanation for this timeout here. For giving information to reviewers it would be fine as a review comment but I don't think this needs to be a comment on the code.

+1 that the comment "// Increased from 4 to 7 for multiple kind" can be removed since it only provides the context about why the change is made, but it doesn't provide any valuable info itself along with the source code.

Or perhaps we can change the comment to something like this:

Suggested change

zookeeperClusterCreateTimeout = 7 * time.Minute // Increased from 4 to 7 for multiple kind

// increase timeout for supporting multiple Kind clusters

zookeeperClusterCreateTimeout = 7 * time.Minute

I can remove but in this case who will face with this issue on multiple kind cluster on github will not know what was the original setting for one kind cluster setup.

I think default values and the reason of their changes are valuable in this case.

Because the increased timeout was not helped, I put back to the original value and I removed the comment
fixed: 129e7fe

Kuvesz · 2023-08-07T09:32:17Z

tests/e2e/pkg/common/common.go

+	return kubecontext, nil
+}
+
+// GetRawConfig creates a raw clientcmd api config


I mean in my head a get function isn't supposed to create things, just get them but what do the others think? @panyuenlau @mihaialexandrescu @pregnor ?

.github/actions/kind-create/action.yaml

Kuvesz · 2023-08-07T09:39:44Z

tests/e2e/pkg/tests/tests_test.go

What I mean is we only have unit tests here for one struct's functions and nothing for any of the other functions. I know not everything can be unit tested easily but it would be nice to add most we can do.

.github/workflows/e2e-test.yaml

Kuvesz

Looks okay to me, though I still have some comments open and there are some TODOs so I'll not approve just yet.

required stuff has been fixed

panyuenlau

Generally LGTM, will approve when the un-resolved comments are addressed

panyuenlau

Thanks for the excellent work!

bartam1 changed the title ~~feat(e2e): parallel testing with different kubeconfigs~~ parallel e2e testing with different kubeconfigs Jul 21, 2023

bartam1 force-pushed the e2e_param branch from 3bbb1ea to 6de9f34 Compare July 25, 2023 09:21

bartam1 mentioned this pull request Jul 26, 2023

Same test executed multiple times in parallel on different processes and some of them not at all onsi/ginkgo#1242

Open

bartam1 added 5 commits July 27, 2023 17:04

feat(e2e): parallel testing with different kubeconfigs

3800d03

downgrade e2e k8s.io/apimachinery to the same ver that is used by mai…

1561b6a

…n module

add bin/ginkgo Makefile

d7947cc

parent d7947cc

d9efd7f

author marbarta <MARBARTA@cisco.com> 1690214601 +0200 committer marbarta <MARBARTA@cisco.com> 1690470420 +0200 create test report

update ginkgo to 2.11.0

ccc9a6d

bartam1 force-pushed the e2e_param branch from 7601b8d to ccc9a6d Compare July 27, 2023 15:10

Merge branch 'master' into e2e_param

f8a786d

bartam1 marked this pull request as ready for review July 27, 2023 15:16

bartam1 requested a review from a team as a code owner July 27, 2023 15:16

banzaicloud deleted a comment from github-actions bot Jul 27, 2023

bartam1 added 4 commits July 27, 2023 17:41

run allTestCase

56c0634

format time output

527e501

increase zookeepercluster and kafkacluster creation timeout

0f0e07e

run allTests serial

4826fa1

panyuenlau reviewed Jul 27, 2023

View reviewed changes

bartam1 and others added 5 commits July 28, 2023 04:49

zookeeper timeout increased

0764631

add test progress indicator

86586a4

report entry refactored

2df49bb

go with one kind cluster

cd250f5

Merge branch 'master' into e2e_param

6958dce

mihaialexandrescu reviewed Jul 31, 2023

View reviewed changes

tests/e2e/test_alltestcases.go Outdated Show resolved Hide resolved

refactor comments and K8sClusterPool

347d28d

Kuvesz previously requested changes Aug 1, 2023

View reviewed changes

bartam1 added 4 commits August 3, 2023 09:07

fix mockTests description

2b85e9f

fix: missing kubectlOptions param

b678b17

fix David suggestions 1

400c0ff

fix David suggestions 2

4c0b935

fix unnecessary imports

4031097

mihaialexandrescu mentioned this pull request Aug 3, 2023

e2e test for KafkaTopic webhook #1031

Draft

4 tasks

Kuvesz reviewed Aug 7, 2023

View reviewed changes

bartam1 and others added 3 commits August 8, 2023 13:33

fix: feedFromDirectory

edab06c

Merge branch 'master' into e2e_param

6f55555

add fix and test for version and provider identifier

98b4a0d

Kuvesz reviewed Aug 9, 2023

View reviewed changes

bartam1 added 2 commits August 9, 2023 17:23

add test for GetTestSuiteDurationParallel

8b72d6c

add test for testpool

5154223

panyuenlau reviewed Aug 11, 2023

View reviewed changes

bartam1 added 7 commits August 12, 2023 17:32

refactor classifier using NewTest constructor

bc10e92

fix classifier unit test

c1fd100

add e2e unit tests execution into Makefile

eb7c5a1

add e2e go fmt go vet

a2952f6

fix go.mod

95d6371

rename GetRawConfig to CreateRawConfig

50d3a81

zookeepecluster create timeout defaults 4min

129e7fe

panyuenlau previously approved these changes Aug 14, 2023

View reviewed changes

bartam1 added 2 commits August 16, 2023 11:21

test 2 kind cluster setup

7029681

default strategy versionComplete

7bc2a2d

bartam1 dismissed panyuenlau’s stale review via 7bc2a2d August 16, 2023 09:21

bartam1 and others added 4 commits August 16, 2023 11:57

Merge branch 'master' into e2e_param

4c7b111

go with parallel

842273f

increase zookeeper and kafka cluster creation timeout

ba66727

default pod readiness timeout increased to 60

a13805f

pregnor mentioned this pull request Aug 17, 2023

kafkauser: copy ca to secret #1028

Merged

7 tasks

bartam1 and others added 2 commits August 17, 2023 13:51

go with one kind cluster by default

a947c8c

Merge branch 'master' into e2e_param

18442f6

	zookeeperClusterCreateTimeout = 7 * time.Minute // Increased from 4 to 7 for multiple kind
	// increase timeout for supporting multiple Kind clusters
	zookeeperClusterCreateTimeout = 7 * time.Minute

Conversation

bartam1 commented Jul 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of Change

Checklist

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kuvesz left a comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

Uh oh!

Kuvesz left a comment

Choose a reason for hiding this comment

Uh oh!

panyuenlau left a comment

Choose a reason for hiding this comment

Uh oh!

panyuenlau left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

bartam1 commented Jul 21, 2023 •

edited

Loading

panyuenlau left a comment •

edited

Loading