e2e: remove the use of test context #1606

raghavendra-talur · 2024-10-25T20:05:39Z

We remove the need for test context by using t.Run() to create anonymous
test functions and always passing the required parameters to the
required functions.

There are two other related changes in the commit:

By creating one subtest with the combined name of the workload and
the deployer, we remove the problem where go test was adding a number
suffix to the subtests.
The name of the workload is changed back to Deployment. This is to
differentiate it from the Deploy action in the test names.

We can run any combination of workload and deployer. We can also restrict it to any one of the action.

Example:

$ go test -run "TestSuites/Exhaustive/Deployment-rbd-Subscr" ./... -v
?       github.com/ramendr/ramen/e2e/argocd     [no test files]
?       github.com/ramendr/ramen/e2e/deployers  [no test files]
?       github.com/ramendr/ramen/e2e/dractions  [no test files]
?       github.com/ramendr/ramen/e2e/util       [no test files]
?       github.com/ramendr/ramen/e2e/workloads  [no test files]
=== RUN   TestSuites
=== RUN   TestSuites/Exhaustive
=== PAUSE TestSuites/Exhaustive
=== CONT  TestSuites/Exhaustive
=== RUN   TestSuites/Exhaustive/Deployment-rbd-Subscr
=== PAUSE TestSuites/Exhaustive/Deployment-rbd-Subscr
=== CONT  TestSuites/Exhaustive/Deployment-rbd-Subscr
=== RUN   TestSuites/Exhaustive/Deployment-rbd-Subscr/Deploy
=== RUN   TestSuites/Exhaustive/Deployment-rbd-Subscr/Enable
=== RUN   TestSuites/Exhaustive/Deployment-rbd-Subscr/Failover
=== RUN   TestSuites/Exhaustive/Deployment-rbd-Subscr/Relocate
=== RUN   TestSuites/Exhaustive/Deployment-rbd-Subscr/Disable
=== RUN   TestSuites/Exhaustive/Deployment-rbd-Subscr/Undeploy
--- PASS: TestSuites (0.00s)
    --- PASS: TestSuites/Exhaustive (0.02s)
        --- PASS: TestSuites/Exhaustive/Deployment-rbd-Subscr (470.19s)
            --- PASS: TestSuites/Exhaustive/Deployment-rbd-Subscr/Deploy (5.02s)
            --- PASS: TestSuites/Exhaustive/Deployment-rbd-Subscr/Enable (90.07s)
            --- PASS: TestSuites/Exhaustive/Deployment-rbd-Subscr/Failover (215.04s)
            --- PASS: TestSuites/Exhaustive/Deployment-rbd-Subscr/Relocate (125.03s)
            --- PASS: TestSuites/Exhaustive/Deployment-rbd-Subscr/Disable (35.01s)
            --- PASS: TestSuites/Exhaustive/Deployment-rbd-Subscr/Undeploy (0.02s)
PASS
ok      github.com/ramendr/ramen/e2e    470.740s

$ go test -run "TestSuites/Exhaustive/Deployment-rbd-Subscr/Deploy" ./... -v
?       github.com/ramendr/ramen/e2e/argocd     [no test files]
?       github.com/ramendr/ramen/e2e/deployers  [no test files]
?       github.com/ramendr/ramen/e2e/dractions  [no test files]
?       github.com/ramendr/ramen/e2e/util       [no test files]
?       github.com/ramendr/ramen/e2e/workloads  [no test files]
=== RUN   TestSuites
=== RUN   TestSuites/Exhaustive
=== PAUSE TestSuites/Exhaustive
=== CONT  TestSuites/Exhaustive
=== RUN   TestSuites/Exhaustive/Deployment-rbd-Subscr
=== PAUSE TestSuites/Exhaustive/Deployment-rbd-Subscr
=== CONT  TestSuites/Exhaustive/Deployment-rbd-Subscr
=== RUN   TestSuites/Exhaustive/Deployment-rbd-Subscr/Deploy
--- PASS: TestSuites (0.00s)
    --- PASS: TestSuites/Exhaustive (0.02s)
        --- PASS: TestSuites/Exhaustive/Deployment-rbd-Subscr (5.03s)
            --- PASS: TestSuites/Exhaustive/Deployment-rbd-Subscr/Deploy (5.03s)
