test: verify shamhub behavior compliance (#974)

Take advantage of the generalized integration tests
which verify behavior of github and gitlab
to also verify that our test shamhub implementation
complies with the same behavior.

This is necessary since we use shamhub
for all of git-spice's own integration tests
so any divergence in behavior would be problematic.

This found minor discrepancies in ShamHub behavior that have been fixed.

Author: abhinav

internal/forge/shamhub/auth.go

@@ -65,6 +65,27 @@ func (sh *ShamHub) RegisterUser(username string) error {
 	return nil
 }
 
+// IssueToken issues an authentication token for the given username.
+// The user must already be registered.
+// This is a test helper method.
+func (sh *ShamHub) IssueToken(username string) (string, error) {
+	var buf [8]byte
+	_, _ = rand.Read(buf[:])
+	token := hex.EncodeToString(buf[:])
+
+	sh.mu.Lock()
+	defer sh.mu.Unlock()
+
+	for _, u := range sh.users {
+		if u.Username == username {
+			sh.tokens[token] = username
+			return token, nil
+		}
+	}
+
+	return "", fmt.Errorf("user %q not found", username)
+}
+
 // AuthenticationToken defines the token returned by the ShamHub forge.
 type AuthenticationToken struct {
 	forge.AuthenticationToken

internal/forge/shamhub/comment.go

@@ -217,10 +217,12 @@ func (sh *ShamHub) handleListChangeComments(_ context.Context, req *listChangeCo
 	}, nil
 }
 
+var _listChangeCommentsPageSize = 10 // var for testing
+
 func (r *forgeRepository) ListChangeComments(
 	ctx context.Context,
 	id forge.ChangeID,
-	_ *forge.ListChangeCommentsOptions,
+	opts *forge.ListChangeCommentsOptions,
 ) iter.Seq2[*forge.ListChangeCommentItem, error] {
 	u := r.apiURL.JoinPath(r.owner, r.repo, "comments")
 	q := u.Query()
@@ -231,7 +233,7 @@ func (r *forgeRepository) ListChangeComments(
 		offset := 0
 		for {
 			q.Set("offset", strconv.Itoa(offset))
-			q.Set("limit", "10")
+			q.Set("limit", strconv.Itoa(_listChangeCommentsPageSize))
 			u.RawQuery = q.Encode()
 
 			var res listChangeCommentsResponse
@@ -241,6 +243,23 @@ func (r *forgeRepository) ListChangeComments(
 			}
 
 			for _, item := range res.Items {
+				// Apply filtering if options are provided.
+				if opts != nil {
+					// Filter by BodyMatchesAll patterns.
+					if len(opts.BodyMatchesAll) > 0 {
+						matches := true
+						for _, pattern := range opts.BodyMatchesAll {
+							if !pattern.MatchString(item.Body) {
+								matches = false
+								break
+							}
+						}
+						if !matches {
+							continue
+						}
+					}
+				}
+
 				if !yield(&forge.ListChangeCommentItem{
 					ID:   ChangeCommentID(item.ID),
 					Body: item.Body,

internal/forge/shamhub/comment_test.go

@@ -0,0 +1,13 @@
+package shamhub
+
+import (
+	"testing"
+
+	"go.abhg.dev/testing/stub"
+)
+
+// SetListChangeCommentsPageSize sets the page size for listing change comments.
+// This is used to test pagination.
+func SetListChangeCommentsPageSize(t testing.TB, pageSize int) {
+	t.Cleanup(stub.Value(&_listChangeCommentsPageSize, pageSize))
+}

internal/forge/shamhub/edit.go

@@ -81,6 +81,20 @@ func (sh *ShamHub) handleEditChange(_ context.Context, req *editChangeRequest) (
 	}
 
 	if len(req.Assignees) > 0 {
+		// Validate that all assignees are registered users.
+		for _, assignee := range req.Assignees {
+			found := false
+			for _, u := range sh.users {
+				if u.Username == assignee {
+					found = true
+					break
+				}
+			}
+			if !found {
+				return nil, badRequestErrorf("assignee %q is not a registered user", assignee)
+			}
+		}
+
 		assignees := sh.changes[changeIdx].Assignees
 		for _, assignee := range req.Assignees {
 			if !slices.Contains(assignees, assignee) {

internal/forge/shamhub/find.go

@@ -108,47 +108,58 @@ nextChange:
 	return changes, nil
 }
 
-func (r *forgeRepository) FindChangeByID(ctx context.Context, fid forge.ChangeID) (*forge.FindChangeItem, error) {
-	id := fid.(ChangeID)
-	u := r.apiURL.JoinPath(r.owner, r.repo, "change", strconv.Itoa(int(id)))
-	var res Change
-	if err := r.client.Get(ctx, u.String(), &res); err != nil {
-		return nil, fmt.Errorf("find change by ID: %w", err)
-	}
-
+// toFindChangeItem converts a Change to a forge.FindChangeItem.
+func toFindChangeItem(c *Change) *forge.FindChangeItem {
 	var state forge.ChangeState
-	switch res.State {
+	switch c.State {
 	case "open":
 		state = forge.ChangeOpen
 	case "closed":
-		if res.Merged {
+		if c.Merged {
 			state = forge.ChangeMerged
 		} else {
 			state = forge.ChangeClosed
 		}
 	}
 
-	labels := res.Labels
+	labels := c.Labels
 	if len(labels) == 0 {
 		labels = nil
 	}
 
-	reviewers := res.RequestedReviewers
+	reviewers := c.RequestedReviewers
 	if len(reviewers) == 0 {
 		reviewers = nil
 	}
 
+	assignees := c.Assignees
+	if len(assignees) == 0 {
+		assignees = nil
+	}
+
 	return &forge.FindChangeItem{
-		ID:        ChangeID(res.Number),
-		URL:       res.URL,
-		Subject:   res.Subject,
-		HeadHash:  git.Hash(res.Head.Hash),
-		BaseName:  res.Base.Name,
-		Draft:     res.Draft,
+		ID:        ChangeID(c.Number),
+		URL:       c.URL,
+		Subject:   c.Subject,
+		HeadHash:  git.Hash(c.Head.Hash),
+		BaseName:  c.Base.Name,
+		Draft:     c.Draft,
 		State:     state,
 		Labels:    labels,
 		Reviewers: reviewers,
-	}, nil
+		Assignees: assignees,
+	}
+}
+
+func (r *forgeRepository) FindChangeByID(ctx context.Context, fid forge.ChangeID) (*forge.FindChangeItem, error) {
+	id := fid.(ChangeID)
+	u := r.apiURL.JoinPath(r.owner, r.repo, "change", strconv.Itoa(int(id)))
+	var res Change
+	if err := r.client.Get(ctx, u.String(), &res); err != nil {
+		return nil, fmt.Errorf("find change by ID: %w", err)
+	}
+
+	return toFindChangeItem(&res), nil
 }
 
 func (r *forgeRepository) FindChangesByBranch(ctx context.Context, branch string, opts forge.FindChangesOptions) ([]*forge.FindChangeItem, error) {
@@ -173,39 +184,7 @@ func (r *forgeRepository) FindChangesByBranch(ctx context.Context, branch string
 
 	changes := make([]*forge.FindChangeItem, len(res))
 	for i, c := range res {
-		var state forge.ChangeState
-		switch c.State {
-		case "open":
-			state = forge.ChangeOpen
-		case "closed":
-			if c.Merged {
-				state = forge.ChangeMerged
-			} else {
-				state = forge.ChangeClosed
-			}
-		}
-
-		labels := c.Labels
-		if len(labels) == 0 {
-			labels = nil
-		}
-
-		reviewers := c.RequestedReviewers
-		if len(reviewers) == 0 {
-			reviewers = nil
-		}
-
-		changes[i] = &forge.FindChangeItem{
-			ID:        ChangeID(c.Number),
-			URL:       c.URL,
-			State:     state,
-			Subject:   c.Subject,
-			HeadHash:  git.Hash(c.Head.Hash),
-			BaseName:  c.Base.Name,
-			Draft:     c.Draft,
-			Labels:    labels,
-			Reviewers: reviewers,
-		}
+		changes[i] = toFindChangeItem(c)
 	}
 	return changes, nil
 }

internal/forge/shamhub/forge.go

@@ -71,14 +71,20 @@ func (f *Forge) ParseRemoteURL(remoteURL string) (forge.RepositoryID, error) {
 
 // OpenRepository opens the repository that this repository ID points to.
 func (f *Forge) OpenRepository(_ context.Context, token forge.AuthenticationToken, id forge.RepositoryID) (forge.Repository, error) {
+	return newRepository(f, token.(*AuthenticationToken), id.(*RepositoryID), http.DefaultClient)
+}
+
+// newRepository creates a new repository instance with the given HTTP client.
+func newRepository(f *Forge, token *AuthenticationToken, rid *RepositoryID, httpClient *http.Client) (forge.Repository, error) {
 	must.NotBeBlankf(f.URL, "URL is required")
 	must.NotBeBlankf(f.APIURL, "API URL is required")
 
-	rid := id.(*RepositoryID)
-	tok := token.(*AuthenticationToken).tok
-	client := f.jsonHTTPClient()
-	client.headers = map[string]string{
-		"Authentication-Token": tok,
+	client := &jsonHTTPClient{
+		log:    f.Log,
+		client: httpClient,
+		headers: map[string]string{
+			"Authentication-Token": token.tok,
+		},
 	}
 
 	apiURL, err := url.Parse(f.APIURL)