chore: add additional E2E tests for local user management #1931
Conversation
WalkthroughAdds an exported Secret matcher Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Ginkgo test
participant K8s as Kubernetes API
participant Reconciler as Controller/Reconciler
participant Fixtures as Test fixtures
rect rgb(240,248,255)
Note right of Test: Setup initial resources\n(Secret, ConfigMap, ArgoCD CR)
Test ->> K8s: Create Secret & ConfigMap
K8s -->> Test: Resources created
end
rect rgb(245,255,240)
Note right of Reconciler: Reconciliation phases\n(ensure users/config)
Reconciler ->> K8s: Read resources
Reconciler ->> K8s: Create/Update managed Secrets & ConfigMap
K8s -->> Reconciler: ACK
Test ->> Fixtures: verifyConfigurationIsAsExpected(stage: initial)
Fixtures -->> Test: assertions pass/fail
end
rect rgb(255,250,240)
Note right of Test: Simulate deletions and changes
Test ->> K8s: Delete local user Secret (manual)
Test ->> K8s: Delete/Modify argocd-cm
K8s -->> Reconciler: informs change
Reconciler ->> K8s: Recreate/cleanup resources
Test ->> Fixtures: verifyConfigurationIsAsExpected(stage: after-change)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/ginkgo/parallel/1-052_validate_local_user_management_test.go (2)
173-176: Remove redundant verification calls.Lines 173-174 and lines 175-176 perform identical checks for
argocdSecretexistence. The second pair is redundant.Apply this diff to remove the duplicate checks:
By("verifying argocd-secret Secret contains token for bob") Eventually(argocdSecret).Should(k8sFixture.ExistByName()) Consistently(argocdSecret, "5s", "1s").Should(k8sFixture.ExistByName()) -Eventually(argocdSecret).Should(k8sFixture.ExistByName()) -Consistently(argocdSecret, "5s", "1s").Should(k8sFixture.ExistByName()) Eventually(argocdSecret).Should(secretFixture.HaveNonEmptyKeyValue("accounts.bob.tokens")) Consistently(argocdSecret, "5s", "1s").Should(secretFixture.HaveNonEmptyKeyValue("accounts.bob.tokens"))
225-231: Remove unnecessary type conversions.Lines 229-230 use
string()casts on string literals, which is redundant since the literals are already strings.Apply this diff:
configmap.Update(argocdCMConfigMap, func(cm *corev1.ConfigMap) { if cm.Data == nil { cm.Data = map[string]string{} } - cm.Data["accounts.alice"] = string("apiKey") - cm.Data["accounts.alice.enabled"] = string("true") + cm.Data["accounts.alice"] = "apiKey" + cm.Data["accounts.alice.enabled"] = "true" })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/ginkgo/fixture/secret/fixture.go(1 hunks)tests/ginkgo/parallel/1-052_validate_local_user_management_test.go(2 hunks)tests/ginkgo/parallel/1-053_validate_local_user_token_renewal_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ginkgo/parallel/1-052_validate_local_user_management_test.go (5)
tests/ginkgo/fixture/argocd/fixture.go (1)
Update(23-44)tests/ginkgo/fixture/k8s/fixture.go (3)
ExistByName(101-118)Update(144-161)NotExistByName(122-141)tests/ginkgo/fixture/secret/fixture.go (4)
HaveStringDataKeyValue(74-87)HaveNonEmptyKeyValue(58-71)Update(19-35)NotHaveDataKey(102-109)tests/ginkgo/fixture/configmap/fixture.go (3)
HaveStringDataKeyValue(49-74)Update(20-36)NotHaveStringDataKey(77-83)tests/ginkgo/fixture/fixture.go (1)
CreateRandomE2ETestNamespaceWithCleanupFunc(118-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build-operator
- GitHub Check: build
- GitHub Check: Run end-to-end tests (v1.27.1)
- GitHub Check: Run golangci-lint and gosec
🔇 Additional comments (5)
tests/ginkgo/fixture/secret/fixture.go (1)
101-109: LGTM!The
NotHaveDataKeymatcher correctly implements negative assertion for Secret data keys, following the established pattern of other matchers in this file and mirroring theNotHaveStringDataKeymatcher in the ConfigMap fixture.tests/ginkgo/parallel/1-053_validate_local_user_token_renewal_test.go (1)
70-70: LGTM!Improved wording for clarity.
tests/ginkgo/parallel/1-052_validate_local_user_management_test.go (3)
26-27: LGTM!The new imports support UUID generation for unique test data and pointer utilities for Kubernetes API fields, both used appropriately in the test scenarios.
Also applies to: 33-33, 36-36
95-134: Well-structured refactoring!The
verifyConfigurationIsAsExpectedhelper function effectively reduces code duplication and makes the test flow clearer. The use ofEventuallyandConsistentlymatchers is appropriate for E2E testing, ensuring resources are not only created but remain stable.
244-282: Excellent test coverage for reconciler cleanup behavior!The manually crafted Secret with proper labels, owner references, and realistic data effectively validates that the reconciler correctly removes orphaned local user resources. The use of
ptr.To()for bool pointers anduuid.NewUUID()for unique test data follows Kubernetes best practices.
Signed-off-by: Jonathan West <jonwest@redhat.com>
jgwest
force-pushed
the
add-local-user-tests-oct-2025
branch
from
8a023e1 to
db89e22
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/ginkgo/parallel/1-052_validate_local_user_management_test.go (2)
102-102: Fix spacing in By statement concatenations.The string concatenations are missing a space before the dash, resulting in messages like "initial creation- verifying..." instead of "initial creation - verifying...".
Apply this diff:
- By(description + "- verifying Argo CD argocd-cm ConfigMap references user, and user is enabled") + By(description + " - verifying Argo CD argocd-cm ConfigMap references user, and user is enabled")- By(description + "- verifying argocd-secret Secret contains token for user") + By(description + " - verifying argocd-secret Secret contains token for user")Also applies to: 108-108
136-180: LGTM: Comprehensive user switching verification.The test thoroughly validates that switching from alice to bob properly removes old resources and creates new ones in both ConfigMap and Secret.
Minor consistency suggestion: Consider adding explicit timeouts to all
Consistentlycalls (lines 163-164, 179) to match the pattern used elsewhere ("5s", "1s"), though the default timeout is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
tests/ginkgo/fixture/secret/fixture.go(1 hunks)tests/ginkgo/parallel/1-052_validate_local_user_management_test.go(2 hunks)tests/ginkgo/parallel/1-053_validate_local_user_token_renewal_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/ginkgo/fixture/secret/fixture.go
- tests/ginkgo/parallel/1-053_validate_local_user_token_renewal_test.go
🧰 Additional context used
🧬 Code graph analysis (1)
tests/ginkgo/parallel/1-052_validate_local_user_management_test.go (5)
tests/ginkgo/fixture/argocd/fixture.go (2)
BeAvailable(47-49)Update(23-44)tests/ginkgo/fixture/k8s/fixture.go (3)
ExistByName(101-118)Update(144-161)NotExistByName(122-141)tests/ginkgo/fixture/secret/fixture.go (4)
HaveStringDataKeyValue(74-87)HaveNonEmptyKeyValue(58-71)Update(19-35)NotHaveDataKey(102-109)tests/ginkgo/fixture/configmap/fixture.go (3)
HaveStringDataKeyValue(49-74)Update(20-36)NotHaveStringDataKey(77-83)tests/ginkgo/fixture/fixture.go (1)
CreateRandomE2ETestNamespaceWithCleanupFunc(118-122)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build
- GitHub Check: Run golangci-lint and gosec
- GitHub Check: build-operator
- GitHub Check: Run end-to-end tests (v1.27.1)
🔇 Additional comments (5)
tests/ginkgo/parallel/1-052_validate_local_user_management_test.go (5)
26-27: LGTM: New imports are properly utilized.The new imports (
uuid,ptr, andsecretFixturealias) are appropriately used throughout the expanded test scenarios for generating unique tokens, creating owner references, and Secret-related assertions.Also applies to: 35-35
73-73: LGTM: Improved test step clarity.The wording adjustment enhances readability and consistency with other test steps.
95-133: LGTM: Excellent refactoring with the verification helper.The
verifyConfigurationIsAsExpectedhelper effectively eliminates duplication and provides consistent verification logic across multiple test stages. The staged verification approach (initial creation, after deletions) demonstrates good test design.
183-195: LGTM: Proper cleanup verification when localUsers is removed.The test correctly validates that removing the
localUsersfield cleans up the user Secret while preserving the argocd-cm ConfigMap.
198-279: LGTM: Comprehensive leftover resource cleanup test.This new test case provides excellent coverage for ensuring the reconciler properly cleans up manually created resources that don't correspond to ArgoCD CR configuration. The ArgoCD CRD API version at line 254 (
argoproj.io/v1beta1) is the current version, having been upgraded from v1alpha1.
1 participant
What type of PR is this?
/kind chore
What does this PR do / why we need it:
Have you updated the necessary documentation?
Summary by CodeRabbit