PASS
ok      github.com/ramendr/ramen/e2e    5.476s

$ go test -run "TestSuites/Exhaustive/Deployment-rbd-Subscr/Enable" ./... -v
?       github.com/ramendr/ramen/e2e/argocd     [no test files]
?       github.com/ramendr/ramen/e2e/deployers  [no test files]
?       github.com/ramendr/ramen/e2e/dractions  [no test files]
?       github.com/ramendr/ramen/e2e/util       [no test files]
?       github.com/ramendr/ramen/e2e/workloads  [no test files]
=== RUN   TestSuites
=== RUN   TestSuites/Exhaustive
=== PAUSE TestSuites/Exhaustive
=== CONT  TestSuites/Exhaustive
=== RUN   TestSuites/Exhaustive/Deployment-rbd-Subscr
=== PAUSE TestSuites/Exhaustive/Deployment-rbd-Subscr
=== CONT  TestSuites/Exhaustive/Deployment-rbd-Subscr
=== RUN   TestSuites/Exhaustive/Deployment-rbd-Subscr/Enable
--- PASS: TestSuites (0.00s)
    --- PASS: TestSuites/Exhaustive (0.02s)
        --- PASS: TestSuites/Exhaustive/Deployment-rbd-Subscr (90.05s)
            --- PASS: TestSuites/Exhaustive/Deployment-rbd-Subscr/Enable (90.05s)
PASS
ok      github.com/ramendr/ramen/e2e    90.487s

We remove the need for test context by using t.Run() to create anonymous test functions and always passing the required parameters to the required functions. There are two other related changes in the commit: 1. By creating one subtest with the combined name of the workload and the deployer, we remove the problem where go test was adding a number suffix to the subtests. 2. The name of the workload is changed back to Deployment. This is to differentiate it from the Deploy action in the test names. Signed-off-by: Raghavendra Talur <[email protected]>

raghavendra-talur · 2024-10-25T20:07:16Z

Fixes #1571

nirs · 2024-10-25T20:21:01Z

We remove the need for test context by using t.Run() to create anonymous test functions and always passing the required parameters to the required functions.

Did you see #1598?

I planned to replace all the code accepting (Workload, Deployer) to accept a TestContext. This way when we add more stuff to the context, we don't need to change all the functions signatures.

There are two other related changes in the commit:

By creating one subtest with the combined name of the workload and
the deployer, we remove the problem where go test was adding a number
suffix to the subtests.

Great!

The name of the workload is changed back to Deployment. This is to
differentiate it from the Deploy action in the test names.

I think the name of the workload should be something like "rbd-vr" or "rbd-vs" - we don't care much about the actual deployment or busybox, but about the the area we test in ramen.

We can run any combination of workload and deployer. We can also restrict it to any one of the action.

Awesome! I could not make this work before.

nirs · 2024-10-25T20:32:40Z

e2e/exhaustive_suite_test.go

