cli-stubbing.md

I believe that one avenue of helping improve our test suite, specifically making old tests easier to understand/edit and making new tests easier to write, is to focus on isolating parts of functionality that should not be exercised together in the same test. I suggest that one viable way of doing so is dependency injection by accepting Go interfaces.

Consider this stripped down implementation of the pr create command, which roughly reflects our current code practices:

func prCreate(opts *PRCreateOptions) error {
	httpClient, err := opts.HttpClient()
	// ...
	baseRepo, err := opts.BaseRepo()
	// ...
	currentBranch, err := git.CurrentBranch()
	// ...
	apiClient := api.NewClientFromHTTP(httpClient)
	// ...
	existingPR, err := api.PullRequestForBranch(apiClient, baseRepo, "main", currentBranch, []string{"OPEN"})
	// ...
	templateFiles, legacyTemplate := shared.FindTemplates(opts.RootDirOverride, "PULL_REQUEST_TEMPLATE")
	// ...
	templateContent, err = shared.TemplateSurvey(templateFiles, legacyTemplate, &titleBodyState)
	// ...
	git.Push("fork", currentBranch, opts.IO.ErrOut)
	// ...
	if opts.WebMode {
		return browser.OpenURL(compareURL)
	}
	api.CreatePullRequest(apiClient, baseRepo, params)
}

There is a lot happening in a single function. To name a few:

• it interfaces with Git by shelling out to git commands;
• it fetches information from the GitHub API in several places and at different levels of abstraction;
• it looks up PR templates on disk;
• it presents an interactive Survey interface to the user;
• it potentially opens a web browser;
• finally, it creates the PR via making another API call.

So to exercise prCreate() in tests, we need to stub out around a dozen API endpoints and their responses, several git command invocations and their responses, verify that a git push was invoked with the right arguments, maintain fake PR template files in a testing location on disk, intercept the open <URL> browser invocation. This is a lot of responsibility for a single test and it reflects as a giant test setup.

We already inject some other pieces of context, though: for example, BaseRepo is easy to inject in tests and offers a conventient way to avoid having to stub parsing out git remote -v every time a base repository is determined. It does not matter what kind of concrete type BaseRepo is; only that it implements RepoOwner() and RepoName() methods.

A different approach

I suggest to take the approach of accepting tiny interfaces further. In the following sketch, names are intentionally exaggerated for illustration purposes:

type gitContext interface {
    CurrentBranch() (string, error)
    Push(string, string) error
}

type templateLookerUpper interface {
    Lookup() ([]Template, error)
}

type surveyAsker interface {
    SurveyAsk(string, []string) (string, error)
}

type prValidator interface {
    Validate(params) error
}

type prCreator interface {
    Create(params) (PullRequest, error)
}

type browser interface {
    OpenURL(string) error
}

type PRCreateOptions struct {
    localRepo gitContext
    templates templateLookerUpper
    survey    surveyAsker
    validator prValidator
    creator   prCreator
    browser   browser
}

func prCreate(opts *PRCreateOptions) error {
    currentBranch, err := opts.localRepo.CurrentBranch()
    // ...
    err = opts.validator.Validate(params)
    // ...
    templates, err := opts.templates.Lookup()
    opts.survey.SurveyAsk("Pick a template:", templates)
    // ...
    opts.localRepo.Push()
    // ...
    if opts.WebMode {
        return browser.OpenURL(compareURL)
    }
    pr, err := opts.creator.Create(params)
    return err
}

The immediate benefits are:

• No API stubs whatsover, since prCreate() does not make API requests directly.
• No stubbing of specific git or open commands that we otherwise shell out to at runtime.
• No filesystem stubbing.

Note that the very package that defines prCreate() also defines the interfaces of the kinds of objects that it expects, and resists working with concrete types. This gives a lot of flexibility to the caller, especially when the caller is a test that wants to exercise that e.g. Survey allows the user to pick a template, but the same test doesn't want to be concerned with stubbing API responses, setting up PR template files on disk, etc. Tests will generally inject objects that implement these methods in the simplest way possible to satisfy the caller and the needs of the test.

As a more concrete example, observe this changeset that makes the interactive portions of MetadataSurvey() easier to test by decoupling it from api.RepoMetadata() directly, and therefore frees of from having to maintain the kind of API stubs that don't belong to MetadataSurvey(), but really only belong in tests that are more focused on api.RepoMetadata() alone.

Notes on adoption

The approach of tiny interfaces comes with some drawback: namely, the general proliferation of extra types in the codebase, as well as tests usually having to declare concrete types as stubs that satisfy those interfaces. The impacts of these requirements, however, can be mitigated by using tooling that generates method stubs to conform to an interface.

If desireable, this approach would not be implemented as one giant refactor, but rather as a series of incremental improvements whenever we write new commands or revisit old ones.