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

e2e: Simplify test context management #1598

Merged
merged 12 commits into from
Dec 4, 2024
Merged

Conversation

nirs
Copy link
Member

@nirs nirs commented Oct 22, 2024

Simplify the way we managed text context and prepare for using it in the entire e2e module.

  • Eliminate the global contexts map and complicated logic to access the context from different modules by moving the action wrappers to the contexts.
  • Rename testcontext.TestContext to test.Context
  • Simplify test generation
  • Add test.Context.Name
  • Add per-test loger to test.Context
  • Eliminate interface dependencies with the e2e/types package
  • Implement the e2e/types.Context interface
  • Introduce test.Context.Validate()
  • Return more context from kubectl error
  • Remove unneeded log argument
  • Pass test context to utility functions
  • Add test.Context.Namsapce()

With these changes we are ready to complete #1683.

@nirs nirs requested review from rakeshgm and BenamarMk October 22, 2024 17:30
This was referenced Oct 25, 2024
@nirs nirs requested a review from ELENAGER October 28, 2024 17:03
@nirs nirs changed the title Simplify test context management e2e: Simplify test context management Oct 30, 2024
@nirs nirs mentioned this pull request Oct 30, 2024
@nirs
Copy link
Member Author

nirs commented Nov 26, 2024

Summary from discussion with @raghavendra-talur:

We want to have a DRTest struct keeping everything needed for a test:

type DRTest struct {
    Workload ...
    Deployer ...
    Log ...
}

We will not implement any wrappers like Deploy/Undeploy/Protect/Unprotect/Failover/Relocate on the context for now, using functions to create sub tests. We can add this later when we have more flows.

We want to create this struct when we start a test, and pass it around instead of passing multiple arguments to all utility functions. This way we have access to everything related to a single test everywhere, and it is very easy to add or remove stuff without changing function signature everywhere.

This can also be used to eliminate the global Ctx later.

@nirs nirs marked this pull request as draft November 26, 2024 18:17
@nirs nirs marked this pull request as ready for review November 28, 2024 02:35
@nirs
Copy link
Member Author

nirs commented Nov 28, 2024

Summary from discussion with @raghavendra-talur:

We want to have a DRTest struct keeping everything needed for a test:

type DRTest struct {
    Workload ...
    Deployer ...
    Log ...
}

DRTest or Test does not works well like test.Context, so I kept the current name.

We will not implement any wrappers like Deploy/Undeploy/Protect/Unprotect/Failover/Relocate on the context for now, using functions to create sub tests. We can add this later when we have more flows.

Tried this, it is 2 times longer, full of boilerplate and required multiple //nolint: to make the linter happy. Keeping the actions are methods on the context is much better so I kept the current change.

Also tried table based test - with method on the context is very nice:

func runTestFlow(t *testing.T, ctx testcontext.TestContext) {
    t.Helper()

    if !ctx.Deployer.IsWorkloadSupported(ctx.Workload) {
        t.Skipf("Workload %s not supported by deployer %s, skip test", ctx.Workload.GetName(), ctx.Deployer.GetName())
    }   

    flow := []testcontext.Step{
        {Name: "Deploy", Func: ctx.Deploy},
        {Name: "Enable", Func: ctx.Enable},
        {Name: "Failover", Func: ctx.Failover},
        {Name: "Relocate", Func: ctx.Relocate},
        {Name: "Disable", Func: ctx.Disable},
        {Name: "Undeploy", Func: ctx.Uneploy},
    }   

    for _, step := range flow {
        if !t.Run(step.Name, step.Func) {
            t.Fatalf("%s failed", step.Name)
        }   
    }   
}    

But the current test is much simpler code so I kept is as is. If we will have multiple flows or much long flow, table based test is the way to keep it clean and easy to modify.

The key to keep is nice is to use methods on the context.

Copy link
Member

@raghavendra-talur raghavendra-talur left a comment

Choose a reason for hiding this comment

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

There is only one small typo that I would like to see fixed before merging this PR. We can make more enhancements later.

e2e/testcontext/testcontext.go Outdated Show resolved Hide resolved
nirs added 2 commits December 4, 2024 15:35
We had 2 places using a test context - test suites, and action helpers.
To work with both, we store the test context in a global map and had
complicated search mechanism to locate the context in both the parent
and child sub-tests.

