Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use the cached client for the controller tests #2501

Closed
wants to merge 1 commit into from
Closed

Use the cached client for the controller tests #2501

wants to merge 1 commit into from

Conversation

shuheiktgw
Copy link
Contributor

Problem Statement

SSIA. It would be better if we use the cached client instead of the uncached one (k8sClient) for the controller tests since that is what we use in the actual environment. Thanks!

Related Issue

N/A

Proposed Changes

Use the cached client instead.

Checklist

  • I have read the contribution guidelines
  • All commits are signed with git commit --signoff
  • My changes have reasonable test coverage
  • All tests pass with make test
  • I ensured my PR is ready for review with make reviewable

@shuheiktgw shuheiktgw requested a review from a team as a code owner July 15, 2023 07:17
@shuheiktgw shuheiktgw requested review from gusfcarvalho and removed request for a team July 15, 2023 07:17
shuheiktgw
shuheiktgw commented Jul 15, 2023
View reviewed changes
pkg/controllers/pushsecret/pushsecret_controller_test.go
@@ -38,7 +38,7 @@ import (

var (
fakeProvider *fake.Client
timeout = time.Second * 10
timeout = time.Second * 20
Copy link
Contributor Author

@shuheiktgw shuheiktgw Jul 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like some of the tests take more than 10 seconds if we use the cached client 😢

Use the cached client for the controller tests
8cec658 
Signed-off-by: shuheiktgw <s-kitagawa@mercari.com>
@shuheiktgw shuheiktgw force-pushed the use_cached_client_for_tests branch from 834a206 to 8cec658 Compare July 15, 2023 07:37
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.6% 0.6% Duplication

shuheiktgw
shuheiktgw commented Jul 15, 2023
View reviewed changes
pkg/controllers/externalsecret/externalsecret_controller_test.go
@@ -45,7 +45,7 @@ var (
fakeProvider *fake.Client
metric dto.Metric
metricDuration dto.Metric
timeout = time.Second * 10
timeout = time.Second * 20
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same with the PushSecret, and it seems some tests take more than 10 seconds...

@moolen
Copy link
Member

moolen commented Jul 17, 2023

We've used the cached client in the past and ran into the issue that you get unpredictable results in tests which is hard to work with (hence the timeout increase 😃).
IMO we shouldn't use the cached client. Though i get your point, i just think that our e2e tests cover exactly that: running the controllers in an actual cluster and verify that it works as expected - with a cached client and all that other stuff that isn't covered by the controller tests.

WRT:

external-secrets/pkg/controllers/externalsecret/suite_test.go

Lines 82 to 89 in e5f953b

// do not use k8sManager.GetClient()
// see https://github.com/kubernetes-sigs/controller-runtime/issues/343#issuecomment-469435686
k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
Expect(k8sClient).ToNot(BeNil())
Expect(err).ToNot(HaveOccurred())
err = (&Reconciler{
Client: k8sClient,

@shuheiktgw
Copy link
Contributor Author

Hmm, I see. That makes sense! Thank you and I'm closing the PR 🙂

@shuheiktgw shuheiktgw closed this Jul 19, 2023
@shuheiktgw shuheiktgw deleted the use_cached_client_for_tests branch July 19, 2023 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gusfcarvalho gusfcarvalho Awaiting requested review from gusfcarvalho gusfcarvalho is a code owner automatically assigned from external-secrets/maintainers

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shuheiktgw @moolen