🐛 Fix unit tests, improve debuggability #3126
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/M
sbueringer
force-pushed
the
pr-improve-tests
branch
from
f6f0b9e
to
d9fd3b1
Compare
| for _, pSvcAccount := range pSvcAccounts { | ||
| pSvcAccountNames = append(pSvcAccountNames, pSvcAccount.Name) | ||
| } | ||
| log.V(5).Info(fmt.Sprintf("Reconcile ProviderServiceAccounts: %v", strings.Join(pSvcAccountNames, ","))) |
There was a problem hiding this comment.
Just printing the ProviderServiceAccounts, this should make it easier to catch race conditions where there might not be any ProviderServiceAccounts
| guestClient, err := tracker.GetClient(ctx, client.ObjectKeyFromObject(intCtx.Cluster)) | ||
| Expect(err).ToNot(HaveOccurred()) | ||
| // Note: Create a Service informer, so the test later doesn't fail if this doesn't work. | ||
| Expect(guestClient.List(ctx, &corev1.ServiceList{}, client.InNamespace(metav1.NamespaceDefault))).To(Succeed()) |
There was a problem hiding this comment.
Saw some errors in the logs. Not sure if the test failures are due to envtest not coming up (probalby not, but shouldn't hurt to have this explicit check here)
| deleteTestResource(ctx, intCtx.Client, pSvcAccount) | ||
| deleteTestResource(ctx, intCtx.Client, intCtx.VSphereCluster) |
There was a problem hiding this comment.
Just some explicit cleanup. Deleting a namespace in testenv doesn't lead to all object in the namespace being deleted (there is no kube-controller-manager)
| @@ -60,7 +62,15 @@ import ( | |||
| ) | |||
|
|
|||
| func init() { | |||
| // Set log level 5 as default for testing (default for prod is 2). | |||
There was a problem hiding this comment.
Log level 5 gives us more verbose logs of our controllers, but also a lot of useful logs from CR around Reconciles
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: e1af9ed6dade9a60dfe6d28b8eddeca975285451
|
Signed-off-by: Stefan Büringer [email protected]
sbueringer
force-pushed
the
pr-improve-tests
branch
from
d9fd3b1
to
612740d
Compare
k8s-ci-robot
removed
the
lgtm
|
/lgtm /hold if @fabriziopandini wants to take a look too |
k8s-ci-robot
added
do-not-merge/hold
|
LGTM label has been added. Git tree hash: 6aa47c6886b812015b6d39c47cb8858c65f69e52
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chrischdi 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
|
/hold cancel Happy to follow-up if necessary (want new data :)) |
k8s-ci-robot
removed
the
do-not-merge/hold
What this PR does / why we need it:
This PR should:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #