internal/gerrit: support setting any field from a txtar file
Change-Id: I84fb44e45a61231303c3bbb91b95b59a2e477c69
Reviewed-on: https://go-review.googlesource.com/c/oscar/+/617315
Commit-Queue: Ian Lance Taylor <iant@google.com>
Reviewed-by: Ian Lance Taylor <iant@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
Reviewed-by: Zvonimir Pavlinovic <zpavlinovic@google.com>
diff --git a/internal/gerrit/change.go b/internal/gerrit/change.go
index 59d7d46..78ca091 100644
--- a/internal/gerrit/change.go
+++ b/internal/gerrit/change.go
@@ -109,7 +109,7 @@
}
if abandoned.IsZero() {
c.slog.Error("gerrit change abandoned missing message", "num", ch.num, "data", ch.data)
- c.db.Panic("gerrit change abandoned missing message for %d", ch.num)
+ c.db.Panic("gerrit change abandoned missing message", "num", ch.num)
}
}
@@ -160,7 +160,7 @@
rev, ok := revisions.Revisions[revisions.CurrentRevision]
if !ok {
c.slog.Error("gerrit no revision data for current revision", "num", ch.num, "data", ch.data, "currentRevision", revisions.CurrentRevision)
- c.db.Panic("gerrit no revision data for current revision for %d", ch.num)
+ c.db.Panic("gerrit no revision data for current revision", "num", ch.num)
}
return rev.Commit.Message
diff --git a/internal/gerrit/sync_test.go b/internal/gerrit/sync_test.go
index 6becb42..82ee675 100644
--- a/internal/gerrit/sync_test.go
+++ b/internal/gerrit/sync_test.go
@@ -100,6 +100,14 @@
}
}
+// changeTests is a list of tests to run on a change.
+type changeTests struct {
+ name string
+ accessor func(*Change) any
+ want any
+ eq func(any, any) bool
+}
+
// accessor is one of the accessor methods to retrieve Change values.
type accessor[T any] func(*Change) T
@@ -110,6 +118,23 @@
}
}
+// testChangeTests checks that a [Change] satisfies a list of [changeTests].
+func testChangeTests(t *testing.T, ch *Change, tests []changeTests) {
+ t.Helper()
+ for _, test := range tests {
+ got := test.accessor(ch)
+ var ok bool
+ if test.eq == nil {
+ ok = got == test.want
+ } else {
+ ok = test.eq(got, test.want)
+ }
+ if !ok {
+ t.Errorf("%s got %v, want %v", test.name, got, test.want)
+ }
+ }
+}
+
// checkFirstCL checks the first CL in our saved sync against
// the values we expect. The first CL is https://go.dev/cl/24894.
func checkFirstCL(t *testing.T, c *Client, ch *Change, num int) {
@@ -117,12 +142,7 @@
t.Errorf("got first CL number %d, want 24894", num)
}
- tests := []struct {
- name string
- accessor func(*Change) any
- want any
- eq func(any, any) bool
- }{
+ tests := []changeTests{
{
"ChangeNumber",
wa(c.ChangeNumber),
@@ -310,18 +330,7 @@
},
}
- for _, test := range tests {
- got := test.accessor(ch)
- var ok bool
- if test.eq == nil {
- ok = got == test.want
- } else {
- ok = test.eq(got, test.want)
- }
- if !ok {
- t.Errorf("%s got %v, want %v", test.name, got, test.want)
- }
- }
+ testChangeTests(t, ch, tests)
}
// checkChange verifies that we can unpack CL information, and that it
diff --git a/internal/gerrit/testdata/change.txt b/internal/gerrit/testdata/change.txt
new file mode 100644
index 0000000..0faab16
--- /dev/null
+++ b/internal/gerrit/testdata/change.txt
@@ -0,0 +1,22 @@
+-- change#1 --
+Number: 1
+Labels: Hold
+ Optional: true
+ Description: put change on hold
+ Values: 0: not on hold
+ Values: 1: on hold
+ Votes: 0
+Owner: gopher@golang.org
+CurrentRevision: commitid1
+Revisions: commitid1
+ Number: 1
+ Commit:
+ Subject: patch set 1
+ Message: gerrit: test change
+ Author:
+ Email: gopher@golang.org
+Hashtags: tag1 tag2
+Messages:
+ Message: message 1
+Messages:
+ Message: message 2
diff --git a/internal/gerrit/testing.go b/internal/gerrit/testing.go
index 11ed6b6..c3f49f0 100644
--- a/internal/gerrit/testing.go
+++ b/internal/gerrit/testing.go
@@ -11,6 +11,7 @@
"fmt"
"iter"
"os"
+ "reflect"
"strconv"
"strings"
"testing"
@@ -69,6 +70,12 @@
return err
}
+// timeStampType is the [reflect.Type] of [TimeStamp].
+var timeStampType = reflect.TypeFor[TimeStamp]()
+
+// accountInfoType is the [reflect.Type] of [*AccountInfo].
+var accountInfoType = reflect.TypeFor[*AccountInfo]()
+
// LoadTxtarData loads a change info history from the txtar file content data.
// See [LoadTxtar] for a description of the format.
func (tc *TestingClient) LoadTxtarData(data []byte) error {
@@ -77,48 +84,212 @@
data := string(file.Data)
// Skip the name and proceed to read headers.
c := &ChangeInfo{}
- for {
- line, rest, _ := strings.Cut(data, "\n")
- data = rest
- if line == "" {
- break
- }
- key, val, ok := strings.Cut(line, ":")
- if !ok {
- return fmt.Errorf("%s: invalid header line: %q", file.Name, line)
- }
- val = strings.TrimSpace(val)
- if val == "" {
- continue
- }
- switch key {
- case "Number":
- i, err := strconv.Atoi(val)
- if err != nil {
- return err
- }
- c.Number = i
- case "Updated":
- t, err := timestamp(val)
- if err != nil {
- return err
- }
- c.Updated = t
- case "MetaRevID":
- c.MetaRevID = val
- case "interrupt":
- b, err := strconv.ParseBool(val)
- if err != nil {
- return err
- }
- c.interrupt = b
- }
+ cv := reflect.ValueOf(c).Elem()
+ if _, err := tc.setFields(file.Name, data, 0, cv); err != nil {
+ return err
}
tc.chs = append(tc.chs, c)
}
return nil
}
+// setFields reads field values and sets fields in a struct accordingly.
+// The indent parameter says how many spaces are required on each line;
+// this supports fields that are themselves structs.
+// This returns the remaining data.
+func (tc *TestingClient) setFields(filename, data string, indent int, st reflect.Value) (string, error) {
+ prefix := strings.Repeat(" ", indent)
+ for {
+ line, rest, _ := strings.Cut(data, "\n")
+ if line == "" {
+ data = rest
+ break
+ }
+ line, ok := strings.CutPrefix(line, prefix)
+ if !ok {
+ // Don't change data.
+ break
+ }
+ data = rest
+ rest, err := tc.setField(filename, line, data, indent, st)
+ if err != nil {
+ return "", err
+ }
+ data = rest
+ }
+ return data, nil
+}
+
+// setField takes a struct and a line that sets a scalar field.
+// The line should have the form "key: value",
+// where "key" is the name of a field in the struct and
+// "value is the value we want to set it to.
+// This isn't general, it only handles the cases that arise
+// for Gerrit types.
+// The data argument is the data following the line,
+// used for multi-line values.
+// This returns the remaining data.
+func (tc *TestingClient) setField(filename string, line, data string, indent int, st reflect.Value) (string, error) {
+ key, val, ok := strings.Cut(line, ":")
+ if !ok {
+ return "", fmt.Errorf("%s: invalid line: %q", filename, line)
+ }
+ val = strings.TrimSpace(val)
+
+ field := st.FieldByName(key)
+ if !field.IsValid() {
+ return "", fmt.Errorf("%s: unrecognized field name %q in %s", filename, key, st.Type())
+ }
+
+ var vval reflect.Value
+ switch field.Type().Kind() {
+ case reflect.Bool:
+ b, err := strconv.ParseBool(val)
+ if err != nil {
+ return "", fmt.Errorf("%s: field %q: can't parse %q as bool", filename, key, val)
+ }
+ vval = reflect.ValueOf(b)
+
+ case reflect.Int:
+ i, err := strconv.Atoi(val)
+ if err != nil {
+ return "", fmt.Errorf("%s: field %q: can't parse %q as int", filename, key, val)
+ }
+ vval = reflect.ValueOf(i)
+
+ case reflect.String:
+ vval = reflect.ValueOf(val)
+
+ case reflect.Struct:
+ if field.Type() == timeStampType {
+ t, err := timestamp(val)
+ if err != nil {
+ return "", fmt.Errorf("%s: field %q: can't parse %q as timestamp", filename, key, val)
+ }
+ vval = reflect.ValueOf(TimeStamp(t))
+ break
+ }
+ return "", fmt.Errorf("%s: field %q: unexpected struct type %s", filename, key, field.Type())
+
+ case reflect.Pointer:
+ if field.Type() == accountInfoType {
+ // For an account we just an email address.
+ if val == "" {
+ return "", fmt.Errorf("%s: field %q: missing email address for AccountInfo", filename, key)
+ }
+ vval = reflect.ValueOf(&AccountInfo{Email: val})
+ break
+ }
+ if field.Type().Elem().Kind() != reflect.Struct {
+ return "", fmt.Errorf("%s: field %q: unexpected pointer to %s", filename, key, field.Type().Elem())
+ }
+
+ // For struct types in general we expect the fields
+ // to follow, indented.
+ vval = reflect.New(field.Type().Elem())
+ rest, err := tc.setFields(filename, data, indent+1, vval.Elem())
+ if err != nil {
+ return "", err
+ }
+ data = rest
+ break
+
+ case reflect.Slice:
+ switch field.Type().Elem().Kind() {
+ case reflect.Int:
+ // For ints just put all the values on one line.
+ var ints []int
+ for _, vi := range strings.Fields(val) {
+ i, err := strconv.Atoi(vi)
+ if err != nil {
+ return "", fmt.Errorf("%s: field %q: %v", filename, key, err)
+ }
+ ints = append(ints, i)
+ }
+ vval = reflect.ValueOf(ints)
+
+ case reflect.String:
+ // For strings just put all the values on one line.
+ // Strings are space separated, no quoting.
+ var strs []string
+ for _, vs := range strings.Fields(val) {
+ vs = strings.TrimSpace(vs)
+ if vs == "" {
+ continue
+ }
+ strs = append(strs, vs)
+ }
+ vval = reflect.ValueOf(strs)
+
+ case reflect.Pointer:
+ if field.Type().Elem().Elem().Kind() != reflect.Struct {
+ return "", fmt.Errorf("%s: field %q: pointer not to struct in type %s", filename, key, field.Type())
+ }
+ vval = reflect.New(field.Type().Elem().Elem())
+ rest, err := tc.setFields(filename, data, indent+1, vval.Elem())
+ if err != nil {
+ return "", err
+ }
+ data = rest
+ vval = reflect.Append(field, vval)
+
+ default:
+ return "", fmt.Errorf("%s: field %q: unsupported slice type %s", filename, key, field.Type())
+ }
+
+ case reflect.Map:
+ if field.Type().Key().Kind() != reflect.String {
+ return "", fmt.Errorf("%s: field %q: unsupported map key type in %s", filename, key, field.Type())
+ }
+
+ if field.IsZero() {
+ field.Set(reflect.MakeMap(field.Type()))
+ }
+
+ switch field.Type().Elem().Kind() {
+ case reflect.String:
+ mkey, mval, ok := strings.Cut(val, ":")
+ if !ok {
+ return "", fmt.Errorf("%s: field %q: expected key: val in map to string", filename, key)
+ }
+ mkey = strings.TrimSpace(mkey)
+ mval = strings.TrimSpace(mval)
+ field.SetMapIndex(reflect.ValueOf(mkey), reflect.ValueOf(mval))
+ // Don't fall through to bottom of function.
+ return data, nil
+
+ case reflect.Pointer:
+ if field.Type().Elem().Elem().Kind() != reflect.Struct {
+ return "", fmt.Errorf("%s: field %q: pointer not to struct in map element type in %s", filename, key, field.Type())
+ }
+ vval = reflect.New(field.Type().Elem().Elem())
+ rest, err := tc.setFields(filename, data, indent+1, vval.Elem())
+ if err != nil {
+ return "", err
+ }
+ data = rest
+ field.SetMapIndex(reflect.ValueOf(val), vval)
+ // Don't fall through to bottom of function.
+ return data, nil
+
+ default:
+ return "", fmt.Errorf("%s: field %q: unsupported map element type in %s", filename, key, field.Type())
+ }
+
+ default:
+ return "", fmt.Errorf("%s: field %q: unsupported type %s", filename, key, field.Type())
+ }
+
+ if key == "interrupt" {
+ // Special case for the only unexported field.
+ st.Addr().Interface().(*ChangeInfo).interrupt = vval.Bool()
+ } else {
+ field.Set(vval)
+ }
+
+ return data, nil
+}
+
// changes returns an iterator of change updates in tc.chs that are updated
// in the interval [after, before], in reverse chronological order. First
// skip number of matching change updates are disregarded.
@@ -194,6 +365,19 @@
return ain && bin, nil
}
+// change returns the data for a single testing change.
+func (tc *TestingClient) change(changeNum int) *Change {
+ for _, ch := range tc.chs {
+ if ch.Number == changeNum {
+ return &Change{
+ num: changeNum,
+ data: storage.JSON(ch),
+ }
+ }
+ }
+ return nil
+}
+
func timestamp(gt string) (TimeStamp, error) {
var ts TimeStamp
if err := ts.UnmarshalJSON([]byte(quote(gt))); err != nil {
diff --git a/internal/gerrit/testing_test.go b/internal/gerrit/testing_test.go
index a6418a3..9c12dd8 100644
--- a/internal/gerrit/testing_test.go
+++ b/internal/gerrit/testing_test.go
@@ -6,11 +6,94 @@
import (
"context"
+ "maps"
+ "slices"
"testing"
+ "golang.org/x/oscar/internal/secret"
+ "golang.org/x/oscar/internal/storage"
"golang.org/x/oscar/internal/testutil"
)
+func TestLoadTxtar(t *testing.T) {
+ check := testutil.Checker(t)
+
+ lg := testutil.Slogger(t)
+ db := storage.MemDB()
+ sdb := secret.Empty()
+ c := New("gerrit-test", lg, db, sdb, nil)
+
+ tc := c.Testing()
+ check(tc.LoadTxtar("testdata/change.txt"))
+ ch := tc.change(1)
+ if ch == nil {
+ t.Fatal("could not find loaded change")
+ }
+
+ tests := []changeTests{
+ {
+ "ChangeLabels",
+ wa(c.ChangeLabels),
+ map[string]string{
+ "Hold": "put change on hold",
+ },
+ func(got, want any) bool {
+ g := got.(map[string]*LabelInfo)
+ w := want.(map[string]string)
+ m := make(map[string]string)
+ for k, l := range g {
+ m[k] = l.Description
+ }
+ return maps.Equal(w, m)
+ },
+ },
+ {
+ "ChangeOwner",
+ wa(c.ChangeOwner),
+ "gopher@golang.org",
+ func(got, want any) bool {
+ return got.(*AccountInfo).Email == want
+ },
+ },
+ {
+ "ChangeDescription",
+ wa(c.ChangeDescription),
+ "gerrit: test change",
+ nil,
+ },
+ {
+ "ChangeHashtags",
+ wa(c.ChangeHashtags),
+ []string{"tag1", "tag2"},
+ func(got, want any) bool {
+ g := got.([]string)
+ w := want.([]string)
+ return slices.Equal(g, w)
+ },
+ },
+ {
+ "ChangeMessages",
+ wa(c.ChangeMessages),
+ []string{
+ "message 1",
+ "message 2",
+ },
+ func(got, want any) bool {
+ g := got.([]ChangeMessageInfo)
+ w := want.([]string)
+ for i, m := range g {
+ if m.Message != w[i] {
+ return false
+ }
+ }
+ return true
+ },
+ },
+ }
+
+ testChangeTests(t, ch, tests)
+}
+
func TestTestingChanges(t *testing.T) {
check := testutil.Checker(t)
ctx := context.Background()
diff --git a/internal/gerrit/types.go b/internal/gerrit/types.go
index 05fad19..196b820 100644
--- a/internal/gerrit/types.go
+++ b/internal/gerrit/types.go
@@ -28,13 +28,13 @@
// The map that maps account IDs to AttentionSetInfo of that
// account. Those are all accounts that are currently in the
// attention set.
- AttentionSet map[int]AttentionSetInfo `json:"attention_set,omitempty"`
+ AttentionSet map[int]*AttentionSetInfo `json:"attention_set,omitempty"`
// The map that maps account IDs to AttentionSetInfo of that
// account. Those are all accounts that were in the attention
// set but were removed. The AttentionSetInfo is the latest
// and most recent removal of the account from the attention set.
- RemovedFromAttentionSet map[int]AttentionSetInfo `json:"removed_from_attention_set,omitempty"`
+ RemovedFromAttentionSet map[int]*AttentionSetInfo `json:"removed_from_attention_set,omitempty"`
// List of hashtags that are set on the change.
Hashtags []string `json:"hashtags,omitempty"`
@@ -95,7 +95,7 @@
// List of the [SubmitRecordInfo] containing the submit records
// for the change at the latest patchset.
- SubmitRecords []SubmitRecordInfo `json:"submit_records"`
+ SubmitRecords []*SubmitRecordInfo `json:"submit_records"`
// The labels of the change as a map that maps the label names
// to LabelInfo entries.
@@ -119,7 +119,7 @@
PendingReviewers map[string][]*AccountInfo `json:"pending_reviewers,omitempty"`
// Messages associated with the change.
- Messages []ChangeMessageInfo `json:"messages,omitempty"`
+ Messages []*ChangeMessageInfo `json:"messages,omitempty"`
// The number of the current patch set of this change.
CurrentRevisionNumber int `json:"current_revision_number"`
@@ -129,7 +129,7 @@
// All patch sets of this change as a map that maps the commit
// ID of the patch set to a [RevisionInfo] entity.
- Revisions map[string]RevisionInfo `json:"revisions,omitempty"`
+ Revisions map[string]*RevisionInfo `json:"revisions,omitempty"`
// The SHA-1 of the NoteDb meta ref.
MetaRevID string `json:"meta_rev_id,omitempty"`
