thaloco/banger
1
Fork 0
Code Pull requests Releases Packages Activity Actions

seams: move the last four package globals onto instance fields

Browse source 
Three test seams were still package-level mutable vars, which tests
had to swap before use. That's the classic path to flaky parallel
tests — two goroutines fighting over the same global fake. Push each
down to the struct that owns the behaviour.

internal/daemon/dns_routing.go
  lookupExecutableFunc + vmDNSAddrFunc → fields on *HostNetwork,
  defaulted at newHostNetwork time. dns_routing_test builds
  HostNetwork{..., lookupExecutable: stub, vmDNSAddr: stub} inline,
  no more t.Cleanup dance around package-level vars.

internal/daemon/preflight.go + doctor.go
  vsockHostDevicePath (mutable string) → vsockHostDevice field on
  *VMService, defaulted via defaultVsockHostDevice constant in
  newVMService. Preflight reads s.vsockHostDevice; doctor reads
  d.vm.vsockHostDevice. Logger test sets d.vm.vsockHostDevice = tmp
  after wireServices.

internal/daemon/workspace/workspace.go
  HostCommandOutputFunc → *Inspector struct with a Runner field.
  Every git-using helper (GitOutput, GitTrimmedOutput,
  GitResolvedConfigValue, RunHostCommand, ListSubmodules,
  ListOverlayPaths, CountUntrackedPaths, InspectRepo,
  ImportRepoToGuest, PrepareRepoCopy) is now a method on *Inspector.
  NewInspector() wraps the real host runner for production;
  WorkspaceService holds one via repoInspector, CLI deps holds one
  too. cli_test.go's submodule-rejection test builds its own
  Inspector with a scripted Runner instead of patching a global.
  Pure helpers (FinalizeScript, ResolveSourcePath, ParsePrepareMode,
  ShellQuote, FormatStepError, GitFileURL, ParseNullSeparatedOutput)
  stay free functions since they don't touch the host.

Sentinel: grep for HostCommandOutputFunc, lookupExecutableFunc,
vmDNSAddrFunc, vsockHostDevicePath is now empty across internal/.
make lint test green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Thales Maciel 2026-04-22 12:07:14 -03:00
parent 2685bc73f8
commit ecb18ce6ca
No known key found for this signature in database
GPG key ID: 33112E6833C34679
17 changed files with 201 additions and 137 deletions
Download patch file Download diff file Expand all files Collapse all files

43
internal/cli/cli_test.go
View file

@ -1116,27 +1116,28 @@ func TestVMRunPreflightRejectsSubmodules(t *testing.T) {
d := defaultDeps()
repoRoot := t.TempDir()
origHostCommandOutput := workspace.HostCommandOutputFunc
t.Cleanup(func() {
workspace.HostCommandOutputFunc = origHostCommandOutput
})
workspace.HostCommandOutputFunc = func(ctx context.Context, name string, args ...string) ([]byte, error) {
t.Helper()
if name != "git" {
t.Fatalf("command = %q, want git", name)
}
switch {
case reflect.DeepEqual(args, []string{"-C", repoRoot, "rev-parse", "--show-toplevel"}):
return []byte(repoRoot + "\n"), nil
case reflect.DeepEqual(args, []string{"-C", repoRoot, "rev-parse", "--is-bare-repository"}):
return []byte("false\n"), nil
case reflect.DeepEqual(args, []string{"-C", repoRoot, "ls-files", "--stage", "-z"}):
return []byte("160000 deadbeef 0\tvendor/submodule\x00"), nil
default:
t.Fatalf("unexpected git args: %v", args)
return nil, nil
}
// Stub the CLI's repo-inspector with a scripted runner. Per-deps
// injection means this test no longer mutates any package global,
// so t.Parallel() is safe to add here in the future without
// worrying about racing another test's fake runner.
d.repoInspector = &workspace.Inspector{
Runner: func(ctx context.Context, name string, args ...string) ([]byte, error) {
t.Helper()
if name != "git" {
t.Fatalf("command = %q, want git", name)
}
switch {
case reflect.DeepEqual(args, []string{"-C", repoRoot, "rev-parse", "--show-toplevel"}):
return []byte(repoRoot + "\n"), nil
case reflect.DeepEqual(args, []string{"-C", repoRoot, "rev-parse", "--is-bare-repository"}):
return []byte("false\n"), nil
case reflect.DeepEqual(args, []string{"-C", repoRoot, "ls-files", "--stage", "-z"}):
return []byte("160000 deadbeef 0\tvendor/submodule\x00"), nil
default:
t.Fatalf("unexpected git args: %v", args)
return nil, nil
}
},
}
_, err := d.vmRunPreflightRepo(context.Background(), repoRoot)

6
internal/cli/commands_vm.go
View file

@ -119,7 +119,7 @@ Three modes:
if strings.TrimSpace(repoPtr.branchName) != "" {
dryFromRef = repoPtr.fromRef
}
return runWorkspaceDryRun(cmd.Context(), cmd.OutOrStdout(), repoPtr.sourcePath, repoPtr.branchName, dryFromRef, repoPtr.includeUntracked)
return d.runWorkspaceDryRun(cmd.Context(), cmd.OutOrStdout(), repoPtr.sourcePath, repoPtr.branchName, dryFromRef, repoPtr.includeUntracked)
}
layout, err := paths.Resolve()
@ -618,14 +618,14 @@ func (d *deps) newVMWorkspacePrepareCommand() *cobra.Command {
prepareFrom = fromRef
}
if dryRun {
return runWorkspaceDryRun(cmd.Context(), cmd.OutOrStdout(), resolvedPath, branchName, prepareFrom, includeUntracked)
return d.runWorkspaceDryRun(cmd.Context(), cmd.OutOrStdout(), resolvedPath, branchName, prepareFrom, includeUntracked)
}
layout, _, err := d.ensureDaemon(cmd.Context())
if err != nil {
return err
}
if !includeUntracked {
if err := noteUntrackedSkipped(cmd.Context(), cmd.ErrOrStderr(), resolvedPath); err != nil {
if err := d.noteUntrackedSkipped(cmd.Context(), cmd.ErrOrStderr(), resolvedPath); err != nil {
return err
}
}

8
internal/cli/deps.go
View file

@ -12,6 +12,7 @@ import (
"banger/internal/api"
"banger/internal/daemon"
"banger/internal/daemon/workspace"
"banger/internal/guest"
"banger/internal/paths"
"banger/internal/rpc"
@ -52,6 +53,12 @@ type deps struct {
buildVMRunToolingPlan func(ctx context.Context, repoRoot string) toolingplan.Plan
cwd func() (string, error)
completionLister func(ctx context.Context, socketPath, method string) ([]string, error)
// repoInspector is the CLI's single workspace-package Inspector.
// Every code path that needs to shell out to git on the host
// (preflight, dry-run, untracked-count note) goes through it, so
// tests inject a stub Runner via this field instead of mutating a
// package global.
repoInspector *workspace.Inspector
}
func defaultDeps() *deps {
@ -127,5 +134,6 @@ func defaultDeps() *deps {
buildVMRunToolingPlan: toolingplan.Build,
cwd: os.Getwd,
completionLister: defaultCompletionLister,
repoInspector: workspace.NewInspector(),
}
}

8
internal/cli/vm_run.go
View file

@ -93,18 +93,18 @@ func (d *deps) vmRunPreflightRepo(ctx context.Context, rawPath string) (string,
if err != nil {
return "", err
}
repoRoot, err := workspace.GitTrimmedOutput(ctx, sourcePath, "rev-parse", "--show-toplevel")
repoRoot, err := d.repoInspector.GitTrimmedOutput(ctx, sourcePath, "rev-parse", "--show-toplevel")
if err != nil {
return "", fmt.Errorf("%s is not inside a git repository", sourcePath)
}
isBare, err := workspace.GitTrimmedOutput(ctx, repoRoot, "rev-parse", "--is-bare-repository")
isBare, err := d.repoInspector.GitTrimmedOutput(ctx, repoRoot, "rev-parse", "--is-bare-repository")
if err != nil {
return "", fmt.Errorf("inspect git repository %s: %w", repoRoot, err)
}
if isBare == "true" {
return "", fmt.Errorf("vm run requires a non-bare git repository: %s", repoRoot)
}
submodules, err := workspace.ListSubmodules(ctx, repoRoot)
submodules, err := d.repoInspector.ListSubmodules(ctx, repoRoot)
if err != nil {
return "", err
}
@ -195,7 +195,7 @@ func (d *deps) runVMRun(ctx context.Context, socketPath string, cfg model.Daemon
fromRef = repo.fromRef
}
if !repo.includeUntracked {
if err := noteUntrackedSkipped(ctx, stderr, repo.sourcePath); err != nil {
if err := d.noteUntrackedSkipped(ctx, stderr, repo.sourcePath); err != nil {
printVMRunWarning(stderr, fmt.Sprintf("count untracked files failed: %v", err))
}
}

15
internal/cli/workspace_preview.go
View file

@ -4,17 +4,16 @@ import (
"context"
"fmt"
"io"
"banger/internal/daemon/workspace"
)
// runWorkspaceDryRun inspects the local repo at resolvedPath and
// prints the file list that `vm run` / `workspace prepare` would ship
// into the guest. Runs on the CLI side (no daemon RPC needed) since
// the daemon is always local and the workspace inspection is a pure
// git read.
func runWorkspaceDryRun(ctx context.Context, out io.Writer, resolvedPath, branchName, fromRef string, includeUntracked bool) error {
spec, err := workspace.InspectRepo(ctx, resolvedPath, branchName, fromRef, includeUntracked)
// git read. Git calls go through d.repoInspector so tests inject a
// stub Runner via the deps struct instead of touching package globals.
func (d *deps) runWorkspaceDryRun(ctx context.Context, out io.Writer, resolvedPath, branchName, fromRef string, includeUntracked bool) error {
spec, err := d.repoInspector.InspectRepo(ctx, resolvedPath, branchName, fromRef, includeUntracked)
if err != nil {
return err
}
@ -30,7 +29,7 @@ func runWorkspaceDryRun(ctx context.Context, out io.Writer, resolvedPath, branch
fmt.Fprintln(out, path)
}
if !includeUntracked {
if err := noteUntrackedSkipped(ctx, out, spec.RepoRoot); err != nil {
if err := d.noteUntrackedSkipped(ctx, out, spec.RepoRoot); err != nil {
return err
}
}
@ -41,8 +40,8 @@ func runWorkspaceDryRun(ctx context.Context, out io.Writer, resolvedPath, branch
// the repo has untracked non-ignored files that will NOT be copied
// because --include-untracked was not passed. Silent when there are
// no such files, or when the count can't be determined.
func noteUntrackedSkipped(ctx context.Context, out io.Writer, repoRoot string) error {
count, err := workspace.CountUntrackedPaths(ctx, repoRoot)
func (d *deps) noteUntrackedSkipped(ctx context.Context, out io.Writer, repoRoot string) error {
count, err := d.repoInspector.CountUntrackedPaths(ctx, repoRoot)
if err != nil {
return err
}