Proposal: New coding standards for properly use contexts in functional tests #304
Conversation
kubevirt-bot
added
the
dco-signoff: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cc @0xFelix |
|
👋 How does this relate to the work that Felix did in kubevirt/kubevirt repo. |
|
|
||
| ## Goals | ||
| - Define coding standards for the functional tests, regarding the proper way of using contexts | ||
| - Modify the current code to use the new standards |
There was a problem hiding this comment.
yes. I can fix that. but I think the context of this whole page is about functional tests.
| 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. |
There was a problem hiding this comment.
Are we talking about the actual code or the test code here?
There was a problem hiding this comment.
In which way is the impact meaningful? Measurable time difference? Less hangs?
There was a problem hiding this comment.
What I was trying to say is that in production code, it will be very hard to move contexts around, and it much easier to do that in the functional tests, so let's start there
There was a problem hiding this comment.
re-wrote the whole paragraph.
| 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 |
There was a problem hiding this comment.
Could not just be the termination of the application but also termination of a goroutine.
| - Using the new standards as a gate for code review of functional tests | ||
|
|
||
| ## Design | ||
| ### Coding Standards for Using contexts in Functional Tests |
There was a problem hiding this comment.
| ### Coding Standards for Using contexts in Functional Tests | |
| ### Coding Standards for using contexts in Functional Tests |
| ### 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 |
There was a problem hiding this comment.
I don't understand what using a context properly means? IIUC we are not using the appropriate contexts provided by Ginkgo.
There was a problem hiding this comment.
We are not using ginkgo contexts at all. We're using mostly context.Background() and sometimes context.TODO()
Not sure I'm familiar with the work Felix did. The suggestion is to add the coding standards to the kubevirt/kubevirt repo. My PR was merged but is about to be reverted, because there was no proper discussion about it: kubevirt/kubevirt#12148 |
Signed-off-by: Nahshon Unna-Tsameret <nunnatsa@redhat.com>
| Avoid using new contexts (e.g. `context.Background()` or `context.TODO()`). | ||
| Instead, prefer using the context injected by ginkgo. |
There was a problem hiding this comment.
Drop this paragraph, it duplicates what was already said above.
There was a problem hiding this comment.
This makes so much sense to me, not quite sure why it's so controversial...
@0xFelix do you still have any unresolved concern?
@fossedihelm any objection to me approving this?
| # Properly use contexts in functional tests | ||
|
|
||
| ## Overview | ||
| This proposal is about properly use golang contexts in KubeVirt functional tests. |
There was a problem hiding this comment.
| This proposal is about properly use golang contexts in KubeVirt functional tests. | |
| This proposal is about properly using 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 |
There was a problem hiding this comment.
| in golang, when writing an asynchronous logic. In a way, the KubeVirt code left | |
| in golang, when writing an asynchronous logic. In a way, the KubeVirt code is left |
?
It is controversial because in the current form of the tests, it will make them harder to read and add a lot of boilerplate code. We will also need all SIGs approval to enforce this. I’m unsure if all will be able to commit following this. |
| details: | ||
| [https://onsi.github.io/ginkgo/#spec-timeouts-and-interruptible-nodes](https://onsi.github.io/ginkgo/#spec-timeouts-and-interruptible-nodes) | ||
|
|
||
| ## Goals |
There was a problem hiding this comment.
Here I am missing the outcomes.
| The test code should use the contexts provided by ginkgo, rather than new contexts | ||
| like `context.Background()` or `context.TODO()`. | ||
|
|
||
| The following guidelines are base on the [ginkgo documentation](https://onsi.github.io/ginkgo/#spec-timeouts-and-interruptible-nodes). |
There was a problem hiding this comment.
| The following guidelines are base on the [ginkgo documentation](https://onsi.github.io/ginkgo/#spec-timeouts-and-interruptible-nodes). | |
| The following guidelines are based on the [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. |
Release note: