Add testcase for CLI

[huntersman]

What type of PR is this?
/kind enhancement

What this PR does / why we need it:
This pull request adds comprehensive unit tests for the CLI command package and introduces minor refactoring to improve testability and code quality. The main changes include the addition of new test files for core command functionalities, enhancements to support mocking and test data, and minor improvements to error handling and initialization logic.

Testing Improvements:

• Added new test files for the create, describe, and root commands, covering a wide range of scenarios including error cases, flag handling, and command aliases. (cli/kthena/cmd/create_test.go, cli/kthena/cmd/describe_test.go, cli/kthena/cmd/root_test.go) [1] [2] [3]
• Introduced a common test helper for command execution and embedded test data for templates to facilitate isolated and reproducible tests. (cli/kthena/cmd/common_test.go)

Refactoring for Testability:

• Refactored the getKthenaClient logic to use an overridable function variable (getKthenaClientFunc), enabling the injection of mock clients in tests. (cli/kthena/cmd/get.go)
• Made the templatesBasePath variable configurable, allowing tests to use embedded test data directories. (cli/kthena/cmd/templates.go) [1] [2]

Minor Code Improvements:

• Improved error handling in the root command execution by printing errors before exiting. (cli/kthena/cmd/root.go)
• Added a missing import for fmt in the root command implementation. (cli/kthena/cmd/root.go)

These changes collectively increase the reliability of the CLI commands and make the codebase easier to maintain and extend with future features.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

[volcano-sh-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign git-malu for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• cli/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[gemini-code-assist]

Summary of Changes

Hello @huntersman, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request significantly enhances the robustness and maintainability of the CLI by introducing extensive unit tests for its core command functionalities. It also includes strategic refactoring to improve testability, such as enabling client mocking and configurable template paths, alongside minor code quality improvements.

Highlights

• Comprehensive Unit Tests: New unit tests have been added for the create, describe, and root CLI commands, covering various scenarios including error handling, flag parsing, and command aliases.
• Improved Testability: The getKthenaClient logic was refactored to allow mocking of the client in tests, and the templatesBasePath was made configurable for using embedded test data.
• Test Helper and Data: A common test helper (executeCommand) was introduced for executing Cobra commands and capturing output, along with embedded test data for templates.
• Minor Code Enhancements: Error handling in the root command's execution was improved to print errors before exiting, and a missing fmt import was added.
• New Test Templates: Several new YAML template files were added under testdata/helm/templates to support the new unit tests.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request significantly improves the project's quality by adding a comprehensive suite of unit tests for the CLI commands. The refactoring to enable mocking of the Kubernetes client is a commendable change that follows best practices for testability. I've identified one critical issue regarding a syntax error in a test data file and a couple of medium-severity issues related to unhandled errors in test helper functions. Addressing these points will further strengthen the reliability of the new tests.

[Copilot]

Pull request overview

This PR adds comprehensive unit tests for the CLI command package and introduces refactoring to improve testability. The changes enable better testing coverage for core command functionalities and set up infrastructure for reliable CLI testing.

• Introduces test infrastructure with embedded test data and command execution helpers
• Adds extensive test coverage for create, describe, get, and root commands
• Refactors code for improved testability through dependency injection patterns

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
cli/kthena/main.go	Removes placeholder copyright comment block
cli/kthena/cmd/root.go	Adds missing fmt import and improves error handling in Execute function by printing errors before exit
cli/kthena/cmd/get.go	Refactors getKthenaClient to use function variable for test mocking
cli/kthena/cmd/templates.go	Makes templatesBasePath configurable to support test data directories
cli/kthena/cmd/common_test.go	Adds test infrastructure with embedded test data and command execution helper
cli/kthena/cmd/create_test.go	Adds comprehensive tests for create manifest command including flag handling and template rendering
cli/kthena/cmd/describe_test.go	Adds tests for describe command with template and resource description scenarios
cli/kthena/cmd/get_test.go	Adds tests for get commands including templates, model-boosters, and various resource types
cli/kthena/cmd/root_test.go	Adds tests for root command, subcommands, and command aliases
cli/kthena/cmd/templates_test.go	Adds tests for template listing, existence checking, and content retrieval
cli/kthena/cmd/testdata/helm/templates/deepseek-ai/DeepSeek-R1-Distill-Qwen-7B.yaml	Adds test template for DeepSeek R1 Distill Qwen 7B model
cli/kthena/cmd/testdata/helm/templates/deepseek-ai/DeepSeek-R1-Distill-Qwen-32B.yaml	Adds test template for DeepSeek R1 Distill Qwen 32B model
cli/kthena/cmd/testdata/helm/templates/Qwen/Qwen3-8B.yaml	Adds test template for Qwen3 8B model
cli/kthena/cmd/testdata/helm/templates/Qwen/Qwen3-32B.yaml	Adds test template for Qwen3 32B model

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[huntersman]

Since the CLI is relatively simple to use, I suggest we focus on end-to-end (e2e) tests using the compiled binary, rather than unit tests.

E2E tests have limited resources, so they can't run the CLI models.

[hzxuzhonghu]

@huntersman we can test it using a fake image, just check the pod status?

[git-malu]

Your current approach works, but it's not the best practice. It uses global variable injection which has several drawbacks. Let me show you better alternatives:

Problems with Global Variable Injection:

1. Test pollution: Tests can interfere with each other
2. Concurrency issues: Can't run tests in parallel
3. Hidden dependencies: Hard to track what's being mocked
4. Reset complexity: Need careful defer cleanup
5. Not thread-safe: Dangerous in concurrent code

Better Practice: Dependency Injection Patterns

Pattern 1: Constructor Injection (Recommended)

// cmd/get.go
type GetCommand struct {
    client versioned.Interface
    // other dependencies
}

// Constructor accepts dependencies
func NewGetCommand(client versioned.Interface) *GetCommand {
    return &GetCommand{client: client}
}

func (c *GetCommand) Run(args []string) error {
    // Use c.client directly
    boosters, err := c.client.WorkloadV1alpha1().ModelBoosters("").List(context.Background(), metav1.ListOptions{})
    // ...
}

// In main.go
func main() {
    client, _ := getRealClient()
    cmd := NewGetCommand(client)
    cmd.Run(os.Args[1:])
}

// In tests
func TestGetCommand(t *testing.T) {
    fakeClient := fake.NewClientset(testData...)
    cmd := NewGetCommand(fakeClient)
    // Test cmd.Run()
}

Pattern 2: Interface-based Factory

// internal/client/factory.go
type ClientFactory interface {
    GetClient() (versioned.Interface, error)
}

type RealClientFactory struct{}

func (f *RealClientFactory) GetClient() (versioned.Interface, error) {
    config, err := clientcmd.BuildConfigFromFlags("", clientcmd.RecommendedHomeFile)
    if err != nil {
        return nil, err
    }
    return versioned.NewForConfig(config)
}

// In commands
type GetCommand struct {
    clientFactory ClientFactory
}

func NewGetCommand(factory ClientFactory) *GetCommand {
    return &GetCommand{clientFactory: factory}
}

func (c *GetCommand) Run(args []string) error {
    client, err := c.clientFactory.GetClient()
    if err != nil {
        return err
    }
    // Use client...
}

// Mock factory for tests
type MockClientFactory struct {
    Client versioned.Interface
    Err    error
}

func (m *MockClientFactory) GetClient() (versioned.Interface, error) {
    return m.Client, m.Err
}

// Test
func TestGetCommand(t *testing.T) {
    fakeClient := fake.NewClientset(testData...)
    factory := &MockClientFactory{Client: fakeClient}
    cmd := NewGetCommand(factory)
    // Test...
}

Pattern 3: Functional Options (Most Flexible)

// cmd/options.go
type CommandOption func(*CommandConfig)

type CommandConfig struct {
    Client versioned.Interface
    Output io.Writer
    // other config
}

func WithClient(client versioned.Interface) CommandOption {
    return func(c *CommandConfig) {
        c.Client = client
    }
}

func WithOutput(w io.Writer) CommandOption {
    return func(c *CommandConfig) {
        c.Output = w
    }
}

// cmd/get.go
func NewGetCommand(opts ...CommandOption) *GetCommand {
    config := &CommandConfig{
        Output: os.Stdout, // default
    }
  
    for _, opt := range opts {
        opt(config)
    }
  
    return &GetCommand{
        client: config.Client,
        output: config.Output,
    }
}

// Usage in production
func main() {
    client, _ := getRealClient()
    cmd := NewGetCommand(
        WithClient(client),
        WithOutput(os.Stdout),
    )
}

// Usage in tests
func TestGetCommand(t *testing.T) {
    fakeClient := fake.NewClientset(testData...)
    var buf bytes.Buffer
  
    cmd := NewGetCommand(
        WithClient(fakeClient),
        WithOutput(&buf),
    )
  
    // Test and inspect buf
}

Pattern 4: Context/Request-scoped Injection

// internal/context/context.go
type RequestContext struct {
    Client versioned.Interface
    // other request-scoped dependencies
}

// Commands accept context
func RunGetCommand(ctx *RequestContext, args []string) error {
    boosters, err := ctx.Client.WorkloadV1alpha1().ModelBoosters("").List(context.Background(), metav1.ListOptions{})
    // ...
}

// In main
func main() {
    client, _ := getRealClient()
    ctx := &RequestContext{Client: client}
    RunGetCommand(ctx, os.Args[1:])
}

// In tests
func TestGetCommand(t *testing.T) {
    fakeClient := fake.NewClientset(testData...)
    ctx := &RequestContext{Client: fakeClient}
    err := RunGetCommand(ctx, []string{"get", "modelboosters"})
    // assert...
}

Refactoring Your Current Code:

From This (Current):

// ❌ Global variable approach
var getKthenaClientFunc = getKthenaClientImpl

func getModelBoosters() error {
    client, err := getKthenaClientFunc()  // Implicit dependency
    // ...
}

To This (Better):

// ✅ Explicit dependency
type ModelBoosterService struct {
    Client versioned.Interface
}

func NewModelBoosterService(client versioned.Interface) *ModelBoosterService {
    return &ModelBoosterService{Client: client}
}

func (s *ModelBoosterService) ListBoosters(namespace string) ([]workloadv1alpha1.ModelBooster, error) {
    boosters, err := s.Client.WorkloadV1alpha1().ModelBoosters(namespace).List(context.Background(), metav1.ListOptions{})
    // ...
}

// Command uses service
type GetModelBoostersCommand struct {
    service *ModelBoosterService
}

func NewGetModelBoostersCommand(service *ModelBoosterService) *GetModelBoostersCommand {
    return &GetModelBoostersCommand{service: service}
}

func (c *GetModelBoostersCommand) Run(args []string) error {
    boosters, err := c.service.ListBoosters("")
    // Format and output...
}

Complete Example with Cobra (Common for Go CLIs):

// cmd/root.go
package cmd

import (
    "github.com/spf13/cobra"
    "k8s.io/client-go/kubernetes"
)

// Root command holds shared dependencies
type RootCmd struct {
    *cobra.Command
    k8sClient kubernetes.Interface
}

func NewRootCmd(client kubernetes.Interface) *RootCmd {
    root := &RootCmd{
        Command: &cobra.Command{
            Use:   "mycli",
            Short: "My Kubernetes CLI",
        },
        k8sClient: client,
    }
  
    // Add subcommands with dependencies
    root.AddCommand(NewGetCmd(root.k8sClient))
    root.AddCommand(NewApplyCmd(root.k8sClient))
  
    return root
}

// cmd/get.go
func NewGetCmd(client kubernetes.Interface) *cobra.Command {
    return &cobra.Command{
        Use:   "get [resource]",
        Short: "Get resources",
        RunE: func(cmd *cobra.Command, args []string) error {
            // Client is available via closure
            pods, err := client.CoreV1().Pods("").List(cmd.Context(), metav1.ListOptions{})
            // ...
            return nil
        },
    }
}

// main.go
func main() {
    client := createRealClient()
    rootCmd := cmd.NewRootCmd(client)
    rootCmd.Execute()
}

// tests/cmd/get_test.go
func TestGetCommand(t *testing.T) {
    fakeClient := fake.NewSimpleClientset(testPods...)
    cmd := NewGetCmd(fakeClient)
  
    // Test command execution
    buf := new(bytes.Buffer)
    cmd.SetOut(buf)
    cmd.SetArgs([]string{"pods"})
  
    err := cmd.Execute()
    assert.NoError(t, err)
    assert.Contains(t, buf.String(), "test-pod")
}

Best Practice Recommendations:

For Unit Tests:

// Test individual components with mocked dependencies
func TestModelBoosterService_ListBoosters(t *testing.T) {
    mockClient := &MockK8sClient{}
    service := NewModelBoosterService(mockClient)
  
    // Test service logic in isolation
    result, err := service.ListBoosters("default")
    // assertions...
}

For Integration Tests:

// Test command execution with fake client
func TestGetModelBoostersCommand_Integration(t *testing.T) {
    fakeClient := fake.NewClientset(testBoosters...)
    service := NewModelBoosterService(fakeClient)
    cmd := NewGetModelBoostersCommand(service)
  
    // Test full command flow
    output, err := cmd.Run([]string{})
    // assertions...
}

Migration Strategy (If You Can't Refactor Everything):

1. Start new code with proper DI
2. Wrap old code in testable interfaces
3. Gradually refactor high-priority commands

// Temporary adapter for legacy code
type LegacyCommandAdapter struct {
    getClientFunc func() (versioned.Interface, error)
}

func NewLegacyCommandAdapter(clientFunc func() (versioned.Interface, error)) *LegacyCommandAdapter {
    return &LegacyCommandAdapter{getClientFunc: clientFunc}
}

func (a *LegacyCommandAdapter) Run() error {
    // Calls your existing global function
    client, err := a.getClientFunc()
    // Run legacy logic...
}

// Test with mock
func TestLegacyCommand(t *testing.T) {
    fakeClient := fake.NewClientset()
    adapter := NewLegacyCommandAdapter(func() (versioned.Interface, error) {
        return fakeClient, nil
    })
    // Test adapter...
}

Key Principles:

1. Explicit over implicit - Dependencies should be visible
2. Interfaces over concretions - Program to interfaces
3. Composition over inheritance - Build with small, testable parts
4. Single Responsibility - Each component does one thing

Your current approach works, but moving to explicit dependency injection will make your code:

• ✅ More testable
• ✅ More maintainable
• ✅ More readable
• ✅ More flexible
• ✅ Less bug-prone

Start with Pattern 1 (Constructor Injection) for new code - it's the simplest and most effective for CLI tools.

[git-malu]

Since the CLI is relatively simple to use, I suggest we focus on end-to-end (e2e) tests using the compiled binary, rather than unit tests.

E2E tests have limited resources, so they can't run the CLI models.

We can revisit E2E testing once we have the core test foundation established.
For now let's focus on the unit test and integration test.

[hzxuzhonghu]

/lgtm

@git-malu any other comment