ASC-393 Refactoring of proxy_test.go

[jkopriva]

This PR about refactoring proxy_test.go file and splitting it in more files to make tests readable.

Parallelization of proxy tests will be part of next PRs.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jkopriva
Once this PR has been reviewed and has the lgtm label, please assign alexeykazakov for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[xcoulon]

contents looks good overall! I just left a few comments about the form

[rajivnathan]

Thanks, these are great changes! They look good overall. I've left a few comments about the code but I have some general questions/comment as well:

1. If I understand the changes correctly, it's mainly taking everything that was in proxy_test.go and splitting it into separate files and tests and making the tests run in parallel. So shouldn't we delete the proxy_test.go file as part of this PR?

2. I think we would need to move the test files from test/e2e/proxy to test/e2e/parallel/proxy in order for them to run in parallel since only tests under the test/e2e/parallel dir are run in parallel. See https://github.com/codeready-toolchain/toolchain-e2e/blob/2d619cfee45849678d9691a1ce95e6f3b8386ffb/make/test.mk#L119C186-L119C186

3. Would it be better to rename the testsupport/appstudio package to testsupport/proxy? I think those utilities have more to do with the proxy than appstudio.

[MatousJobanek]

Hey @jkopriva, thanks a lot for your PR.
I haven't reviewed the content of the PR yet, just two thoughts before doing that:

Test execution
You created a new package/directory - make sure that the tests in the directory are being executed. I don't see them in the build log. My guess is that you need to modify the makefile target so it runs tests also in sub-packages. Correct me if I'm wrong

PR description
Please add some description to the PR so we understand what you have done here, why, and what is the plan of using it in the future PRs, so we can review that the intended changes are done. For example, Rajiv mentioned:

If I understand the changes correctly, it's mainly taking everything that was in proxy_test.go and splitting it into separate files and tests and making the tests run in parallel.
...
I think we would need to move the test files from test/e2e/proxy to test/e2e/parallel/proxy in order for them to run in parallel since only tests under the test/e2e/parallel dir are run in parallel. See https://github.com/codeready-toolchain/toolchain-e2e/blob/2d619cfee45849678d9691a1ce95e6f3b8386ffb/make/test.mk#L119C186-L119C186

But I don't see any parallelization there yet, or am I missing something?

[jkopriva]

Hi @rajivnathan, Thank you for review!
ad 1. Its deleted now(it get back during rebase).
ad 2. I will add the parallelization in next PR(s). This was mostly for refactoring the proxy_test.go file to have it more readable.
ad 3. Moved/renamed.

[jkopriva]

@MatousJobanek Hi, thank you for comments.
Ad Tests execution
You are correct with I have updated the make file to also include "new" proxy tests.
(I was thinking maybe revisiting make files could be done also in future)
AD PR description
You are correct, I should have added better PR description and what is the plan, I will do it in next PRs.
(I have updated the description in this PR)

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

[MatousJobanek]

overall looks good - If I understand the changes correctly, it's just about moving the tests and functions around and separating them.
I'm slightly missing the point of introducing a new package and moving the tests there and I'm also missing your plans for introducing parallelization. Could you please add a topic to the next sync call so we can discuss it?

[MatousJobanek]

you could do this instead, right?

Suggested change

	$(MAKE) execute-tests MEMBER_NS=${MEMBER_NS} MEMBER_NS_2=${MEMBER_NS_2} HOST_NS=${HOST_NS} REGISTRATION_SERVICE_NS=${REGISTRATION_SERVICE_NS} TESTS_TO_EXECUTE="./test/e2e ./test/e2e/proxy ./test/metrics"
	$(MAKE) execute-tests MEMBER_NS=${MEMBER_NS} MEMBER_NS_2=${MEMBER_NS_2} HOST_NS=${HOST_NS} REGISTRATION_SERVICE_NS=${REGISTRATION_SERVICE_NS} TESTS_TO_EXECUTE="./test/e2e/... ./test/metrics"

but is there really a benefit of moving it to a separate package?

[MatousJobanek]

could you please shed more light on how you want to introduce the parallelization of the tests when you change the global configuration?

[jkopriva]

This will be deleted in future - here it is just to avoid duplicated code.

[MatousJobanek]

related to the other comment, I don't see how we could get closer to sending many requests to proxy in a parallel way. There are many sub-tests that will be executed in sequential order.
I hoped that we could have something closer to what we do in migration tests. see

toolchain-e2e/test/migration/setup_runner.go