@@ -55,7 +55,7 @@ func generateWorkloads([]workloads.Workload) {
 			Path:     GITPATH,
 			Revision: GITREVISION,
 			AppName:  APPNAME,
-			Name:     fmt.Sprintf("Deploy-%s", suffix),
+			Name:     fmt.Sprintf("Deployment-%s", suffix),


Deploy is indeed confusing. I think the a better way is to remove the "Deploy-" prefix.

nirs · 2024-10-25T20:34:00Z

e2e/exhaustive_suite_test.go

@@ -85,52 +85,54 @@ func Exhaustive(t *testing.T) {
 			w := workload
 			d := deployer

-			t.Run(w.GetName(), func(t *testing.T) {
+			t.Run(w.GetName()+"-"+d.GetName(), func(t *testing.T) {


Adding a function accepting a workload and deloyer and returning a test name would be little nicer.

nirs · 2024-10-25T20:35:11Z

e2e/exhaustive_suite_test.go

-					testcontext.AddTestContext(t.Name(), w, d)
-					runTestFlow(t)
-					testcontext.DeleteTestContext(t.Name(), w, d)
-				})


This is much better! the previous way was too complicated without any benefit.

nirs · 2024-10-25T20:38:05Z

e2e/exhaustive_suite_test.go

+	t.Run("Deploy", func(t *testing.T) {
+		if err := d.Deploy(w); err != nil {
+			t.Fatal("Deploy failed")
+		}


This does not improve the code. Being able to run a sub test defined elsewhere is nicer. It will make it easier to create more flows except this simple flow. I think #1598 is better in this case.

We had nested loops, creating a sub test for the workload, and then nesting more sub tests for every deployer. This causes the go test framework to add "#N" suffix to the sub test name, which make the name unpredictable and harder to use for running specific tests using the -run option. Simplify the way we generated sub tests, so we generate one sub test for every deployer-workload combination. The sub test name is is the same name we used for the test namespace and all resources, making it easier to follow and more consistent. Because we use deployer-workload format, and not workload-deployer, switch the loops to match the name format. Example run showing the new structure: --- PASS: TestSuites (0.06s) --- PASS: TestSuites/Validate (0.06s) --- PASS: TestSuites/Validate/CheckRamenHubOperatorStatus (0.03s) --- PASS: TestSuites/Validate/CheckRamenSpokeOperatorStatus (0.03s) --- PASS: TestSuites/Exhaustive (6.04s) --- SKIP: TestSuites/Exhaustive/disapp-deploy-cephfs-busybox (0.00s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox (431.67s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox/Deploy (5.16s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox/Enable (155.16s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox/Failover (85.08s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox/Relocate (150.14s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox/Disable (30.03s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox/Undeploy (6.10s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox (461.77s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox/Deploy (5.17s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox/Enable (90.24s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox/Failover (180.15s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox/Relocate (150.14s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox/Disable (30.03s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox/Undeploy (6.05s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox (496.64s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox/Deploy (1.39s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox/Enable (90.11s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox/Failover (179.97s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox/Relocate (179.87s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox/Disable (30.05s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox/Undeploy (15.24s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox (600.76s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox/Deploy (0.16s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox/Enable (150.18s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox/Failover (180.16s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox/Relocate (240.21s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox/Disable (30.03s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox/Undeploy (0.02s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox (696.19s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox/Deploy (0.16s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox/Enable (95.34s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox/Failover (300.39s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox/Relocate (270.26s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox/Disable (30.03s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox/Undeploy (0.02s) With this change we can run any single test using: ./run.sh -run TestSuites/Exhaustive/appset-deploy-cephfs-busybox We can also run multiple tests, for example all tests using applicationsets: ./run.sh -run TestSuites/Exhaustive/appset Or all tests using rbd storage: ./run.sh -run TestSuites/Exhaustive/rbd Based on Talur's PR RamenDR#1606, which has also other unrelated changes. Fixes: RamenDR#1614 Signed-off-by: Nir Soffer <[email protected]>

We had nested loops, creating a sub test for the workload, and then nesting more sub tests for every deployer. This causes the go test framework to add "#N" suffix to the sub test name, which make the name unpredictable and harder to use for running specific tests using the -run option. Simplify the way we generated sub tests, so we generate one sub test for every deployer-workload combination. The sub test name is is the same name we used for the test namespace and all resources, making it easier to follow and more consistent. Because we use deployer-workload format, and not workload-deployer, switch the loops to match the name format. Example run showing the new structure: --- PASS: TestSuites (0.06s) --- PASS: TestSuites/Validate (0.06s) --- PASS: TestSuites/Validate/CheckRamenHubOperatorStatus (0.03s) --- PASS: TestSuites/Validate/CheckRamenSpokeOperatorStatus (0.03s) --- PASS: TestSuites/Exhaustive (6.04s) --- SKIP: TestSuites/Exhaustive/disapp-deploy-cephfs-busybox (0.00s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox (431.67s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox/Deploy (5.16s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox/Enable (155.16s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox/Failover (85.08s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox/Relocate (150.14s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox/Disable (30.03s) --- PASS: TestSuites/Exhaustive/subscr-deploy-cephfs-busybox/Undeploy (6.10s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox (461.77s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox/Deploy (5.17s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox/Enable (90.24s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox/Failover (180.15s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox/Relocate (150.14s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox/Disable (30.03s) --- PASS: TestSuites/Exhaustive/subscr-deploy-rbd-busybox/Undeploy (6.05s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox (496.64s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox/Deploy (1.39s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox/Enable (90.11s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox/Failover (179.97s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox/Relocate (179.87s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox/Disable (30.05s) --- PASS: TestSuites/Exhaustive/disapp-deploy-rbd-busybox/Undeploy (15.24s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox (600.76s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox/Deploy (0.16s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox/Enable (150.18s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox/Failover (180.16s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox/Relocate (240.21s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox/Disable (30.03s) --- PASS: TestSuites/Exhaustive/appset-deploy-cephfs-busybox/Undeploy (0.02s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox (696.19s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox/Deploy (0.16s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox/Enable (95.34s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox/Failover (300.39s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox/Relocate (270.26s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox/Disable (30.03s) --- PASS: TestSuites/Exhaustive/appset-deploy-rbd-busybox/Undeploy (0.02s) With this change we can run any single test using: ./run.sh -run TestSuites/Exhaustive/appset-deploy-cephfs-busybox We can also run multiple tests, for example all tests using applicationsets: ./run.sh -run TestSuites/Exhaustive/appset Or all tests using rbd storage: ./run.sh -run TestSuites/Exhaustive/rbd Based on Talur's PR #1606, which has also other unrelated changes. Fixes: #1614 Signed-off-by: Nir Soffer <[email protected]>

nirs · 2024-12-12T10:35:00Z

@raghavendra-talur we can closed this now, all the changes already included.

raghavendra-talur requested review from ShyamsundarR and nirs October 25, 2024 20:05

nirs reviewed Oct 25, 2024

View reviewed changes

raghavendra-talur force-pushed the rtalur-test-context branch 3 times, most recently from bc818e2 to 450dc2d Compare October 26, 2024 07:08

This was referenced Oct 28, 2024

e2e: log errors #1596

Closed

e2e: Fix thread unsafe map access #1591

Merged

e2e: Cannot run single test #1614

Closed

nirs mentioned this pull request Oct 31, 2024

e2e: Eliminate unneeded nesting #1634

Merged

raghavendra-talur closed this Dec 12, 2024

raghavendra-talur deleted the rtalur-test-context branch February 11, 2025 08:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

e2e: remove the use of test context #1606

e2e: remove the use of test context #1606

raghavendra-talur commented Oct 25, 2024

raghavendra-talur commented Oct 25, 2024

nirs commented Oct 25, 2024

nirs Oct 25, 2024

nirs Oct 25, 2024

nirs Oct 25, 2024

nirs Oct 25, 2024

nirs commented Dec 12, 2024

e2e: remove the use of test context #1606

e2e: remove the use of test context #1606

Conversation

raghavendra-talur commented Oct 25, 2024

raghavendra-talur commented Oct 25, 2024

nirs commented Oct 25, 2024

nirs Oct 25, 2024

Choose a reason for hiding this comment

nirs Oct 25, 2024

Choose a reason for hiding this comment

nirs Oct 25, 2024

Choose a reason for hiding this comment

nirs Oct 25, 2024

Choose a reason for hiding this comment

nirs commented Dec 12, 2024