🐛 Refactor kubeconfig tests: enhance structure and add comprehensive test cases for context management #650
🐛 Refactor kubeconfig tests: enhance structure and add comprehensive test cases for context management #650adity1raut wants to merge 1 commit intokubestellar:mainfrom
Conversation
…st cases for context management Signed-off-by: adity1raut <araut7798@gmail.com>
kubestellar-prow
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. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome to KubeStellar! 🚀 Thank you for submitting this Pull Request. Before your PR can be merged, please ensure: ✅ DCO Sign-off - All commits must be signed off with ✅ PR Title - Must start with an emoji: ✨ (feature), 🐛 (bug fix), 📖 (docs), 🌱 (infra/tests), Getting Started with KubeStellar: Contributor Resources:
🌟 Help KubeStellar Grow - We Need Adopters! Our roadmap is driven entirely by adopter feedback. Whether you're using KubeStellar yourself or know someone who could benefit from multi-cluster Kubernetes: 📋 Take our Multi-Cluster Survey - Share your use cases and help shape our direction! A maintainer will review your PR soon. Feel free to ask questions in the comments or on Slack! |
kubestellar-prow
bot
added
the
needs-ok-to-test
|
Hi @adity1raut. Thanks for your PR. I'm waiting for a kubestellar 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. DetailsInstructions 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. |
kubestellar-prow
bot
added
the
size/XL
|
/cc @MikeSpreitzer |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the kubeconfig test file (pkg/kubeconfig/kubeconfig_test.go) by completely rewriting it with a more comprehensive and structured approach to testing kubeconfig context management functionality.
Changes:
- Replaced minimal test coverage with comprehensive table-driven tests for all public functions
- Added tests for
adjustConfigKeys,merge,AssignControlPlaneToContext,DeleteAll,RenameKey,SwitchContext,SetHostingClusterContext,GetHostingClusterContext,IsHostingClusterContextSet, andSwitchToHostingClusterContext - Expanded test scenarios to cover positive cases, negative cases, and edge cases for each function
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| validate: func(t *testing.T, kconf *clientcmdapi.Config, cpName string) { | ||
| expectedContext := certs.GenerateContextName(cpName) | ||
| if kconf.CurrentContext != expectedContext { | ||
| t.Errorf("expected current context %s, got %s", expectedContext, kconf.CurrentContext) | ||
| } | ||
| }, |
There was a problem hiding this comment.
The validation for the K3s control plane type test case is incomplete. It only checks the CurrentContext, but should also verify that the clusters, authInfos, and contexts have been properly renamed (similar to the OCM test case validation). The adjustConfigKeys function renames all three maps for K3s type, so all should be validated.
| validate: func(t *testing.T, kconf *clientcmdapi.Config, cpName string) { | ||
| expectedCluster := certs.GenerateClusterName(cpName) | ||
| expectedAuthInfo := certs.GenerateAuthInfoAdminName(cpName) | ||
| expectedContext := certs.GenerateContextName(cpName) | ||
|
|
||
| if _, ok := kconf.Clusters[expectedCluster]; !ok { | ||
| t.Errorf("expected cluster %s not found", expectedCluster) | ||
| } | ||
| if _, ok := kconf.AuthInfos[expectedAuthInfo]; !ok { | ||
| t.Errorf("expected authInfo %s not found", expectedAuthInfo) | ||
| } | ||
| if _, ok := kconf.Contexts[expectedContext]; !ok { | ||
| t.Errorf("expected context %s not found", expectedContext) | ||
| } | ||
| if kconf.CurrentContext != expectedContext { | ||
| t.Errorf("expected current context %s, got %s", expectedContext, kconf.CurrentContext) | ||
| } | ||
| }, |
There was a problem hiding this comment.
The test validation should also verify that the context entry has the correct Cluster and AuthInfo references. The adjustConfigKeys function creates a new context with specific Cluster and AuthInfo values (lines 66-69 of kubeconfig.go), but the test only checks if the context exists, not if it has the correct internal structure.
| name: "successfully assign control plane to existing context", | ||
| kconf: &clientcmdapi.Config{ | ||
| Contexts: map[string]*clientcmdapi.Context{ | ||
| "test-context": { | ||
| Cluster: "test-cluster", | ||
| AuthInfo: "test-user", | ||
| }, | ||
| }, | ||
| }, | ||
| cpName: "test-cp", | ||
| ctxName: "test-context", | ||
| expectError: false, | ||
| }, |
There was a problem hiding this comment.
The successful test case for AssignControlPlaneToContext does not validate that the control plane was actually assigned. The test should verify that the context's Extensions field contains the control plane name after the assignment.
| validate: func(t *testing.T, result *clientcmdapi.Config) { | ||
| if result.Extensions == nil { | ||
| t.Error("expected extensions to be set") | ||
| } | ||
| }, |
There was a problem hiding this comment.
The validation for the merge test case "merge without hosting cluster context - should set it" is incomplete. While it checks that Extensions is not nil, it should also verify that the hosting cluster context was actually set to the correct value (base-context) by calling GetHostingClusterContext or checking the extension data directly.
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| ctx, err := GetHostingClusterContext(tt.kconf) | ||
| if (err != nil) != tt.expectError { | ||
| t.Errorf("expected error: %v, got: %v", tt.expectError, err) | ||
| } | ||
| if err == nil && ctx != tt.expectedCtx { | ||
| t.Errorf("expected context %s, got %s", tt.expectedCtx, ctx) | ||
| } | ||
| }) |
There was a problem hiding this comment.
The test defines an errorContains field but never validates it. The test should check that the error message contains the expected substring when an error occurs. Add validation like: if err != nil && tt.errorContains != "" && !strings.Contains(err.Error(), tt.errorContains) { ... }
| CurrentContext: "base-context", | ||
| Extensions: map[string]runtime.Object{ | ||
| ExtensionKubeflexKey: &runtime.Unknown{ | ||
| Raw: []byte(`{"hostingClusterContextName":"base-context"}`), |
There was a problem hiding this comment.
The JSON field name used in the test is incorrect. According to the KubeflexExtensions struct definition, the JSON tag for HostingClusterContextName is "hosting-cluster-ctx-name" (kebab-case), but this test uses "hostingClusterContextName" (camelCase). This should be changed to match the struct definition.
| Raw: []byte(`{"hostingClusterContextName":"base-context"}`), | |
| Raw: []byte(`{"hosting-cluster-ctx-name":"base-context"}`), |
| Contexts: map[string]*clientcmdapi.Context{}, | ||
| Extensions: map[string]runtime.Object{ | ||
| ExtensionKubeflexKey: &runtime.Unknown{ | ||
| Raw: []byte(`{"hostingClusterContextName":"non-existent"}`), |
There was a problem hiding this comment.
The JSON field name used in the test is incorrect. According to the KubeflexExtensions struct definition, the JSON tag for HostingClusterContextName is "hosting-cluster-ctx-name" (kebab-case), but this test uses "hostingClusterContextName" (camelCase). This should be changed to match the struct definition.
| Raw: []byte(`{"hostingClusterContextName":"non-existent"}`), | |
| Raw: []byte(`{"hosting-cluster-ctx-name":"non-existent"}`), |
| validate: func(t *testing.T, kconf *clientcmdapi.Config, cpName string) { | ||
| expectedContext := certs.GenerateContextName(cpName) | ||
| if kconf.CurrentContext != expectedContext { | ||
| t.Errorf("expected current context %s, got %s", expectedContext, kconf.CurrentContext) | ||
| } | ||
| }, |
There was a problem hiding this comment.
The validation for the VCluster control plane type test case is incomplete. It only checks the CurrentContext, but should also verify that the clusters, authInfos, and contexts have been properly renamed (similar to the OCM test case validation). The adjustConfigKeys function renames all three maps for VCluster type, so all should be validated.
❌ PR Title Verification FailedYour PR title does not follow the required format. Current title: Required FormatPR titles must start with one of these emoji prefixes:
How to FixEdit your PR title to start with the appropriate emoji. For example:
You can edit the title by clicking the Edit button next to your PR title. This comment was posted to help you fix the PR title format. |
adity1raut
changed the title
adity1raut
changed the title
|
/ok-to-test |
kubestellar-prow
bot
added
ok-to-test
…st cases for context management
Summary
Related issue(s)
Fixes #632