Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

parallel e2e testing with different kubeconfigs #1020

Open
wants to merge 41 commits into
base: master
Choose a base branch
from
Open

Conversation

bartam1
Copy link
Contributor

@bartam1 bartam1 commented Jul 21, 2023

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

@bartam1 bartam1 changed the title feat(e2e): parallel testing with different kubeconfigs parallel e2e testing with different kubeconfigs Jul 21, 2023
@bartam1 bartam1 marked this pull request as ready for review July 27, 2023 15:16
@bartam1 bartam1 requested a review from a team as a code owner July 27, 2023 15:16
@banzaicloud banzaicloud deleted a comment from github-actions bot Jul 27, 2023
Copy link
Member

@panyuenlau panyuenlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
tests/e2e/pkg/tests/tests.go Show resolved Hide resolved
tests/e2e/pkg/tests/mocks.go Outdated Show resolved Hide resolved
tests/e2e/pkg/tests/mocks.go Outdated Show resolved Hide resolved
tests/e2e/pkg/tests/mocks.go Outdated Show resolved Hide resolved
tests/e2e/pkg/tests/mocks.go Outdated Show resolved Hide resolved
tests/e2e/pkg/tests/mocks.go Outdated Show resolved Hide resolved
tests/e2e/pkg/tests/mocks.go Outdated Show resolved Hide resolved
tests/e2e/helm.go Outdated Show resolved Hide resolved
Kuvesz
Kuvesz previously requested changes Aug 1, 2023
Copy link
Contributor

@Kuvesz Kuvesz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
kind_k8s_version: v1.24.13
kubeconfig_dir_path: tests/e2e/kubeconfigs

# TODO: ZookeeperCluster create time out always on the second kind cluster
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
tests/e2e/const.go Outdated Show resolved Hide resolved
tests/e2e/const.go Outdated Show resolved Hide resolved
tests/e2e/pkg/tests/mocks.go Outdated Show resolved Hide resolved
tests/e2e/pkg/tests/tests.go Outdated Show resolved Hide resolved
tests/e2e/pkg/tests/tests.go Show resolved Hide resolved
tests/e2e/pkg/tests/tests.go Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -53,7 +53,7 @@ const (
defaultTopicCreationWaitTime = 10 * time.Second
defaultUserCreationWaitTime = 10 * time.Second

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please explain why should we remove this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return kubecontext, nil
}

// GetRawConfig creates a raw clientcmd api config
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Show resolved Hide resolved
Copy link
Contributor

@Kuvesz Kuvesz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Kuvesz Kuvesz dismissed their stale review August 9, 2023 11:49

required stuff has been fixed

Copy link
Member

@panyuenlau panyuenlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

panyuenlau
panyuenlau previously approved these changes Aug 14, 2023
Copy link
Member

@panyuenlau panyuenlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the excellent work!

@pregnor pregnor mentioned this pull request Aug 17, 2023
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants