From 1664ba6b14473e39a7fd8a380d243a4cce471336 Mon Sep 17 00:00:00 2001 From: Nir Soffer Date: Tue, 22 Oct 2024 18:37:40 +0300 Subject: [PATCH] Simplify test context management We had 2 places using a test context - test suites, and action helpers. To work with both, we store the test context in a map and had complicated search mechanism to locate the context in both the parent and child sub-tests. This failed randomly since the map was not protected with a mutex. Simplify the design by implementing the actions (deploy, enable, ..) in the test context. With this we can create a test context instance and pass it to the code running a test flow, and we don't need to manage any global state. Fixes: #1571 Signed-off-by: Nir Soffer --- e2e/actions_test.go | 77 ---------------------------------- e2e/exhaustive_suite_test.go | 28 +++++-------- e2e/testcontext/testcontext.go | 67 +++++++++++++++++------------ 3 files changed, 52 insertions(+), 120 deletions(-) delete mode 100644 e2e/actions_test.go diff --git a/e2e/actions_test.go b/e2e/actions_test.go deleted file mode 100644 index 73bc986eb..000000000 --- a/e2e/actions_test.go +++ /dev/null @@ -1,77 +0,0 @@ -// SPDX-FileCopyrightText: The RamenDR authors -// SPDX-License-Identifier: Apache-2.0 - -package e2e_test - -import ( - "testing" - - "github.com/ramendr/ramen/e2e/dractions" - "github.com/ramendr/ramen/e2e/testcontext" -) - -func DeployAction(t *testing.T) { - testCtx, err := testcontext.GetTestContext(t.Name()) - if err != nil { - t.Error(err) - } - - if err := testCtx.Deployer.Deploy(testCtx.Workload); err != nil { - t.Error(err) - } -} - -func EnableAction(t *testing.T) { - testCtx, err := testcontext.GetTestContext(t.Name()) - if err != nil { - t.Error(err) - } - - if err := dractions.EnableProtection(testCtx.Workload, testCtx.Deployer); err != nil { - t.Error(err) - } -} - -func FailoverAction(t *testing.T) { - testCtx, err := testcontext.GetTestContext(t.Name()) - if err != nil { - t.Error(err) - } - - if err := dractions.Failover(testCtx.Workload, testCtx.Deployer); err != nil { - t.Error(err) - } -} - -func RelocateAction(t *testing.T) { - testCtx, err := testcontext.GetTestContext(t.Name()) - if err != nil { - t.Error(err) - } - - if err := dractions.Relocate(testCtx.Workload, testCtx.Deployer); err != nil { - t.Error(err) - } -} - -func DisableAction(t *testing.T) { - testCtx, err := testcontext.GetTestContext(t.Name()) - if err != nil { - t.Error(err) - } - - if err := dractions.DisableProtection(testCtx.Workload, testCtx.Deployer); err != nil { - t.Error(err) - } -} - -func UndeployAction(t *testing.T) { - testCtx, err := testcontext.GetTestContext(t.Name()) - if err != nil { - t.Error(err) - } - - if err := testCtx.Deployer.Undeploy(testCtx.Workload); err != nil { - t.Error(err) - } -} diff --git a/e2e/exhaustive_suite_test.go b/e2e/exhaustive_suite_test.go index e66cba4d1..581b5c622 100644 --- a/e2e/exhaustive_suite_test.go +++ b/e2e/exhaustive_suite_test.go @@ -89,48 +89,42 @@ func Exhaustive(t *testing.T) { t.Parallel() t.Run(d.GetName(), func(t *testing.T) { t.Parallel() - testcontext.AddTestContext(t.Name(), w, d) - runTestFlow(t) - testcontext.DeleteTestContext(t.Name(), w, d) + ctx := testcontext.TestContext{Workload: w, Deployer: d} + runTestFlow(t, ctx) }) }) } } } -func runTestFlow(t *testing.T) { +func runTestFlow(t *testing.T, ctx testcontext.TestContext) { t.Helper() - testCtx, err := testcontext.GetTestContext(t.Name()) - if err != nil { - t.Fatal(err) + if !ctx.Deployer.IsWorkloadSupported(ctx.Workload) { + t.Skipf("Workload %s not supported by deployer %s, skip test", ctx.Workload.GetName(), ctx.Deployer.GetName()) } - if !testCtx.Deployer.IsWorkloadSupported(testCtx.Workload) { - t.Skipf("Workload %s not supported by deployer %s, skip test", testCtx.Workload.GetName(), testCtx.Deployer.GetName()) - } - - if !t.Run("Deploy", DeployAction) { + if !t.Run("Deploy", ctx.Deploy) { t.Fatal("Deploy failed") } - if !t.Run("Enable", EnableAction) { + if !t.Run("Enable", ctx.Enable) { t.Fatal("Enable failed") } - if !t.Run("Failover", FailoverAction) { + if !t.Run("Failover", ctx.Failover) { t.Fatal("Failover failed") } - if !t.Run("Relocate", RelocateAction) { + if !t.Run("Relocate", ctx.Relocate) { t.Fatal("Relocate failed") } - if !t.Run("Disable", DisableAction) { + if !t.Run("Disable", ctx.Disable) { t.Fatal("Disable failed") } - if !t.Run("Undeploy", UndeployAction) { + if !t.Run("Undeploy", ctx.Undeploy) { t.Fatal("Undeploy failed") } } diff --git a/e2e/testcontext/testcontext.go b/e2e/testcontext/testcontext.go index 1c0078834..e85fe93bd 100644 --- a/e2e/testcontext/testcontext.go +++ b/e2e/testcontext/testcontext.go @@ -4,10 +4,10 @@ package testcontext import ( - "fmt" - "strings" + "testing" "github.com/ramendr/ramen/e2e/deployers" + "github.com/ramendr/ramen/e2e/dractions" "github.com/ramendr/ramen/e2e/workloads" ) @@ -16,35 +16,50 @@ type TestContext struct { Deployer deployers.Deployer } -var testContextMap = make(map[string]TestContext) +func (c *TestContext) Deploy(t *testing.T) { + t.Helper() -// Based on name passed, Init the deployer and Workload and stash in a map[string]TestContext -func AddTestContext(name string, w workloads.Workload, d deployers.Deployer) { - testContextMap[name] = TestContext{w, d} + if err := c.Deployer.Deploy(c.Workload); err != nil { + t.Fatal(err) + } } -func DeleteTestContext(name string, w workloads.Workload, d deployers.Deployer) { - delete(testContextMap, name) +func (c *TestContext) Enable(t *testing.T) { + t.Helper() + + if err := dractions.EnableProtection(c.Workload, c.Deployer); err != nil { + t.Fatal(err) + } } -// Search name in map for a TestContext to return, if not found go backward -// - drop the last / suffix form name and search -// - e.g If name passed is "TestSuites/Exhaustive/DaemonSet/Subscription/Undeploy" -// - Search for above name first (it will not be found as we create context at a point where we have a d+w) -// - Search for "TestSuites/Exhaustive/DaemonSet/Subscription" (should be found) -func GetTestContext(name string) (TestContext, error) { - testCtx, ok := testContextMap[name] - if !ok { - i := strings.LastIndex(name, "/") - if i < 1 { - return TestContext{}, fmt.Errorf("not a valid name in TestContext: %v", name) - } - - testCtx, ok = testContextMap[name[0:i]] - if !ok { - return TestContext{}, fmt.Errorf("can not find testContext with name: %v", name) - } +func (c *TestContext) Failover(t *testing.T) { + t.Helper() + + if err := dractions.Failover(c.Workload, c.Deployer); err != nil { + t.Fatal(err) } +} + +func (c *TestContext) Relocate(t *testing.T) { + t.Helper() - return testCtx, nil + if err := dractions.Relocate(c.Workload, c.Deployer); err != nil { + t.Fatal(err) + } +} + +func (c *TestContext) Disable(t *testing.T) { + t.Helper() + + if err := dractions.DisableProtection(c.Workload, c.Deployer); err != nil { + t.Fatal(err) + } +} + +func (c *TestContext) Undeploy(t *testing.T) { + t.Helper() + + if err := c.Deployer.Undeploy(c.Workload); err != nil { + t.Fatal(err) + } }