Lines 38 to 59 in 713b901

	func (r *SetupMigrationRunner) Run(t *testing.T) {
	var wg sync.WaitGroup
	toRun := []func(t *testing.T){
	r.prepareAppStudioProvisionedSpace,
	r.prepareSecondMemberProvisionedSpace,
	r.prepareProvisionedUser,
	r.prepareSecondMemberProvisionedUser,
	r.prepareDeactivatedUser,
	r.prepareBannedUser,
	r.prepareAppStudioProvisionedUser}
	for _, funcToRun := range toRun {
	wg.Add(1)
	go func(run func(t *testing.T)) {
	defer wg.Done()
	run(t)
	}(funcToRun)
	}
	wg.Wait()
	}

toolchain-e2e/test/migration/verify/verify_migration_test.go

Lines 68 to 106 in 713b901

	func runVerifyFunctions(t *testing.T, awaitilities wait.Awaitilities) {
	// check MUR migrations and get Signups for the users provisioned in the setup part
	t.Log("checking MUR Migrations")
	provisionedSignup := checkMURMigratedAndGetSignup(t, awaitilities.Host(), migration.ProvisionedUser)
	secondMemberProvisionedSignup := checkMURMigratedAndGetSignup(t, awaitilities.Host(), migration.SecondMemberProvisionedUser)
	appstudioProvisionedSignup := checkMURMigratedAndGetSignup(t, awaitilities.Host(), migration.AppStudioProvisionedUser)
	// note: listing banned/deactivated UserSignups should be done as part of setup because the tests are run in parallel and there can be multiple banned/deactivated UserSignups at that point which could lead to test flakiness
	deactivatedSignup := listAndGetSignupWithState(t, awaitilities.Host(), toolchainv1alpha1.UserSignupStateLabelValueDeactivated)
	bannedSignup := listAndGetSignupWithState(t, awaitilities.Host(), toolchainv1alpha1.UserSignupStateLabelValueBanned)
	var wg sync.WaitGroup
	// prepare all functions to verify the state of the Signups and Spaces
	toRun := []func(){
	// Spaces
	func() { verifyAppStudioProvisionedSpace(t, awaitilities) },
	func() { verifySecondMemberProvisionedSpace(t, awaitilities) },
	// UserSignups
	func() { verifyProvisionedSignup(t, awaitilities, provisionedSignup) },
	func() { verifySecondMemberProvisionedSignup(t, awaitilities, secondMemberProvisionedSignup) },
	func() { verifyAppStudioProvisionedSignup(t, awaitilities, appstudioProvisionedSignup) },
	func() { verifyDeactivatedSignup(t, awaitilities, deactivatedSignup) },
	func() { verifyBannedSignup(t, awaitilities, bannedSignup) },
	}
	// when & then - run all functions in parallel
	for _, funcToRun := range toRun {
	wg.Add(1)
	go func(run func()) {
	defer wg.Done()
	run()
	}(funcToRun)
	}
	wg.Wait()
	cleanup.ExecuteAllCleanTasks(t)
	}

A very very quick example of what it could look like:

...
usersToBeTested := createUsersForParallelTests("user1"..."user10")
usersToBeTested["user1"].ShareSpaceWith(t, hostAwait, users["user2"])
usersToBeTested["user4"].ShareSpaceWith(t, hostAwait, users["user2"])
usersToBeTested["user10"].ShareSpaceWith(t, hostAwait, users["user5"])

...

for _, user := range usersToBeTested {
	// provision users
	user.provision()
	user.shareWithOtherUsers()
	user.verify().assessToWorkspaces() // space lister verification

	for _, action := actionsToBeTested { // CRUD, watch, websockets operations, etc...
		go func() {
			user.verify().action(action).inAllWorkspaces()
		}()
	}
}
...

the actions to be tested are the whole stories like
action 1:

1. create CM
2. list CMs
3. delete CM

action2

1. watch secrets
2. create secret
3. modify it
4. delete it

action3

1. create pod
2. see the logs
3. delete pod

etc...

All these things could be easily hidden behind a nice fluent API and set of functions. You can technically reuse most of the functions that are already there - eg the one for running websockets.

Is this still your plan and this PR is only a preparation for it?

[jkopriva]

Thank you for this comment. I am working on running tests in parallel in similar way, as you pointed out and this PR is more about preparation for next PR(s) with parallelization.

[MatousJobanek]

Let's close this PR for now.
The test case has been moved to the parallel test-suite in the meantime. Also, given the latest changes in the set of priorities and org structure, I don't think that you would have time to take a look at it soon.
We can reopen it if the PR becomes valid again.
But thanks a lot for your contribution and your effort @jkopriva