gitutil: refactor the way to mock (#6032)

* Refactor the mock module store

* Only test on 1.14.x

.github/workflows/go.yml

@@ -19,7 +19,7 @@ jobs:
     name: Test
     strategy:
       matrix:
-        go-version: [1.13.x, 1.14.x]
+        go-version: [1.14.x]
         platform: [ubuntu-latest, macos-latest, windows-latest]
     runs-on: ${{ matrix.platform }}
     steps:
@@ -43,4 +43,4 @@ jobs:
           key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
           restore-keys: |
             ${{ runner.os }}-go-
-      
+

internal/gitutil/mock.go

@@ -8,16 +8,49 @@ import (
 	"github.com/gogs/git-module"
 )
 
+var _ ModuleStore = (*MockModuleStore)(nil)
+
+// MockModuleStore is a mock implementation of ModuleStore interface.
 type MockModuleStore struct {
-	RepoAddRemote    func(repoPath, name, url string, opts ...git.AddRemoteOptions) error
+	repoAddRemote    func(repoPath, name, url string, opts ...git.AddRemoteOptions) error
-	RepoDiffNameOnly func(repoPath, base, head string, opts ...git.DiffNameOnlyOptions) ([]string, error)
+	repoDiffNameOnly func(repoPath, base, head string, opts ...git.DiffNameOnlyOptions) ([]string, error)
-	RepoLog          func(repoPath, rev string, opts ...git.LogOptions) ([]*git.Commit, error)
+	repoLog          func(repoPath, rev string, opts ...git.LogOptions) ([]*git.Commit, error)
-	RepoMergeBase    func(repoPath, base, head string, opts ...git.MergeBaseOptions) (string, error)
+	repoMergeBase    func(repoPath, base, head string, opts ...git.MergeBaseOptions) (string, error)
-	RepoRemoveRemote func(repoPath, name string, opts ...git.RemoveRemoteOptions) error
+	repoRemoveRemote func(repoPath, name string, opts ...git.RemoveRemoteOptions) error
-	RepoTags         func(repoPath string, opts ...git.TagsOptions) ([]string, error)
+	repoTags         func(repoPath string, opts ...git.TagsOptions) ([]string, error)
+
+	pullRequestMeta func(headPath, basePath, headBranch, baseBranch string) (*PullRequestMeta, error)
+	listTagsAfter   func(repoPath, after string, limit int) (*TagsPage, error)
 }
 
-// MockModule holds mock implementation of each method for Modulers interface.
+func (m *MockModuleStore) RepoAddRemote(repoPath, name, url string, opts ...git.AddRemoteOptions) error {
-// When the field is non-nil, it is considered mocked. Otherwise, the real
+	return m.repoAddRemote(repoPath, name, url, opts...)
-// implementation will be executed.
+}
-var MockModule MockModuleStore
+
+func (m *MockModuleStore) RepoDiffNameOnly(repoPath, base, head string, opts ...git.DiffNameOnlyOptions) ([]string, error) {
+	return m.repoDiffNameOnly(repoPath, base, head, opts...)
+}
+
+func (m *MockModuleStore) RepoLog(repoPath, rev string, opts ...git.LogOptions) ([]*git.Commit, error) {
+	return m.repoLog(repoPath, rev, opts...)
+}
+
+func (m *MockModuleStore) RepoMergeBase(repoPath, base, head string, opts ...git.MergeBaseOptions) (string, error) {
+	return m.repoMergeBase(repoPath, base, head, opts...)
+}
+
+func (m *MockModuleStore) RepoRemoveRemote(repoPath, name string, opts ...git.RemoveRemoteOptions) error {
+	return m.repoRemoveRemote(repoPath, name, opts...)
+}
+
+func (m *MockModuleStore) RepoTags(repoPath string, opts ...git.TagsOptions) ([]string, error) {
+	return m.repoTags(repoPath, opts...)
+}
+
+func (m *MockModuleStore) PullRequestMeta(headPath, basePath, headBranch, baseBranch string) (*PullRequestMeta, error) {
+	return m.pullRequestMeta(headPath, basePath, headBranch, baseBranch)
+}
+
+func (m *MockModuleStore) ListTagsAfter(repoPath, after string, limit int) (*TagsPage, error) {
+	return m.listTagsAfter(repoPath, after, limit)
+}

internal/gitutil/module.go

@@ -8,10 +8,10 @@ import (
 	"github.com/gogs/git-module"
 )
 
-// Moduler is the interface for Git operations.
+// ModuleStore is the interface for Git operations.
 //
 // NOTE: All methods are sorted in alphabetically.
-type Moduler interface {
+type ModuleStore interface {
 	// AddRemote adds a new remote to the repository in given path.
 	RepoAddRemote(repoPath, name, url string, opts ...git.AddRemoteOptions) error
 	// RepoDiffNameOnly returns a list of changed files between base and head revisions
@@ -28,63 +28,38 @@ type Moduler interface {
 	// RepoTags returns a list of tags of the repository in given path.
 	RepoTags(repoPath string, opts ...git.TagsOptions) ([]string, error)
 
-	Utiler
-}
-
-// Utiler is the interface for utility helpers implemented in this package.
-//
-// NOTE: All methods are sorted in alphabetically.
-type Utiler interface {
 	// GetPullRequestMeta gathers pull request metadata based on given head and base information.
 	PullRequestMeta(headPath, basePath, headBranch, baseBranch string) (*PullRequestMeta, error)
-	// ListTagsAfter returns a list of tags "after" (exlusive) given tag.
+	// ListTagsAfter returns a list of tags "after" (exclusive) given tag.
 	ListTagsAfter(repoPath, after string, limit int) (*TagsPage, error)
 }
 
-// moduler is holds real implementation.
+// module holds the real implementation.
-type moduler struct{}
+type module struct{}
 
-func (moduler) RepoAddRemote(repoPath, name, url string, opts ...git.AddRemoteOptions) error {
+func (module) RepoAddRemote(repoPath, name, url string, opts ...git.AddRemoteOptions) error {
-	if MockModule.RepoAddRemote != nil {
-		return MockModule.RepoAddRemote(repoPath, name, url, opts...)
-	}
 	return git.RepoAddRemote(repoPath, name, url, opts...)
 }
 
-func (moduler) RepoDiffNameOnly(repoPath, base, head string, opts ...git.DiffNameOnlyOptions) ([]string, error) {
+func (module) RepoDiffNameOnly(repoPath, base, head string, opts ...git.DiffNameOnlyOptions) ([]string, error) {
-	if MockModule.RepoDiffNameOnly != nil {
-		return MockModule.RepoDiffNameOnly(repoPath, base, head, opts...)
-	}
 	return git.RepoDiffNameOnly(repoPath, base, head, opts...)
 }
 
-func (moduler) RepoLog(repoPath, rev string, opts ...git.LogOptions) ([]*git.Commit, error) {
+func (module) RepoLog(repoPath, rev string, opts ...git.LogOptions) ([]*git.Commit, error) {
-	if MockModule.RepoLog != nil {
-		return MockModule.RepoLog(repoPath, rev, opts...)
-	}
 	return git.RepoLog(repoPath, rev, opts...)
 }
 
-func (moduler) RepoMergeBase(repoPath, base, head string, opts ...git.MergeBaseOptions) (string, error) {
+func (module) RepoMergeBase(repoPath, base, head string, opts ...git.MergeBaseOptions) (string, error) {
-	if MockModule.RepoMergeBase != nil {
-		return MockModule.RepoMergeBase(repoPath, base, head, opts...)
-	}
 	return git.RepoMergeBase(repoPath, base, head, opts...)
 }
 
-func (moduler) RepoRemoveRemote(repoPath, name string, opts ...git.RemoveRemoteOptions) error {
+func (module) RepoRemoveRemote(repoPath, name string, opts ...git.RemoveRemoteOptions) error {
-	if MockModule.RepoRemoveRemote != nil {
-		return MockModule.RepoRemoveRemote(repoPath, name, opts...)
-	}
 	return git.RepoRemoveRemote(repoPath, name, opts...)
 }
 
-func (moduler) RepoTags(repoPath string, opts ...git.TagsOptions) ([]string, error) {
+func (module) RepoTags(repoPath string, opts ...git.TagsOptions) ([]string, error) {
-	if MockModule.RepoTags != nil {
-		return MockModule.RepoTags(repoPath, opts...)
-	}
 	return git.RepoTags(repoPath, opts...)
 }
 
 // Module is a mockable interface for Git operations.
-var Module Moduler = moduler{}
+var Module ModuleStore = module{}

internal/gitutil/pull_request.go

@@ -24,7 +24,7 @@ type PullRequestMeta struct {
 	NumFiles int
 }
 
-func (moduler) PullRequestMeta(headPath, basePath, headBranch, baseBranch string) (*PullRequestMeta, error) {
+func (module) PullRequestMeta(headPath, basePath, headBranch, baseBranch string) (*PullRequestMeta, error) {
 	tmpRemoteBranch := baseBranch
 
 	// We need to create a temporary remote when the pull request is sent from a forked repository.

internal/gitutil/pull_request_test.go

@@ -24,75 +24,81 @@ func TestModuler_PullRequestMeta(t *testing.T) {
 		{ID: git.MustIDFromString("adfd6da3c0a3fb038393144becbf37f14f780087")},
 	}
 
-	MockModule.RepoAddRemote = func(repoPath, name, url string, opts ...git.AddRemoteOptions) error {
+	mockModule := &MockModuleStore{
-		if repoPath != headPath {
+		repoAddRemote: func(repoPath, name, url string, opts ...git.AddRemoteOptions) error {
-			return fmt.Errorf("repoPath: want %q but got %q", headPath, repoPath)
+			if repoPath != headPath {
-		} else if name == "" {
+				return fmt.Errorf("repoPath: want %q but got %q", headPath, repoPath)
-			return errors.New("empty name")
+			} else if name == "" {
-		} else if url != basePath {
+				return errors.New("empty name")
-			return fmt.Errorf("url: want %q but got %q", basePath, url)
+			} else if url != basePath {
-		}
+				return fmt.Errorf("url: want %q but got %q", basePath, url)
+			}
 
-		if len(opts) == 0 {
+			if len(opts) == 0 {
-			return errors.New("no options")
+				return errors.New("no options")
-		} else if !opts[0].Fetch {
+			} else if !opts[0].Fetch {
-			return fmt.Errorf("opts.Fetch: want %v but got %v", true, opts[0].Fetch)
+				return fmt.Errorf("opts.Fetch: want %v but got %v", true, opts[0].Fetch)
-		}
+			}
 
-		return nil
+			return nil
+		},
+		repoMergeBase: func(repoPath, base, head string, opts ...git.MergeBaseOptions) (string, error) {
+			if repoPath != headPath {
+				return "", fmt.Errorf("repoPath: want %q but got %q", headPath, repoPath)
+			} else if base == "" {
+				return "", errors.New("empty base")
+			} else if head != headBranch {
+				return "", fmt.Errorf("head: want %q but got %q", headBranch, head)
+			}
+
+			return mergeBase, nil
+		},
+		repoLog: func(repoPath, rev string, opts ...git.LogOptions) ([]*git.Commit, error) {
+			if repoPath != headPath {
+				return nil, fmt.Errorf("repoPath: want %q but got %q", headPath, repoPath)
+			}
+
+			expRev := mergeBase + "..." + headBranch
+			if rev != expRev {
+				return nil, fmt.Errorf("rev: want %q but got %q", expRev, rev)
+			}
+
+			return commits, nil
+		},
+		repoDiffNameOnly: func(repoPath, base, head string, opts ...git.DiffNameOnlyOptions) ([]string, error) {
+			if repoPath != headPath {
+				return nil, fmt.Errorf("repoPath: want %q but got %q", headPath, repoPath)
+			} else if base == "" {
+				return nil, errors.New("empty base")
+			} else if head != headBranch {
+				return nil, fmt.Errorf("head: want %q but got %q", headBranch, head)
+			}
+
+			if len(opts) == 0 {
+				return nil, errors.New("no options")
+			} else if !opts[0].NeedsMergeBase {
+				return nil, fmt.Errorf("opts.NeedsMergeBase: want %v but got %v", true, opts[0].NeedsMergeBase)
+			}
+
+			return changedFiles, nil
+		},
+		repoRemoveRemote: func(repoPath, name string, opts ...git.RemoveRemoteOptions) error {
+			if repoPath != headPath {
+				return fmt.Errorf("repoPath: want %q but got %q", headPath, repoPath)
+			} else if name == "" {
+				return errors.New("empty name")
+			}
+
+			return nil
+		},
+
+		pullRequestMeta: Module.PullRequestMeta,
 	}
-	MockModule.RepoMergeBase = func(repoPath, base, head string, opts ...git.MergeBaseOptions) (string, error) {
+	beforeModule := Module
-		if repoPath != headPath {
+	Module = mockModule
-			return "", fmt.Errorf("repoPath: want %q but got %q", headPath, repoPath)
+	t.Cleanup(func() {
-		} else if base == "" {
+		Module = beforeModule
-			return "", errors.New("empty base")
+	})
-		} else if head != headBranch {
-			return "", fmt.Errorf("head: want %q but got %q", headBranch, head)
-		}
-
-		return mergeBase, nil
-	}
-	MockModule.RepoLog = func(repoPath, rev string, opts ...git.LogOptions) ([]*git.Commit, error) {
-		if repoPath != headPath {
-			return nil, fmt.Errorf("repoPath: want %q but got %q", headPath, repoPath)
-		}
-
-		expRev := mergeBase + "..." + headBranch
-		if rev != expRev {
-			return nil, fmt.Errorf("rev: want %q but got %q", expRev, rev)
-		}
-
-		return commits, nil
-	}
-	MockModule.RepoDiffNameOnly = func(repoPath, base, head string, opts ...git.DiffNameOnlyOptions) ([]string, error) {
-		if repoPath != headPath {
-			return nil, fmt.Errorf("repoPath: want %q but got %q", headPath, repoPath)
-		} else if base == "" {
-			return nil, errors.New("empty base")
-		} else if head != headBranch {
-			return nil, fmt.Errorf("head: want %q but got %q", headBranch, head)
-		}
-
-		if len(opts) == 0 {
-			return nil, errors.New("no options")
-		} else if !opts[0].NeedsMergeBase {
-			return nil, fmt.Errorf("opts.NeedsMergeBase: want %v but got %v", true, opts[0].NeedsMergeBase)
-		}
-
-		return changedFiles, nil
-	}
-	MockModule.RepoRemoveRemote = func(repoPath, name string, opts ...git.RemoveRemoteOptions) error {
-		if repoPath != headPath {
-			return fmt.Errorf("repoPath: want %q but got %q", headPath, repoPath)
-		} else if name == "" {
-			return errors.New("empty name")
-		}
-
-		return nil
-	}
-	defer func() {
-		MockModule = MockModuleStore{}
-	}()
 
 	meta, err := Module.PullRequestMeta(headPath, basePath, headBranch, baseBranch)
 	if err != nil {

internal/gitutil/tag.go

@@ -20,7 +20,7 @@ type TagsPage struct {
 	HasNext bool
 }
 
-func (moduler) ListTagsAfter(repoPath, after string, limit int) (*TagsPage, error) {
+func (module) ListTagsAfter(repoPath, after string, limit int) (*TagsPage, error) {
 	all, err := Module.RepoTags(repoPath)
 	if err != nil {
 		return nil, errors.Wrap(err, "get tags")

internal/gitutil/tag_test.go

@@ -12,16 +12,22 @@ import (
 )
 
 func TestModuler_ListTagsAfter(t *testing.T) {
-	MockModule.RepoTags = func(string, ...git.TagsOptions) ([]string, error) {
+	mockModule := &MockModuleStore{
-		return []string{
+		repoTags: func(string, ...git.TagsOptions) ([]string, error) {
-			"v2.3.0", "v2.2.1", "v2.1.0",
+			return []string{
-			"v1.3.0", "v1.2.0", "v1.1.0",
+				"v2.3.0", "v2.2.1", "v2.1.0",
-			"v0.8.0", "v0.5.0", "v0.1.0",
+				"v1.3.0", "v1.2.0", "v1.1.0",
-		}, nil
+				"v0.8.0", "v0.5.0", "v0.1.0",
+			}, nil
+		},
+
+		listTagsAfter: Module.ListTagsAfter,
 	}
-	defer func() {
+	beforeModule := Module
-		MockModule = MockModuleStore{}
+	Module = mockModule
-	}()
+	t.Cleanup(func() {
+		Module = beforeModule
+	})
 
 	tests := []struct {
 		name        string