From 8e0040ad515917340a36a5bd428ecfa5ff121fcb Mon Sep 17 00:00:00 2001 From: Nahshon Unna-Tsameret Date: Wed, 19 Jun 2024 12:15:37 +0300 Subject: [PATCH] Proposal: Properly use contexts in functional tests Signed-off-by: Nahshon Unna-Tsameret --- ...perly-using-context-in-functional-tests.md | 133 ++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 design-proposals/testing-and-ci/properly-using-context-in-functional-tests.md diff --git a/design-proposals/testing-and-ci/properly-using-context-in-functional-tests.md b/design-proposals/testing-and-ci/properly-using-context-in-functional-tests.md new file mode 100644 index 00000000..5e142292 --- /dev/null +++ b/design-proposals/testing-and-ci/properly-using-context-in-functional-tests.md @@ -0,0 +1,133 @@ +# Properly use contexts in functional tests + +## Overview +This proposal is about properly use golang contexts in KubeVirt functional tests. + +## Introduction +Since the start of the KubeVirt project, contexts became a very important concept +in golang, when writing an asynchronous logic. In a way, the KubeVirt code left +behind regarding the proper usage of golang contexts. + +Using `context.Background()` or even worse, `context.TODO()` is a code smell, because +doing that won't allow the application to cancel async-operation, like I/O, for example, +in case of termination. + +Closing this gap is a huge effort. Contexts should be passed deeper and deeper down +the call stack, changing the api of many functions and methods. The impact is meaningful. + +The situation in the functional test is a much easier to handle. ginkgo documentation +describes how to deal with interrupts, by letting ginkgo inject context object to +the tests. See here for more details: +[https://onsi.github.io/ginkgo/#spec-timeouts-and-interruptible-nodes](https://onsi.github.io/ginkgo/#spec-timeouts-and-interruptible-nodes) + +## Goals +- Define coding standards for the functional tests, regarding the proper way of using contexts +- Modify the current code to use the new standards +- Using the new standards as a gate for code review of functional tests + +## Design +### Coding Standards for Using contexts in Functional Tests + +To make sure that any asynchronous operation is canceled when a test is +terminated, make sure to use golang contexts properly. See more details in +[ginkgo documentation](https://onsi.github.io/ginkgo/#spec-timeouts-and-interruptible-nodes) + +Avoid using new contexts (e.g. `context.Background()` or `context.TODO()`). +Instead, prefer using the context injected by ginkgo. + +First, add the `ctx` optional parameter of the anonymous functions in `It`, `BeforeEach`, +`AfterEach` or `DescribeTable`. Then use this parameter for all places where a context is +needed in this container. + +ginkgo will inject a context to the test, by populating the `ctx` parameter with a context +controlled by ginkgo. + +For example: +```go +It("should test something", func(ctx context.Context){ + ... + kv, err = virtClient.KubeVirt(kvName).Update(ctx, kv, metav1.UpdateOptions{}) + Expect(err).ToNot(HaveOccurred()) + ... +}) +``` + +When using asynchronous assertion, i.e. `Eventually` or `Consistently`, pass the container's context +using the `WithContext` method, and add the context parameter to the checked function; e.g. +```go +It("should test something", func(ctx context.Context){ + ... + + Eventually(func(ctx context.Context) error { + kv, err = virtClient.KubeVirt(kvName).Update(ctx, kv, metav1.UpdateOptions{}) + ... + + return err + }).WithTimeout(120 * time.Seconds). + WithPolling(10 * time.Seconds). + WithContext(ctx). + Should(Succeed()) + + ... +}) +``` + +Notice that in case of also passing the `Gomega` parameter, the context parameter will be the second one, i.e. +```go +It("should test something", func (ctx context.Context){ + ... + + Eventually(func(g Gomega, ctx context.Context) { + kv, err = virtClient.KubeVirt(kvName).Update(ctx, kv, metav1.UpdateOptions{}) + g.Expect(err).ToNot(HaveOccurred()) + ... + }).WithTimeout(120 * time.Seconds). + WithPolling(10 * time.Seconds). + WithContext(ctx). + Should(Succeed()) + + ... +}) +``` + +Also, notice that gingko cancels contexts when exiting a container, therefore avoid using the same context across +multiple containers. This example will fail with canceled context error: +```go +var _ = Describe("test something", func(){ + var ctx context.Context + + BeforeEach(func(gctx context.Context){ + ctx = gctx // or something like ctx = context.WithTimeout(gctx, ...) + }) + + It(func(){ + kv, err = virtClient.KubeVirt(kvName).Update(ctx, kv, metav1.UpdateOptions{}) // <= will fail here + ... + }) +}) +``` + +#### Bad Example +```go +It("should test something", func(){ + ... + kv, err = virtClient.KubeVirt(kvName).Update(context.Backgound(), kv, metav1.UpdateOptions{}) + Expect(err).ToNot(HaveOccurred()) + ... +}) +``` +The above example, uses a new context when performing an I/O operation - using the client to perform +a call to the kubernets cluster. This context can't be canceled and in case of interrupt, this is not +handled properly. Using the ginkgo context, will cause a proper cancellation of the I/O operation in +this case. + +## Additional Reading +* [ginkgo documentation](https://onsi.github.io/ginkgo/#spec-timeouts-and-interruptible-nodes) +* [kubernetes coding standards](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-testing/writing-good-e2e-tests.md#interrupting-tests) + +## Related PRs +* HCO implementation: [hyperconverged-cluster-operator/pull/2952](https://github.com/kubevirt/hyperconverged-cluster-operator/pull/2952) +* First KubeVirt PR to start implementing the new coding standards: [kubevirt/pull/12140](https://github.com/kubevirt/kubevirt/pull/12140) + (Halt. Waiting for this proposal) +* KubeVirt coding standards: [kubevirt/pull/12148](https://github.com/kubevirt/kubevirt/pull/12148) + (Reverted. Waiting for this proposal)