e2e: Replace logr.Logger with zap.SugaredLogger #1684
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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]>
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]>
This is an alternative to RamenDR#1596, trying to keep the test code more idiomatic, cleaner, and easier to use correctly. Add e2e/test.T type, embedding the standard library *testing.T, and keeping a logger. This type overrides testing.T methods logging to the standard library logger with methods logging to the specific logger. The Fatal[f] and Error[]f methods log an ERROR messages to make errors easy to find. To use the our T type, a test need to wrap the standard library *testing.T with the per-test logger: func TestFoo(dt *testing.T) { t := e2etesting.WithLog(dt, ctx.Logger()) Now we can use t transparently in this test: t.Log("This logs an INFO message to our logger") Note that the wrapped Error() and Fatal() method accept an error and will log a stracktrace: if err := ...; err != nil { // Log an error with a stracktrace and mark the test as failed. t.Error(err, "Failed, trying next step") } if err := ...; err != nil { // Log an error with a stracktrace and fail the test immediately. t.Fatal(err, "Failed, cannot continue") } Example error log (wrapped for readability): 2024-11-29T03:12:32.837+0200 ERROR subscr-deploy-rbd-busybox test/testing.go:55 Failed to failover workload: fake error in waitAndUpdateDRPC: failover to cluster "dr1" github.com/ramendr/ramen/e2e/test.(*T).Fatalf /Users/nsoffer/src/ramen/e2e/test/testing.go:55 github.com/ramendr/ramen/e2e/test.(*Context).Failover /Users/nsoffer/src/ramen/e2e/test/context.go:102 testing.tRunner /opt/homebrew/Cellar/go/1.23.3/libexec/src/testing/testing.go:1690 Notes: - All the test functions were updated to accept a `*testing.T` and wrap it with per-test log. Test helper functions accept now a `*e2e/test.T` so they don't need extra wrapping. - Sub tests requires wrapping the standard library t again, since testing.T is a type, not an interface. This adds one line of boilerplate for every sub test. - All Error[f] and Fatal[f] calls changed to format an error message with the underlying error so we should get more helpful error messages when something fails deep in the utility functions. - When DR sub test (e.g. Deploy) fails, we already logged the error by failing the sub test, so we call FailNow() in the parent test to abort the parent test silently. Issues: - The stracktrace does not include all callers leading the point the error was created, since fmt.Errorf() does not capture the stack. We can get better stacktrace by using another error package that does this like https://pkg.go.dev/github.com/go-errors/errors. - We need to disable the `thelper` linter in test.Context to allow using `dt *testing.T` and keep the test code idiomatic. Another way is to keep `t *testing.T`, and use `et` for the wrapped t. This makes the test code less idiomatic but it may be more clear that this is a non standard `t`. Fixes: RamenDR#1595 Signed-off-by: Nir Soffer <[email protected]>
We used zap from controller runtime, returning logr.Logger. Replace it with zap SugaredLoger which is much nicer to use, supporting both structured and formatted logging and named log levels. We use the default development configuration which require no special setup. Replace the logs using string concatenation or fmt.Sprintf() with log.Infof() or log.Errorf(), using quoted arguments for more clear logs. Signed-off-by: Nir Soffer <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We use zap from controller runtime, returning logr.Logger. Replace it
with zap SugaredLoger which is much nicer to use, supporting both
structured and formatted logging and named log levels.
We use the default development configuration which require no special
setup.
All the logs using string concatenation or fmt.Sprintf() replace with
log.Infof() and log.Errof() and use %q format for more clear logs.
Part of #1686
Based on #1683 for testing.