Do not cache config if not explicitly set #413
Conversation
This commit fixes a bug where the config object was shared between tests. When running tests in parallel, it was then impossible to rely on fields like the namespace because they could have been overwritten by another test. Also, it led to tests using `-race` to fail because the shared config.klient object that were updating the same fields when initializing the client. This commit creates a new `deepCopyConfig` method that allows to create a deep copy of the config object. This way, each test can have its own without impacting the others. Now the config has the following lifecycle: - It is created when a testEnv is created - Each test uses a child testEnv that inherits the main testEnv's config - Each feature uses a child testEnv that inherits the test's testEnv's config This way, a feature inherits all the changes made in BeforeEach functions while not impacting the other features.
Signed-off-by: Philippe Scorsolini <p.scorsolini@gmail.com>
k8s-ci-robot
added
kind/bug
k8s-ci-robot
added
the
sig/testing
k8s-ci-robot
added
cncf-cla: yes
|
Hi @phisco. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/L
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
…-if-not-explicitly-set
|
/retest |
|
@vladimirvivien, can you trigger the tests? 🙏 |
|
trying pinging @harshanarayana 🙏 😬 |
|
Looking into it @phisco |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/retest |
|
@phisco @Fricounet Do we still need to keep #396 alive? |
|
Didn't want to hijack the original pr, just wanted to see if it broke tests, I'm fine closing this one and porting only my commit to #389 🙏 |
|
/lgtm @vladimirvivien @cpanato mind taking a second look on this ? |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
Thanks @phisco @harshanarayana @cpanato /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: phisco, vladimirvivien The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
And thanks @Fricounet for the initial implementation! |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #352
Fixes #384
Based on #396
Special notes for your reviewer:
Based on #396, but attempting to solve #396 (comment).
This way we won't have any concurrent writes to the config client, as we are not caching it, the internal client is just for user-injected clients, so that a user can continue avoiding to provide a kubeconfig altogether if not needed. We might want to adjust naming.
Does this PR introduce a user-facing change?
The client won't be cached anymore, I wouldn't call it a user-facing change.
Additional documentation e.g., Usage docs, etc.: