refactor(librarian): Inject client factories and add test for generate default - 2 (#1923)

This PR refactors `newCommandRunner` in `command.go` to accept
`GitHubClientFactory` and `ContainerClientFactory` as arguments,
improving testability by enabling direct injection of mock clients.

Key changes include:

* Defined `GitHubClientFactory` and `ContainerClientFactory` types in
`command.go`.
* Updated `newCommandRunner` to use these factories, with default
implementations if `nil` is passed.
* Modified callers of `newCommandRunner` (e.g., in `generate.go`,
`release_init.go`, `tag_and_release.go`) to pass `nil, nil` for the new
factory arguments to retain default behavior.
* Added `TestGenerate_DefaultBehavior` to `librarian_test.go` to test
the default `librarian generate` execution. This test utilizes the new
factory arguments in `newGenerateRunner` to inject mock clients.

This was attempted in #1918
but rolled back in #1919 due
to slowness in tests. This is fixed in this commit.

Fixes #1906

Author: jinseopkim0

internal/librarian/command.go

@@ -41,6 +41,12 @@ const (
 	release  = "release"
 )
 
+// GitHubClientFactory type for creating a GitHubClient.
+type GitHubClientFactory func(token string, repo *github.Repository) (GitHubClient, error)
+
+// ContainerClientFactory type for creating a ContainerClient.
+type ContainerClientFactory func(workRoot, image, userUID, userGID string) (ContainerClient, error)
+
 type commitInfo struct {
 	cfg             *config.Config
 	state           *config.LibrarianState
@@ -66,7 +72,20 @@ type commandRunner struct {
 
 const defaultAPISourceBranch = "master"
 
-func newCommandRunner(cfg *config.Config) (*commandRunner, error) {
+func newCommandRunner(cfg *config.Config, ghClientFactory GitHubClientFactory, containerClientFactory ContainerClientFactory) (*commandRunner, error) {
+	// If no GitHub client factory is provided, use the default one.
+	if ghClientFactory == nil {
+		ghClientFactory = func(token string, repo *github.Repository) (GitHubClient, error) {
+			return github.NewClient(token, repo)
+		}
+	}
+	// If no container client factory is provided, use the default one.
+	if containerClientFactory == nil {
+		containerClientFactory = func(workRoot, image, userUID, userGID string) (ContainerClient, error) {
+			return docker.New(workRoot, image, userUID, userGID)
+		}
+	}
+
 	if cfg.APISource == "" {
 		cfg.APISource = "https://github.com/googleapis/googleapis"
 	}
@@ -109,12 +128,12 @@ func newCommandRunner(cfg *config.Config) (*commandRunner, error) {
 			return nil, fmt.Errorf("failed to get GitHub repo from remote: %w", err)
 		}
 	}
-	ghClient, err := github.NewClient(cfg.GitHubToken, gitRepo)
+	ghClient, err := ghClientFactory(cfg.GitHubToken, gitRepo)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create GitHub client: %w", err)
 	}
 
-	container, err := docker.New(cfg.WorkRoot, image, cfg.UserUID, cfg.UserGID)
+	container, err := containerClientFactory(cfg.WorkRoot, image, cfg.UserUID, cfg.UserGID)
 	if err != nil {
 		return nil, err
 	}

internal/librarian/generate.go

@@ -62,7 +62,7 @@ After generation, if the "-push" flag is provided, the changes are committed to
 a pull request is created. Otherwise, the changes are left in the local working tree for
 inspection.`,
 	Run: func(ctx context.Context, cfg *config.Config) error {
-		runner, err := newGenerateRunner(cfg)
+		runner, err := newGenerateRunner(cfg, nil, nil)
 		if err != nil {
 			return err
 		}
@@ -98,8 +98,8 @@ type generateRunner struct {
 	image           string
 }
 
-func newGenerateRunner(cfg *config.Config) (*generateRunner, error) {
-	runner, err := newCommandRunner(cfg)
+func newGenerateRunner(cfg *config.Config, ghClientFactory GitHubClientFactory, containerClientFactory ContainerClientFactory) (*generateRunner, error) {
+	runner, err := newCommandRunner(cfg, ghClientFactory, containerClientFactory)
 	if err != nil {
 		return nil, err
 	}

internal/librarian/generate_test.go

@@ -504,7 +504,7 @@ func TestNewGenerateRunner(t *testing.T) {
 				}
 			}
 
-			r, err := newGenerateRunner(test.cfg)
+			r, err := newGenerateRunner(test.cfg, nil, nil)
 			if (err != nil) != test.wantErr {
 				t.Errorf("newGenerateRunner() error = %v, wantErr %v", err, test.wantErr)
 			}

internal/librarian/librarian_test.go

@@ -30,6 +30,7 @@ import (
 
 	"github.com/googleapis/librarian/internal/cli"
 	"github.com/googleapis/librarian/internal/config"
+	"github.com/googleapis/librarian/internal/github"
 	"github.com/googleapis/librarian/internal/gitrepo"
 	"gopkg.in/yaml.v3"
 
@@ -82,6 +83,52 @@ func TestParentCommands(t *testing.T) {
 	}
 }
 
+func TestGenerate_DefaultBehavior(t *testing.T) {
+	ctx := context.Background()
+
+	// 1. Setup a mock repository with a state file
+	repo := newTestGitRepoWithState(t, true)
+	repoDir := repo.GetDir()
+
+	// Setup a dummy API Source repo to prevent cloning googleapis/googleapis
+	apiSourceDir := t.TempDir()
+	runGit(t, apiSourceDir, "init")
+	runGit(t, apiSourceDir, "config", "user.email", "[email protected]")
+	runGit(t, apiSourceDir, "config", "user.name", "Test User")
+	runGit(t, apiSourceDir, "commit", "--allow-empty", "-m", "initial commit")
+
+	t.Chdir(repoDir)
+
+	// 2. Override dependency creation to use mocks
+	mockContainer := &mockContainerClient{
+		wantLibraryGen: true,
+	}
+	mockGH := &mockGitHubClient{}
+
+	// 3. Call librarian.Run
+	cfg := config.New("generate")
+	cfg.WorkRoot = repoDir
+	cfg.Repo = repoDir
+	cfg.APISource = apiSourceDir
+	runner, err := newGenerateRunner(cfg, func(token string, repo *github.Repository) (GitHubClient, error) {
+		return mockGH, nil
+	}, func(workRoot, image, userUID, userGID string) (ContainerClient, error) {
+		return mockContainer, nil
+	})
+	if err != nil {
+		t.Fatalf("newGenerateRunner() failed: %v", err)
+	}
+	if err := runner.run(ctx); err != nil {
+		t.Fatalf("runner.run() failed: %v", err)
+	}
+
+	// 4. Assertions
+	expectedGenerateCalls := 1
+	if mockContainer.generateCalls != expectedGenerateCalls {
+		t.Errorf("Run(ctx, \"generate\"): got %d generate calls, want %d", mockContainer.generateCalls, expectedGenerateCalls)
+	}
+}
+
 func TestIsURL(t *testing.T) {
 	for _, test := range []struct {
 		name  string

internal/librarian/release_init.go

@@ -78,7 +78,7 @@ type initRunner struct {
 }
 
 func newInitRunner(cfg *config.Config) (*initRunner, error) {
-	runner, err := newCommandRunner(cfg)
+	runner, err := newCommandRunner(cfg, nil, nil)
 	if err != nil {
 		return nil, fmt.Errorf("failed to create init runner: %w", err)
 	}

internal/librarian/tag_and_release.go

@@ -74,7 +74,7 @@ type tagAndReleaseRunner struct {
 }
 
 func newTagAndReleaseRunner(cfg *config.Config) (*tagAndReleaseRunner, error) {
-	runner, err := newCommandRunner(cfg)
+	runner, err := newCommandRunner(cfg, nil, nil)
 	if err != nil {
 		return nil, err
 	}