Simplify the design by moving the actions wrappers (deploy, enable, ..)
to the test context, and passing a text context to the function running
the test flow.

Signed-off-by: Nir Soffer <[email protected]>
Repeating the same names in the package name and types is anti-pattern.

I think this a really good name for our use case. The only issue is not
confusing it with context.Context, but since we always use the package
name test.Context is clearly not a context.Context.

The test package can keep other testing helpers that today are in the
generic util package.

Signed-off-by: Nir Soffer <[email protected]>
nirs added 10 commits December 4, 2024 15:38
We had a workaround for loop iteration issue which is not needed for Go
1.22. It was actually not needed even in older versions since we can
assign the workload and deployer in the context outside of the subtest.

Signed-off-by: Nir Soffer <[email protected]>
To create a sub test we need a name. This name is used also for per-test
logger related resources. Lets compute and keep this name in the test
context.

To keep test code clean, add test.NewContext() function to create the
test. This will be useful for other initialization later.

Signed-off-by: Nir Soffer <[email protected]>
This will be used to log errors during the test flow, and once we pass
the context to all utility functions, they will be able to use the right
logger instead of passing log argument everywhere.

Signed-off-by: Nir Soffer <[email protected]>
We have Workload and Deployer interfaces, allowing multiple
implementations without depending on concrete types. This is great, but
the interfaces are part of the workloads and deployers pacakges, which
the wrong place. This creates unneeded and unwanted dependencies between
the packages, and create circular dependencies if we want to pass a test
context to the deployers and workloads.

Fix the dependencies by introducing the e2e/types package, providing:

- Workload interface
- Deployer interface
- Context interface (not implemented yet)

We still have some dependencies on deployers functions like
deployers.GetCombinedName(). These will be moved to test.Context or util
package later.

Signed-off-by: Nir Soffer <[email protected]>
The test.Context type implements now the e2e/types.Context interface.
This allows passing a text.Context to deployers, workloads, and actions
functions, so they can access all the details of a test, and log to the
per-test logger.

Tests do not use the types.Context interface, since they need the
actions methods which are not part of the interface. This is not an
issue since test.Context is couple to a test; the test creates the
context and use it action methods to drive the test flow.

Signed-off-by: Nir Soffer <[email protected]>
Validate() returns and error if the combination of deployer and workload
is not supported. The error is used as the message for skipping the
test. This cleans up the test and makes it easy to add more tests.

In the future this will also validate if the workload can work on the
clusters. One example is testing virtual machines if clusters does not
have have kubevirt and CDI.

Signed-off-by: Nir Soffer <[email protected]>
Previously if kubectl failed, we logged the command stdout, and return
the command error, which does *not* include stderr output from the
command. This ensure that we don't have any way to debug the failure.

Fix by extracting stderr from the command error, and returning an error
wrapping the command error and including the command stdout and stderr.

Signed-off-by: Nir Soffer <[email protected]>
When we fail and return an error, we should log INFO message about the
same error. Return an error with some context and remove log.

Signed-off-by: Nir Soffer <[email protected]>
Instead of passing workload, deployer, log to workloads and deployers
functions, pass the text context providing all of them.

This also remove most accesses to the global util.Ctx.Log for creating a
per-test logger, since we already created the logger in the test
context.

util.DeleteNameapce() is used both in test context and in global
context, so it still accepts a logger.

Signed-off-by: Nir Soffer <[email protected]>
The namespace for ramen resources depends on the deployer and the
workload. ApplicationSet uses argocd namespace, and DiscoveredApps uses
the ramen-ops namesapce. Subscription uses an arbitrary namespace set by
the admin.

Since the admin namespace is arbitrary, the right place for configuring
a test is the text context.

Changes:
- Add Deployer.Namespace() - return the special namespace for the
  deployer
- Add Context.Namesapce() - return the namespace for this context, which
  is the deployer namespace if not empty, or the context name.
- Build the context name in the context, removing
  deployers.GetCombinedName().

This eliminates the dependency on the deployers package from the test
package.

Signed-off-by: Nir Soffer <[email protected]>
@nirs
Copy link
Member Author

nirs commented Dec 4, 2024

The e2e failure looks like #1659

@raghavendra-talur raghavendra-talur merged commit 507773a into RamenDR:main Dec 4, 2024
22 of 23 checks passed
@nirs nirs deleted the e2e-context branch December 4, 2024 15:02
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.

2 participants