identity: add more test for serialisation and push/pull/merge + fixes

Michael Muré created

Change summary

bug/bug_actions.go                |   7 -
bug/bug_actions_test.go           |  84 +----------------
bug/operation_test.go             |   3 
graphql/graphql_test.go           |  16 +++
identity/bare_test.go             |  19 ++++
identity/identity.go              |  64 +++++++++++++
identity/identity_actions.go      |  11 -
identity/identity_actions_test.go | 151 +++++++++++++++++++++++++++++++++
identity/identity_stub_test.go    |  23 +++++
identity/version_test.go          |  42 +++++++++
tests/read_bugs_test.go           |  18 +++
util/test/repo.go                 |  43 +++++++-
12 files changed, 381 insertions(+), 100 deletions(-)

Detailed changes

bug/bug_actions.go 🔗

@@ -23,8 +23,7 @@ func Push(repo repository.Repo, remote string) (string, error) {
 }
 
 // Pull will do a Fetch + MergeAll
-// This function won't give details on the underlying process. If you need more,
-// use Fetch and MergeAll separately.
+// This function will return an error if a merge fail
 func Pull(repo repository.ClockedRepo, remote string) error {
 	_, err := Fetch(repo, remote)
 	if err != nil {
@@ -36,9 +35,7 @@ func Pull(repo repository.ClockedRepo, remote string) error {
 			return merge.Err
 		}
 		if merge.Status == MergeStatusInvalid {
-			// Not awesome: simply output the merge failure here as this function
-			// is only used in tests for now.
-			fmt.Println(merge)
+			return errors.Errorf("merge failure: %s", merge.Reason)
 		}
 	}
 

bug/bug_actions_test.go 🔗

@@ -1,81 +1,15 @@
 package bug
 
 import (
-	"github.com/MichaelMure/git-bug/repository"
+	"github.com/MichaelMure/git-bug/util/test"
 	"github.com/stretchr/testify/assert"
 
-	"io/ioutil"
-	"log"
-	"os"
 	"testing"
 )
 
-func createRepo(bare bool) *repository.GitRepo {
-	dir, err := ioutil.TempDir("", "")
-	if err != nil {
-		log.Fatal(err)
-	}
-
-	// fmt.Println("Creating repo:", dir)
-
-	var creator func(string) (*repository.GitRepo, error)
-
-	if bare {
-		creator = repository.InitBareGitRepo
-	} else {
-		creator = repository.InitGitRepo
-	}
-
-	repo, err := creator(dir)
-	if err != nil {
-		log.Fatal(err)
-	}
-
-	if err := repo.StoreConfig("user.name", "testuser"); err != nil {
-		log.Fatal("failed to set user.name for test repository: ", err)
-	}
-	if err := repo.StoreConfig("user.email", "testuser@example.com"); err != nil {
-		log.Fatal("failed to set user.email for test repository: ", err)
-	}
-
-	return repo
-}
-
-func cleanupRepo(repo repository.Repo) error {
-	path := repo.GetPath()
-	// fmt.Println("Cleaning repo:", path)
-	return os.RemoveAll(path)
-}
-
-func setupRepos(t testing.TB) (repoA, repoB, remote *repository.GitRepo) {
-	repoA = createRepo(false)
-	repoB = createRepo(false)
-	remote = createRepo(true)
-
-	remoteAddr := "file://" + remote.GetPath()
-
-	err := repoA.AddRemote("origin", remoteAddr)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	err = repoB.AddRemote("origin", remoteAddr)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	return repoA, repoB, remote
-}
-
-func cleanupRepos(repoA, repoB, remote *repository.GitRepo) {
-	cleanupRepo(repoA)
-	cleanupRepo(repoB)
-	cleanupRepo(remote)
-}
-
 func TestPushPull(t *testing.T) {
-	repoA, repoB, remote := setupRepos(t)
-	defer cleanupRepos(repoA, repoB, remote)
+	repoA, repoB, remote := test.SetupReposAndRemote(t)
+	defer test.CleanupRepos(repoA, repoB, remote)
 
 	err := rene.Commit(repoA)
 	assert.NoError(t, err)
@@ -139,8 +73,8 @@ func BenchmarkRebaseTheirs(b *testing.B) {
 }
 
 func _RebaseTheirs(t testing.TB) {
-	repoA, repoB, remote := setupRepos(t)
-	defer cleanupRepos(repoA, repoB, remote)
+	repoA, repoB, remote := test.SetupReposAndRemote(t)
+	defer test.CleanupRepos(repoA, repoB, remote)
 
 	bug1, _, err := Create(rene, unix, "bug1", "message")
 	assert.NoError(t, err)
@@ -200,8 +134,8 @@ func BenchmarkRebaseOurs(b *testing.B) {
 }
 
 func _RebaseOurs(t testing.TB) {
-	repoA, repoB, remote := setupRepos(t)
-	defer cleanupRepos(repoA, repoB, remote)
+	repoA, repoB, remote := test.SetupReposAndRemote(t)
+	defer test.CleanupRepos(repoA, repoB, remote)
 
 	bug1, _, err := Create(rene, unix, "bug1", "message")
 	assert.NoError(t, err)
@@ -281,8 +215,8 @@ func BenchmarkRebaseConflict(b *testing.B) {
 }
 
 func _RebaseConflict(t testing.TB) {
-	repoA, repoB, remote := setupRepos(t)
-	defer cleanupRepos(repoA, repoB, remote)
+	repoA, repoB, remote := test.SetupReposAndRemote(t)
+	defer test.CleanupRepos(repoA, repoB, remote)
 
 	bug1, _, err := Create(rene, unix, "bug1", "message")
 	assert.NoError(t, err)

bug/operation_test.go 🔗

@@ -6,6 +6,7 @@ import (
 	"github.com/MichaelMure/git-bug/identity"
 	"github.com/MichaelMure/git-bug/repository"
 	"github.com/MichaelMure/git-bug/util/git"
+	"github.com/MichaelMure/git-bug/util/test"
 	"github.com/stretchr/testify/require"
 )
 
@@ -76,7 +77,7 @@ func TestMetadata(t *testing.T) {
 func TestHash(t *testing.T) {
 	repos := []repository.ClockedRepo{
 		repository.NewMockRepoForTest(),
-		createRepo(false),
+		test.CreateRepo(false),
 	}
 
 	for _, repo := range repos {

graphql/graphql_test.go 🔗

@@ -5,12 +5,26 @@ import (
 	"testing"
 
 	"github.com/MichaelMure/git-bug/graphql/models"
+	"github.com/MichaelMure/git-bug/misc/random_bugs"
+	"github.com/MichaelMure/git-bug/repository"
 	"github.com/MichaelMure/git-bug/util/test"
 	"github.com/vektah/gqlgen/client"
 )
 
+func CreateFilledRepo(bugNumber int) repository.ClockedRepo {
+	repo := test.CreateRepo(false)
+
+	var seed int64 = 42
+	options := random_bugs.DefaultOptions()
+
+	options.BugNumber = bugNumber
+
+	random_bugs.CommitRandomBugsWithSeed(repo, options, seed)
+	return repo
+}
+
 func TestQueries(t *testing.T) {
-	repo := test.CreateFilledRepo(10)
+	repo := CreateFilledRepo(10)
 
 	handler, err := NewHandler(repo)
 	if err != nil {

identity/bare_test.go 🔗

@@ -1,6 +1,7 @@
 package identity
 
 import (
+	"encoding/json"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -11,3 +12,21 @@ func TestBare_Id(t *testing.T) {
 	id := i.Id()
 	assert.Equal(t, "7b226e616d65223a226e616d65222c22", id)
 }
+
+func TestBareSerialize(t *testing.T) {
+	before := &Bare{
+		login:     "login",
+		email:     "email",
+		name:      "name",
+		avatarUrl: "avatar",
+	}
+
+	data, err := json.Marshal(before)
+	assert.NoError(t, err)
+
+	var after Bare
+	err = json.Unmarshal(data, &after)
+	assert.NoError(t, err)
+
+	assert.Equal(t, before, &after)
+}

identity/identity.go 🔗

@@ -18,6 +18,8 @@ const identityRemoteRefPattern = "refs/remotes/%s/identities/"
 const versionEntryName = "version"
 const identityConfigKey = "git-bug.identity"
 
+var ErrNonFastForwardMerge = errors.New("non fast-forward identity merge")
+
 var _ Interface = &Identity{}
 
 type Identity struct {
@@ -136,6 +138,50 @@ func read(repo repository.Repo, ref string) (*Identity, error) {
 	return i, nil
 }
 
+type StreamedIdentity struct {
+	Identity *Identity
+	Err      error
+}
+
+// ReadAllLocalIdentities read and parse all local Identity
+func ReadAllLocalIdentities(repo repository.ClockedRepo) <-chan StreamedIdentity {
+	return readAllIdentities(repo, identityRefPattern)
+}
+
+// ReadAllRemoteIdentities read and parse all remote Identity for a given remote
+func ReadAllRemoteIdentities(repo repository.ClockedRepo, remote string) <-chan StreamedIdentity {
+	refPrefix := fmt.Sprintf(identityRemoteRefPattern, remote)
+	return readAllIdentities(repo, refPrefix)
+}
+
+// Read and parse all available bug with a given ref prefix
+func readAllIdentities(repo repository.ClockedRepo, refPrefix string) <-chan StreamedIdentity {
+	out := make(chan StreamedIdentity)
+
+	go func() {
+		defer close(out)
+
+		refs, err := repo.ListRefs(refPrefix)
+		if err != nil {
+			out <- StreamedIdentity{Err: err}
+			return
+		}
+
+		for _, ref := range refs {
+			b, err := read(repo, ref)
+
+			if err != nil {
+				out <- StreamedIdentity{Err: err}
+				return
+			}
+
+			out <- StreamedIdentity{Identity: b}
+		}
+	}()
+
+	return out
+}
+
 // NewFromGitUser will query the repository for user detail and
 // build the corresponding Identity
 func NewFromGitUser(repo repository.Repo) (*Identity, error) {
@@ -195,6 +241,22 @@ func (i *Identity) AddVersion(version *Version) {
 func (i *Identity) Commit(repo repository.Repo) error {
 	// Todo: check for mismatch between memory and commited data
 
+	needCommit := false
+	for _, v := range i.versions {
+		if v.commitHash == "" {
+			needCommit = true
+			break
+		}
+	}
+
+	if !needCommit {
+		return fmt.Errorf("can't commit an identity with no pending version")
+	}
+
+	if err := i.Validate(); err != nil {
+		return errors.Wrap(err, "can't commit an identity with invalid data")
+	}
+
 	for _, v := range i.versions {
 		if v.commitHash != "" {
 			i.lastCommit = v.commitHash
@@ -299,7 +361,7 @@ func (i *Identity) Merge(repo repository.Repo, other *Identity) (bool, error) {
 		// we have a non fast-forward merge.
 		// as explained in the doc above, refusing to merge
 		if i.versions[j].commitHash != otherVersion.commitHash {
-			return false, errors.New("non fast-forward identity merge")
+			return false, ErrNonFastForwardMerge
 		}
 	}
 

identity/identity_actions.go 🔗

@@ -12,7 +12,7 @@ import (
 // This does not change the local identities state
 func Fetch(repo repository.Repo, remote string) (string, error) {
 	remoteRefSpec := fmt.Sprintf(identityRemoteRefPattern, remote)
-	fetchRefSpec := fmt.Sprintf("%s:%s*", identityRefPattern, remoteRefSpec)
+	fetchRefSpec := fmt.Sprintf("%s*:%s*", identityRefPattern, remoteRefSpec)
 
 	return repo.FetchRefs(remote, fetchRefSpec)
 }
@@ -23,8 +23,7 @@ func Push(repo repository.Repo, remote string) (string, error) {
 }
 
 // Pull will do a Fetch + MergeAll
-// This function won't give details on the underlying process. If you need more,
-// use Fetch and MergeAll separately.
+// This function will return an error if a merge fail
 func Pull(repo repository.ClockedRepo, remote string) error {
 	_, err := Fetch(repo, remote)
 	if err != nil {
@@ -36,9 +35,7 @@ func Pull(repo repository.ClockedRepo, remote string) error {
 			return merge.Err
 		}
 		if merge.Status == MergeStatusInvalid {
-			// Not awesome: simply output the merge failure here as this function
-			// is only used in tests for now.
-			fmt.Println(merge)
+			return errors.Errorf("merge failure: %s", merge.Reason)
 		}
 	}
 
@@ -64,7 +61,7 @@ func MergeAll(repo repository.ClockedRepo, remote string) <-chan MergeResult {
 			refSplitted := strings.Split(remoteRef, "/")
 			id := refSplitted[len(refSplitted)-1]
 
-			remoteIdentity, err := ReadLocal(repo, remoteRef)
+			remoteIdentity, err := read(repo, remoteRef)
 
 			if err != nil {
 				out <- newMergeInvalidStatus(id, errors.Wrap(err, "remote identity is not readable").Error())

identity/identity_actions_test.go 🔗

@@ -0,0 +1,151 @@
+package identity
+
+import (
+	"testing"
+
+	"github.com/MichaelMure/git-bug/util/test"
+	"github.com/stretchr/testify/require"
+)
+
+func TestPushPull(t *testing.T) {
+	repoA, repoB, remote := test.SetupReposAndRemote(t)
+	defer test.CleanupRepos(repoA, repoB, remote)
+
+	identity1 := NewIdentity("name1", "email1")
+	err := identity1.Commit(repoA)
+	require.NoError(t, err)
+
+	// A --> remote --> B
+	_, err = Push(repoA, "origin")
+	require.NoError(t, err)
+
+	err = Pull(repoB, "origin")
+	require.NoError(t, err)
+
+	identities := allIdentities(t, ReadAllLocalIdentities(repoB))
+
+	if len(identities) != 1 {
+		t.Fatal("Unexpected number of bugs")
+	}
+
+	// B --> remote --> A
+	identity2 := NewIdentity("name2", "email2")
+	err = identity2.Commit(repoB)
+	require.NoError(t, err)
+
+	_, err = Push(repoB, "origin")
+	require.NoError(t, err)
+
+	err = Pull(repoA, "origin")
+	require.NoError(t, err)
+
+	identities = allIdentities(t, ReadAllLocalIdentities(repoA))
+
+	if len(identities) != 2 {
+		t.Fatal("Unexpected number of bugs")
+	}
+
+	// Update both
+
+	identity1.AddVersion(&Version{
+		name:  "name1b",
+		email: "email1b",
+	})
+	err = identity1.Commit(repoA)
+	require.NoError(t, err)
+
+	identity2.AddVersion(&Version{
+		name:  "name2b",
+		email: "email2b",
+	})
+	err = identity2.Commit(repoB)
+	require.NoError(t, err)
+
+	//  A --> remote --> B
+
+	_, err = Push(repoA, "origin")
+	require.NoError(t, err)
+
+	err = Pull(repoB, "origin")
+	require.NoError(t, err)
+
+	identities = allIdentities(t, ReadAllLocalIdentities(repoB))
+
+	if len(identities) != 2 {
+		t.Fatal("Unexpected number of bugs")
+	}
+
+	// B --> remote --> A
+
+	_, err = Push(repoB, "origin")
+	require.NoError(t, err)
+
+	err = Pull(repoA, "origin")
+	require.NoError(t, err)
+
+	identities = allIdentities(t, ReadAllLocalIdentities(repoA))
+
+	if len(identities) != 2 {
+		t.Fatal("Unexpected number of bugs")
+	}
+
+	// Concurrent update
+
+	identity1.AddVersion(&Version{
+		name:  "name1c",
+		email: "email1c",
+	})
+	err = identity1.Commit(repoA)
+	require.NoError(t, err)
+
+	identity1B, err := ReadLocal(repoB, identity1.Id())
+	require.NoError(t, err)
+
+	identity1B.AddVersion(&Version{
+		name:  "name1concurrent",
+		email: "email1concurrent",
+	})
+	err = identity1B.Commit(repoB)
+	require.NoError(t, err)
+
+	//  A --> remote --> B
+
+	_, err = Push(repoA, "origin")
+	require.NoError(t, err)
+
+	// Pulling a non-fast-forward update should fail
+	err = Pull(repoB, "origin")
+	require.Error(t, err)
+
+	identities = allIdentities(t, ReadAllLocalIdentities(repoB))
+
+	if len(identities) != 2 {
+		t.Fatal("Unexpected number of bugs")
+	}
+
+	// B --> remote --> A
+
+	// Pushing a non-fast-forward update should fail
+	_, err = Push(repoB, "origin")
+	require.Error(t, err)
+
+	err = Pull(repoA, "origin")
+	require.NoError(t, err)
+
+	identities = allIdentities(t, ReadAllLocalIdentities(repoA))
+
+	if len(identities) != 2 {
+		t.Fatal("Unexpected number of bugs")
+	}
+}
+
+func allIdentities(t testing.TB, identities <-chan StreamedIdentity) []*Identity {
+	var result []*Identity
+	for streamed := range identities {
+		if streamed.Err != nil {
+			t.Fatal(streamed.Err)
+		}
+		result = append(result, streamed.Identity)
+	}
+	return result
+}

identity/identity_stub_test.go 🔗

@@ -0,0 +1,23 @@
+package identity
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestIdentityStubSerialize(t *testing.T) {
+	before := &IdentityStub{
+		id: "id1234",
+	}
+
+	data, err := json.Marshal(before)
+	assert.NoError(t, err)
+
+	var after IdentityStub
+	err = json.Unmarshal(data, &after)
+	assert.NoError(t, err)
+
+	assert.Equal(t, before, &after)
+}

identity/version_test.go 🔗

@@ -0,0 +1,42 @@
+package identity
+
+import (
+	"encoding/json"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestVersionSerialize(t *testing.T) {
+	before := &Version{
+		login:     "login",
+		name:      "name",
+		email:     "email",
+		avatarURL: "avatarUrl",
+		keys: []Key{
+			{
+				Fingerprint: "fingerprint1",
+				PubKey:      "pubkey1",
+			},
+			{
+				Fingerprint: "fingerprint2",
+				PubKey:      "pubkey2",
+			},
+		},
+		nonce: makeNonce(20),
+		metadata: map[string]string{
+			"key1": "value1",
+			"key2": "value2",
+		},
+		time: 3,
+	}
+
+	data, err := json.Marshal(before)
+	assert.NoError(t, err)
+
+	var after Version
+	err = json.Unmarshal(data, &after)
+	assert.NoError(t, err)
+
+	assert.Equal(t, before, &after)
+}

tests/read_bugs_test.go 🔗

@@ -4,11 +4,25 @@ import (
 	"testing"
 
 	"github.com/MichaelMure/git-bug/bug"
+	"github.com/MichaelMure/git-bug/misc/random_bugs"
+	"github.com/MichaelMure/git-bug/repository"
 	"github.com/MichaelMure/git-bug/util/test"
 )
 
+func CreateFilledRepo(bugNumber int) repository.ClockedRepo {
+	repo := test.CreateRepo(false)
+
+	var seed int64 = 42
+	options := random_bugs.DefaultOptions()
+
+	options.BugNumber = bugNumber
+
+	random_bugs.CommitRandomBugsWithSeed(repo, options, seed)
+	return repo
+}
+
 func TestReadBugs(t *testing.T) {
-	repo := test.CreateFilledRepo(15)
+	repo := CreateFilledRepo(15)
 	bugs := bug.ReadAllLocalBugs(repo)
 	for b := range bugs {
 		if b.Err != nil {
@@ -18,7 +32,7 @@ func TestReadBugs(t *testing.T) {
 }
 
 func benchmarkReadBugs(bugNumber int, t *testing.B) {
-	repo := test.CreateFilledRepo(bugNumber)
+	repo := CreateFilledRepo(bugNumber)
 	t.ResetTimer()
 
 	for n := 0; n < t.N; n++ {

util/test/repo.go 🔗

@@ -3,8 +3,9 @@ package test
 import (
 	"io/ioutil"
 	"log"
+	"os"
+	"testing"
 
-	"github.com/MichaelMure/git-bug/misc/random_bugs"
 	"github.com/MichaelMure/git-bug/repository"
 )
 
@@ -39,14 +40,40 @@ func CreateRepo(bare bool) *repository.GitRepo {
 	return repo
 }
 
-func CreateFilledRepo(bugNumber int) repository.ClockedRepo {
-	repo := CreateRepo(false)
+func CleanupRepo(repo repository.Repo) error {
+	path := repo.GetPath()
+	// fmt.Println("Cleaning repo:", path)
+	return os.RemoveAll(path)
+}
 
-	var seed int64 = 42
-	options := random_bugs.DefaultOptions()
+func SetupReposAndRemote(t testing.TB) (repoA, repoB, remote *repository.GitRepo) {
+	repoA = CreateRepo(false)
+	repoB = CreateRepo(false)
+	remote = CreateRepo(true)
 
-	options.BugNumber = bugNumber
+	remoteAddr := "file://" + remote.GetPath()
 
-	random_bugs.CommitRandomBugsWithSeed(repo, options, seed)
-	return repo
+	err := repoA.AddRemote("origin", remoteAddr)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = repoB.AddRemote("origin", remoteAddr)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	return repoA, repoB, remote
+}
+
+func CleanupRepos(repoA, repoB, remote *repository.GitRepo) {
+	if err := CleanupRepo(repoA); err != nil {
+		log.Println(err)
+	}
+	if err := CleanupRepo(repoB); err != nil {
+		log.Println(err)
+	}
+	if err := CleanupRepo(remote); err != nil {
+		log.Println(err)
+	}
 }