From f5d7b54b0280c84ed9ced99ece38e769f4c0df11 Mon Sep 17 00:00:00 2001 From: Adam Bettigole Date: Fri, 12 Jun 2026 11:08:27 -0700 Subject: [PATCH 1/3] adding Phabricator ChangeProvider --- .../changeprovider/phabricator/BUILD.bazel | 41 +++ .../changeprovider/phabricator/conduit.go | 121 +++++++ .../phabricator/conduit_test.go | 250 ++++++++++++++ .../changeprovider/phabricator/convert.go | 41 +++ .../phabricator/convert_test.go | 78 +++++ .../changeprovider/phabricator/provider.go | 148 +++++++++ .../phabricator/provider_test.go | 307 ++++++++++++++++++ .../changeprovider/phabricator/validate.go | 47 +++ .../phabricator/validate_test.go | 125 +++++++ 9 files changed, 1158 insertions(+) create mode 100644 submitqueue/extension/changeprovider/phabricator/BUILD.bazel create mode 100644 submitqueue/extension/changeprovider/phabricator/conduit.go create mode 100644 submitqueue/extension/changeprovider/phabricator/conduit_test.go create mode 100644 submitqueue/extension/changeprovider/phabricator/convert.go create mode 100644 submitqueue/extension/changeprovider/phabricator/convert_test.go create mode 100644 submitqueue/extension/changeprovider/phabricator/provider.go create mode 100644 submitqueue/extension/changeprovider/phabricator/provider_test.go create mode 100644 submitqueue/extension/changeprovider/phabricator/validate.go create mode 100644 submitqueue/extension/changeprovider/phabricator/validate_test.go diff --git a/submitqueue/extension/changeprovider/phabricator/BUILD.bazel b/submitqueue/extension/changeprovider/phabricator/BUILD.bazel new file mode 100644 index 00000000..8a7c3818 --- /dev/null +++ b/submitqueue/extension/changeprovider/phabricator/BUILD.bazel @@ -0,0 +1,41 @@ +load("@rules_go//go:def.bzl", "go_library", "go_test") + +go_library( + name = "phabricator", + srcs = [ + "conduit.go", + "convert.go", + "provider.go", + "validate.go", + ], + importpath = "github.com/uber/submitqueue/submitqueue/extension/changeprovider/phabricator", + visibility = ["//visibility:public"], + deps = [ + "//core/metrics", + "//submitqueue/entity", + "//submitqueue/entity/phabricator", + "//submitqueue/extension/changeprovider", + "@com_github_uber_go_tally//:tally", + "@org_uber_go_zap//:zap", + ], +) + +go_test( + name = "phabricator_test", + srcs = [ + "conduit_test.go", + "convert_test.go", + "provider_test.go", + "validate_test.go", + ], + embed = [":phabricator"], + deps = [ + "//submitqueue/entity", + "//submitqueue/entity/phabricator", + "//submitqueue/extension/changeprovider", + "@com_github_stretchr_testify//assert", + "@com_github_stretchr_testify//require", + "@com_github_uber_go_tally//:tally", + "@org_uber_go_zap//zaptest", + ], +) diff --git a/submitqueue/extension/changeprovider/phabricator/conduit.go b/submitqueue/extension/changeprovider/phabricator/conduit.go new file mode 100644 index 00000000..b1911da9 --- /dev/null +++ b/submitqueue/extension/changeprovider/phabricator/conduit.go @@ -0,0 +1,121 @@ +package phabricator + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strconv" +) + +// conduitResponse stores a response from the Conduit API. Every Conduit endpoint wraps its +// payload in this shape; a non-nil ErrorCode signals a server-side failure. +type conduitResponse struct { + Result json.RawMessage `json:"result"` + ErrorCode *string `json:"error_code"` + ErrorInfo *string `json:"error_info"` +} + +// diffResult represents a single diff from the differential.querydiffs response. +type diffResult struct { + Changes []fileChange `json:"changes"` + Properties properties `json:"properties"` + AuthorName string `json:"authorName"` + AuthorEmail string `json:"authorEmail"` +} + +// fileChange represents a single file modification in a Phabricator diff. +type fileChange struct { + CurrentPath string `json:"currentPath"` + AddLines string `json:"addLines"` + DelLines string `json:"delLines"` +} + +// properties holds diff metadata from the querydiffs response. +type properties struct { + LocalCommits map[string]localCommit `json:"local:commits"` +} + +// localCommit represents a commit entry from the local:commits property. +type localCommit struct { + Commit string `json:"commit"` +} + +// extractHeadSHA returns the head commit SHA from the diff's local:commits +// property. The first key of the map is the commit SHA. +func extractHeadSHA(diff *diffResult) (string, error) { + for sha := range diff.Properties.LocalCommits { + return sha, nil + } + return "", fmt.Errorf("no local commits found in diff properties") +} + +// buildQueryDiffsRequest builds query parameters for a differential.querydiffs call. +func buildQueryDiffsRequest(diffIDs []int, apiToken string) url.Values { + form := url.Values{} + for _, id := range diffIDs { + form.Add("ids[]", strconv.Itoa(id)) + } + if apiToken != "" { + form.Set("api.token", apiToken) + } + return form +} + +// doConduitRequest executes a Conduit HTTP request. +// The path is relative — transport layers on the client resolve it to the full URL. +// Authentication (e.g. Bearer token) is handled by the client's transport. +func doConduitRequest(ctx context.Context, httpClient *http.Client, form url.Values) (*http.Response, error) { + req, err := http.NewRequestWithContext(ctx, http.MethodPost, "/api/differential.querydiffs", nil) + if err != nil { + return nil, fmt.Errorf("failed to create HTTP request: %w", err) + } + req.URL.RawQuery = form.Encode() + + resp, err := httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("HTTP request failed: %w", err) + } + + return resp, nil +} + +// parseConduitResponse reads the HTTP response, unwraps the Conduit response, +// and extracts the diffResults for the requested diff IDs. +func parseConduitResponse(resp *http.Response, diffIDs []int) (map[int]*diffResult, error) { + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + return nil, fmt.Errorf("Conduit API returned status %d: %s", resp.StatusCode, string(body)) + } + + var phabResponse conduitResponse + if err := json.NewDecoder(resp.Body).Decode(&phabResponse); err != nil { + return nil, fmt.Errorf("failed to decode Conduit response: %w", err) + } + + if phabResponse.ErrorCode != nil { + info := "" + if phabResponse.ErrorInfo != nil { + info = *phabResponse.ErrorInfo + } + return nil, fmt.Errorf("Conduit error %s: %s", *phabResponse.ErrorCode, info) + } + + var rawMap map[string]*diffResult + if err := json.Unmarshal(phabResponse.Result, &rawMap); err != nil { + return nil, fmt.Errorf("failed to decode querydiffs result: %w", err) + } + + results := make(map[int]*diffResult, len(diffIDs)) + for _, id := range diffIDs { + diff, ok := rawMap[strconv.Itoa(id)] + if !ok { + return nil, fmt.Errorf("diff %d not found in querydiffs response", id) + } + results[id] = diff + } + + return results, nil +} diff --git a/submitqueue/extension/changeprovider/phabricator/conduit_test.go b/submitqueue/extension/changeprovider/phabricator/conduit_test.go new file mode 100644 index 00000000..5e5a68c6 --- /dev/null +++ b/submitqueue/extension/changeprovider/phabricator/conduit_test.go @@ -0,0 +1,250 @@ +package phabricator + +import ( + "encoding/json" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestExtractHeadSHA(t *testing.T) { + testCases := []struct { + name string + diff *diffResult + want string + wantErr string + }{ + { + name: "valid local commits", + diff: &diffResult{ + Properties: properties{ + LocalCommits: map[string]localCommit{ + "abc123def456": {Commit: "abc123def456"}, + }, + }, + }, + want: "abc123def456", + }, + { + name: "empty local commits", + diff: &diffResult{ + Properties: properties{ + LocalCommits: map[string]localCommit{}, + }, + }, + wantErr: "no local commits found", + }, + { + name: "nil local commits", + diff: &diffResult{}, + wantErr: "no local commits found", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + sha, err := extractHeadSHA(tc.diff) + if tc.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, sha) + }) + } +} + +func TestBuildQueryDiffsRequest(t *testing.T) { + testCases := []struct { + name string + diffIDs []int + apiToken string + wantAll []string + }{ + { + name: "single ID", + diffIDs: []int{42}, + wantAll: []string{"ids%5B%5D=42"}, + }, + { + name: "multiple IDs", + diffIDs: []int{42, 43, 44}, + wantAll: []string{"ids%5B%5D=42", "ids%5B%5D=43", "ids%5B%5D=44"}, + }, + { + name: "includes api.token when set", + diffIDs: []int{42}, + apiToken: "my-secret", + wantAll: []string{"ids%5B%5D=42", "api.token=my-secret"}, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + form := buildQueryDiffsRequest(tc.diffIDs, tc.apiToken) + encoded := form.Encode() + for _, want := range tc.wantAll { + assert.Contains(t, encoded, want) + } + }) + } +} + +func TestDoConduitRequest(t *testing.T) { + var capturedPath string + var capturedQuery string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedPath = r.URL.Path + capturedQuery = r.URL.RawQuery + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"result":{}}`)) + })) + defer server.Close() + + client := &http.Client{Transport: &testTransport{baseURL: server.URL}} + form := buildQueryDiffsRequest([]int{100, 200}, "") + + resp, err := doConduitRequest(t.Context(), client, form) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, "/api/differential.querydiffs", capturedPath) + assert.Contains(t, capturedQuery, "ids%5B%5D=100") + assert.Contains(t, capturedQuery, "ids%5B%5D=200") +} + +func TestDoConduitRequest_ConnectionError(t *testing.T) { + client := &http.Client{Transport: &testTransport{baseURL: "http://127.0.0.1:0"}} + form := buildQueryDiffsRequest([]int{1}, "") + + _, err := doConduitRequest(t.Context(), client, form) + + require.Error(t, err) + assert.Contains(t, err.Error(), "HTTP request failed") +} + +func TestParseConduitResponse(t *testing.T) { + validResult := map[string]*diffResult{ + "100": {AuthorName: "Alice", Changes: []fileChange{{CurrentPath: "a.go"}}}, + "101": {AuthorName: "Bob", Changes: []fileChange{{CurrentPath: "b.go"}}}, + } + + testCases := []struct { + name string + resp *http.Response + diffIDs []int + wantErr string + }{ + { + name: "success", + resp: newConduitHTTPResponse(t, validResult, http.StatusOK), + diffIDs: []int{100, 101}, + }, + { + name: "HTTP error", + resp: newPlainHTTPResponse("server error", http.StatusInternalServerError), + diffIDs: []int{100}, + wantErr: "Conduit API returned status 500", + }, + { + name: "conduit error", + resp: newConduitErrorHTTPResponse(t, "ERR-CONDUIT-CORE", "Invalid diff ID"), + diffIDs: []int{100}, + wantErr: "Conduit error", + }, + { + name: "diff not found in response", + resp: newConduitHTTPResponse(t, map[string]*diffResult{"999": {}}, http.StatusOK), + diffIDs: []int{100}, + wantErr: "diff 100 not found", + }, + { + name: "malformed JSON", + resp: newPlainHTTPResponse("{not json", http.StatusOK), + diffIDs: []int{100}, + wantErr: "failed to decode Conduit response", + }, + { + name: "malformed result payload", + resp: newPlainHTTPResponse(`{"result":"not a map"}`, http.StatusOK), + diffIDs: []int{100}, + wantErr: "failed to decode querydiffs result", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + defer tc.resp.Body.Close() + results, err := parseConduitResponse(tc.resp, tc.diffIDs) + + if tc.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + return + } + require.NoError(t, err) + assert.Len(t, results, len(tc.diffIDs)) + }) + } +} + +// newConduitHTTPResponse builds an http.Response with a successful Conduit envelope. +func newConduitHTTPResponse(t *testing.T, result any, statusCode int) *http.Response { + t.Helper() + resultBytes, err := json.Marshal(result) + require.NoError(t, err) + envelope := conduitResponse{Result: resultBytes} + body, err := json.Marshal(envelope) + require.NoError(t, err) + return &http.Response{ + StatusCode: statusCode, + Body: io.NopCloser(strings.NewReader(string(body))), + } +} + +// newConduitErrorHTTPResponse builds an http.Response with a Conduit error envelope. +func newConduitErrorHTTPResponse(t *testing.T, code, info string) *http.Response { + t.Helper() + envelope := conduitResponse{ErrorCode: &code, ErrorInfo: &info} + body, err := json.Marshal(envelope) + require.NoError(t, err) + return &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader(string(body))), + } +} + +// newPlainHTTPResponse builds an http.Response with a plain string body. +func newPlainHTTPResponse(body string, statusCode int) *http.Response { + return &http.Response{ + StatusCode: statusCode, + Body: io.NopCloser(strings.NewReader(body)), + } +} + +// serveConduit writes a successful Conduit response envelope to an HTTP handler. +func serveConduit(t *testing.T, w http.ResponseWriter, result any) { + t.Helper() + resultBytes, err := json.Marshal(result) + require.NoError(t, err) + resp := conduitResponse{Result: resultBytes} + w.Header().Set("Content-Type", "application/json") + require.NoError(t, json.NewEncoder(w).Encode(resp)) +} + +// testTransport rewrites request URLs to point at the test server. +type testTransport struct { + baseURL string +} + +func (t *testTransport) RoundTrip(req *http.Request) (*http.Response, error) { + req.URL.Scheme = "http" + req.URL.Host = t.baseURL[len("http://"):] + return http.DefaultTransport.RoundTrip(req) +} diff --git a/submitqueue/extension/changeprovider/phabricator/convert.go b/submitqueue/extension/changeprovider/phabricator/convert.go new file mode 100644 index 00000000..07478fb2 --- /dev/null +++ b/submitqueue/extension/changeprovider/phabricator/convert.go @@ -0,0 +1,41 @@ +package phabricator + +import ( + "strconv" + + "github.com/uber/submitqueue/submitqueue/entity" + entityphab "github.com/uber/submitqueue/submitqueue/entity/phabricator" +) + +// convertToChangeInfo converts a Phabricator diff result to an entity.ChangeInfo. +func convertToChangeInfo(parsed entityphab.ChangeID, diff *diffResult) entity.ChangeInfo { + return entity.ChangeInfo{ + URI: parsed.String(), + Details: entity.ChangeDetails{ + Author: entity.Author{ + Name: diff.AuthorName, + Email: diff.AuthorEmail, + }, + ChangedFiles: convertFiles(diff.Changes), + }, + } +} + +// convertFiles converts Phabricator file changes to entity.ChangedFile structs. +// Phabricator reports addLines and delLines as strings; parsing failures default +// to zero. Unlike GitHub, Phabricator reports both additions and deletions, so +// LinesModified is set to their sum. +func convertFiles(changes []fileChange) []entity.ChangedFile { + files := make([]entity.ChangedFile, 0, len(changes)) + for _, c := range changes { + added, _ := strconv.Atoi(c.AddLines) + deleted, _ := strconv.Atoi(c.DelLines) + files = append(files, entity.ChangedFile{ + Path: c.CurrentPath, + LinesAdded: added, + LinesDeleted: deleted, + LinesModified: added + deleted, + }) + } + return files +} diff --git a/submitqueue/extension/changeprovider/phabricator/convert_test.go b/submitqueue/extension/changeprovider/phabricator/convert_test.go new file mode 100644 index 00000000..64931e1e --- /dev/null +++ b/submitqueue/extension/changeprovider/phabricator/convert_test.go @@ -0,0 +1,78 @@ +package phabricator + +import ( + "testing" + + "github.com/stretchr/testify/assert" + + "github.com/uber/submitqueue/submitqueue/entity" + entityphab "github.com/uber/submitqueue/submitqueue/entity/phabricator" +) + +func TestConvertToChangeInfo(t *testing.T) { + parsed := entityphab.ChangeID{Scheme: "phab", RevisionID: 12345, DiffID: 100} + diff := &diffResult{ + AuthorName: "Alice", + AuthorEmail: "alice@example.com", + Changes: []fileChange{ + {CurrentPath: "main.go", AddLines: "10", DelLines: "3"}, + {CurrentPath: "test.go", AddLines: "20", DelLines: "0"}, + }, + } + + info := convertToChangeInfo(parsed, diff) + + assert.Equal(t, "phab://D12345/100", info.URI) + assert.Equal(t, entity.Author{Name: "Alice", Email: "alice@example.com"}, info.Details.Author) + assert.Len(t, info.Details.ChangedFiles, 2) +} + +func TestConvertFiles(t *testing.T) { + testCases := []struct { + name string + changes []fileChange + expected []entity.ChangedFile + }{ + { + name: "normal files", + changes: []fileChange{ + {CurrentPath: "a.go", AddLines: "10", DelLines: "5"}, + {CurrentPath: "b.go", AddLines: "0", DelLines: "3"}, + }, + expected: []entity.ChangedFile{ + {Path: "a.go", LinesAdded: 10, LinesDeleted: 5, LinesModified: 15}, + {Path: "b.go", LinesAdded: 0, LinesDeleted: 3, LinesModified: 3}, + }, + }, + { + name: "empty changes", + changes: []fileChange{}, + expected: []entity.ChangedFile{}, + }, + { + name: "unparseable line counts default to zero", + changes: []fileChange{ + {CurrentPath: "c.go", AddLines: "not_a_number", DelLines: ""}, + }, + expected: []entity.ChangedFile{ + {Path: "c.go", LinesAdded: 0, LinesDeleted: 0, LinesModified: 0}, + }, + }, + { + name: "add-only file", + changes: []fileChange{ + {CurrentPath: "new.go", AddLines: "52", DelLines: "0"}, + }, + expected: []entity.ChangedFile{ + {Path: "new.go", LinesAdded: 52, LinesDeleted: 0, LinesModified: 52}, + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := convertFiles(tc.changes) + assert.Equal(t, tc.expected, result) + }) + } +} diff --git a/submitqueue/extension/changeprovider/phabricator/provider.go b/submitqueue/extension/changeprovider/phabricator/provider.go new file mode 100644 index 00000000..babe6329 --- /dev/null +++ b/submitqueue/extension/changeprovider/phabricator/provider.go @@ -0,0 +1,148 @@ +package phabricator + +import ( + "context" + "fmt" + "net/http" + + "github.com/uber-go/tally" + "go.uber.org/zap" + + coremetrics "github.com/uber/submitqueue/core/metrics" + "github.com/uber/submitqueue/submitqueue/entity" + entityphab "github.com/uber/submitqueue/submitqueue/entity/phabricator" + "github.com/uber/submitqueue/submitqueue/extension/changeprovider" +) + +// queryDiffsBatchSize is the maximum number of diff IDs sent in a single +// differential.querydiffs request. +const queryDiffsBatchSize = 10 + +// Params holds the dependencies for the Phabricator ChangeProvider. +type Params struct { + // HTTPClient is a pre-configured HTTP client. The caller is responsible for + // configuring the base URL (e.g. via httpclient.NewClient) and authentication + // (e.g. via oauth2.Transport for Bearer tokens) via transport layers. + HTTPClient *http.Client + // APIToken is the Conduit API token, sent as the api.token form parameter. + APIToken string + // Logger is the structured logger. + Logger *zap.SugaredLogger + // MetricsScope is the metrics scope for instrumentation. + MetricsScope tally.Scope +} + +type provider struct { + httpClient *http.Client + apiToken string + logger *zap.SugaredLogger + metricsScope tally.Scope +} + +// NewProvider creates a new Phabricator ChangeProvider. +func NewProvider(params Params) changeprovider.ChangeProvider { + return &provider{ + httpClient: params.HTTPClient, + apiToken: params.APIToken, + logger: params.Logger.Named("phabricator_changeprovider"), + metricsScope: params.MetricsScope.SubScope("phabricator_changeprovider"), + } +} + +// Get retrieves change information from Phabricator for the request's change. +// Returns one ChangeInfo per URI. +func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity.ChangeInfo, retErr error) { + op := coremetrics.Begin(p.metricsScope, "get") + defer func() { op.Complete(retErr) }() + + change := request.Change + + changeIDs := make([]entityphab.ChangeID, 0, len(change.URIs)) + for _, uri := range change.URIs { + parsed, err := entityphab.ParseChangeID(uri) + if err != nil { + return nil, fmt.Errorf("failed to parse Phabricator change ID %q: %w", uri, err) + } + changeIDs = append(changeIDs, parsed) + } + + p.logger.Debugw("fetching diff data from Phabricator", + "diff_count", len(changeIDs), + "uris", change.URIs, + ) + + if err := validateChangeConsistency(changeIDs); err != nil { + return nil, err + } + + diffs, err := p.fetchAllDiffs(ctx, changeIDs) + if err != nil { + return nil, err + } + + changeInfos := make([]entity.ChangeInfo, 0, len(changeIDs)) + for _, cid := range changeIDs { + diff := diffs[cid.DiffID] + + if err := validateDiffResponse(cid.DiffID, diff); err != nil { + return nil, err + } + + changeInfo := convertToChangeInfo(cid, diff) + changeInfos = append(changeInfos, changeInfo) + + headSHA, _ := extractHeadSHA(diff) + p.logger.Debugw("fetched diff data", + "revision", cid.Revision(), + "diff_id", cid.DiffID, + "files_count", len(changeInfo.Details.ChangedFiles), + "head_sha", headSHA, + ) + } + + p.logger.Debugw("successfully fetched diff data", + "diff_count", len(changeIDs), + ) + + return changeInfos, nil +} + +// fetchAllDiffs fetches diff data for all change IDs in batches of queryDiffsBatchSize. +func (p *provider) fetchAllDiffs(ctx context.Context, changeIDs []entityphab.ChangeID) (map[int]*diffResult, error) { + diffIDs := make([]int, 0, len(changeIDs)) + for _, cid := range changeIDs { + diffIDs = append(diffIDs, cid.DiffID) + } + + results := make(map[int]*diffResult, len(diffIDs)) + + for start := 0; start < len(diffIDs); start += queryDiffsBatchSize { + end := start + queryDiffsBatchSize + if end > len(diffIDs) { + end = len(diffIDs) + } + + batch, err := p.fetchDiffBatch(ctx, diffIDs[start:end]) + if err != nil { + return nil, fmt.Errorf("failed to fetch diffs: %w", err) + } + for id, diff := range batch { + results[id] = diff + } + } + + return results, nil +} + +// fetchDiffBatch fetches a single batch of diffs from Phabricator. +func (p *provider) fetchDiffBatch(ctx context.Context, diffIDs []int) (map[int]*diffResult, error) { + form := buildQueryDiffsRequest(diffIDs, p.apiToken) + + resp, err := doConduitRequest(ctx, p.httpClient, form) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + return parseConduitResponse(resp, diffIDs) +} diff --git a/submitqueue/extension/changeprovider/phabricator/provider_test.go b/submitqueue/extension/changeprovider/phabricator/provider_test.go new file mode 100644 index 00000000..1471f069 --- /dev/null +++ b/submitqueue/extension/changeprovider/phabricator/provider_test.go @@ -0,0 +1,307 @@ +package phabricator + +import ( + "context" + "encoding/json" + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/uber-go/tally" + "go.uber.org/zap/zaptest" + + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/changeprovider" +) + +const testHeadSHA = "7c0ab82c9f074a8d60292fcd121dde3738e3fb7e" + +func newTestProvider(t *testing.T, client *http.Client) changeprovider.ChangeProvider { + t.Helper() + return NewProvider(Params{ + HTTPClient: client, + Logger: zaptest.NewLogger(t).Sugar(), + MetricsScope: tally.NoopScope, + }) +} + +func validDiffResponse() map[string]*diffResult { + return map[string]*diffResult{ + "100": { + AuthorName: "Test Author", + AuthorEmail: "test@example.com", + Changes: []fileChange{ + {CurrentPath: "main.go", AddLines: "10", DelLines: "3"}, + {CurrentPath: "test.go", AddLines: "20", DelLines: "0"}, + }, + Properties: properties{ + LocalCommits: map[string]localCommit{ + testHeadSHA: {Commit: testHeadSHA}, + }, + }, + }, + } +} + +func TestProvider_Get(t *testing.T) { + testCases := []struct { + name string + handler http.HandlerFunc + uris []string + wantErr string + }{ + { + name: "returns result for valid diff", + handler: func(w http.ResponseWriter, r *http.Request) { + serveConduit(t, w, validDiffResponse()) + }, + uris: []string{"phab://D200/100"}, + }, + { + name: "invalid URI returns error", + uris: []string{"invalid://uri"}, + wantErr: "failed to parse Phabricator change ID", + }, + { + name: "HTTP error returns error", + handler: func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + }, + uris: []string{"phab://D200/100"}, + wantErr: "Conduit API returned status 500", + }, + { + name: "diff with no changes returns error", + handler: func(w http.ResponseWriter, r *http.Request) { + serveConduit(t, w, map[string]*diffResult{ + "100": { + Changes: []fileChange{}, + Properties: properties{ + LocalCommits: map[string]localCommit{testHeadSHA: {Commit: testHeadSHA}}, + }, + }, + }) + }, + uris: []string{"phab://D200/100"}, + wantErr: "diff 100 has no file changes", + }, + { + name: "diff with no local commits returns error", + handler: func(w http.ResponseWriter, r *http.Request) { + serveConduit(t, w, map[string]*diffResult{ + "100": { + Changes: []fileChange{{CurrentPath: "main.go", AddLines: "1", DelLines: "0"}}, + }, + }) + }, + uris: []string{"phab://D200/100"}, + wantErr: "no local commits found", + }, + { + name: "duplicate revision returns error", + uris: []string{"phab://D200/100", "phab://D200/101"}, + wantErr: "duplicate revision D200", + }, + { + name: "duplicate diff returns error", + uris: []string{"phab://D200/100", "phab://D201/100"}, + wantErr: "duplicate diff 100", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + var client *http.Client + if tc.handler != nil { + server := httptest.NewServer(tc.handler) + defer server.Close() + client = &http.Client{Transport: &testTransport{baseURL: server.URL}} + } else { + client = &http.Client{Transport: &testTransport{baseURL: "http://localhost"}} + } + + p := newTestProvider(t, client) + infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{URIs: tc.uris}}) + + if tc.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + return + } + require.NoError(t, err) + require.Len(t, infos, 1) + assert.Equal(t, "phab://D200/100", infos[0].URI) + assert.Equal(t, "Test Author", infos[0].Details.Author.Name) + assert.Len(t, infos[0].Details.ChangedFiles, 2) + }) + } +} + +func TestProvider_Get_MultipleDiffs(t *testing.T) { + callCount := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + serveConduit(t, w, map[string]*diffResult{ + "100": { + AuthorName: "Author A", + AuthorEmail: "a@example.com", + Changes: []fileChange{{CurrentPath: "file1.go", AddLines: "5", DelLines: "1"}}, + Properties: properties{LocalCommits: map[string]localCommit{"sha1": {Commit: "sha1"}}}, + }, + "101": { + AuthorName: "Author B", + AuthorEmail: "b@example.com", + Changes: []fileChange{{CurrentPath: "file2.go", AddLines: "8", DelLines: "2"}}, + Properties: properties{LocalCommits: map[string]localCommit{"sha2": {Commit: "sha2"}}}, + }, + }) + })) + defer server.Close() + + client := &http.Client{Transport: &testTransport{baseURL: server.URL}} + p := newTestProvider(t, client) + infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{ + URIs: []string{"phab://D200/100", "phab://D201/101"}, + }}) + + require.NoError(t, err) + assert.Equal(t, 1, callCount) + require.Len(t, infos, 2) + assert.Equal(t, "phab://D200/100", infos[0].URI) + assert.Equal(t, "phab://D201/101", infos[1].URI) +} + +func TestProvider_Get_ConnectionError(t *testing.T) { + client := &http.Client{Transport: &testTransport{baseURL: "http://127.0.0.1:0"}} + p := newTestProvider(t, client) + _, err := p.Get(context.Background(), entity.Request{Change: entity.Change{ + URIs: []string{"phab://D200/100"}, + }}) + + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to fetch diffs") +} + +func TestProvider_Get_FileDetails(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + result := map[string]*diffResult{ + "100": { + AuthorName: "Author", + AuthorEmail: "author@example.com", + Changes: []fileChange{ + {CurrentPath: "added.go", AddLines: "52", DelLines: "0"}, + {CurrentPath: "modified.go", AddLines: "10", DelLines: "5"}, + {CurrentPath: "deleted.go", AddLines: "0", DelLines: "30"}, + }, + Properties: properties{ + LocalCommits: map[string]localCommit{testHeadSHA: {Commit: testHeadSHA}}, + }, + }, + } + resultBytes, _ := json.Marshal(result) + resp := conduitResponse{Result: resultBytes} + w.Header().Set("Content-Type", "application/json") + _ = json.NewEncoder(w).Encode(resp) + })) + defer server.Close() + + client := &http.Client{Transport: &testTransport{baseURL: server.URL}} + p := newTestProvider(t, client) + infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{ + URIs: []string{"phab://D200/100"}, + }}) + + require.NoError(t, err) + require.Len(t, infos, 1) + + files := infos[0].Details.ChangedFiles + require.Len(t, files, 3) + + assert.Equal(t, "added.go", files[0].Path) + assert.Equal(t, 52, files[0].LinesAdded) + assert.Equal(t, 0, files[0].LinesDeleted) + assert.Equal(t, 52, files[0].LinesModified) + + assert.Equal(t, "modified.go", files[1].Path) + assert.Equal(t, 10, files[1].LinesAdded) + assert.Equal(t, 5, files[1].LinesDeleted) + assert.Equal(t, 15, files[1].LinesModified) + + assert.Equal(t, "deleted.go", files[2].Path) + assert.Equal(t, 0, files[2].LinesAdded) + assert.Equal(t, 30, files[2].LinesDeleted) + assert.Equal(t, 30, files[2].LinesModified) +} + +func TestProvider_Get_Batching(t *testing.T) { + callCount := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + query := r.URL.Query() + ids := query["ids[]"] + result := make(map[string]*diffResult, len(ids)) + for _, id := range ids { + result[id] = &diffResult{ + AuthorName: "Author " + id, + Changes: []fileChange{{CurrentPath: id + ".go", AddLines: "1", DelLines: "0"}}, + Properties: properties{LocalCommits: map[string]localCommit{"sha": {Commit: "sha"}}}, + } + } + serveConduit(t, w, result) + })) + defer server.Close() + + uris := make([]string, 25) + for i := range uris { + diffID := i + 1 + revisionID := 1000 + i + uris[i] = fmt.Sprintf("phab://D%d/%d", revisionID, diffID) + } + + client := &http.Client{Transport: &testTransport{baseURL: server.URL}} + p := newTestProvider(t, client) + infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{URIs: uris}}) + + require.NoError(t, err) + assert.Equal(t, 3, callCount) + assert.Len(t, infos, 25) +} + +func TestProvider_Get_BatchingStopsOnError(t *testing.T) { + callCount := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + callCount++ + if callCount == 2 { + w.WriteHeader(http.StatusInternalServerError) + return + } + query := r.URL.Query() + ids := query["ids[]"] + result := make(map[string]*diffResult, len(ids)) + for _, id := range ids { + result[id] = &diffResult{ + Changes: []fileChange{{CurrentPath: id + ".go", AddLines: "1", DelLines: "0"}}, + Properties: properties{LocalCommits: map[string]localCommit{"sha": {Commit: "sha"}}}, + } + } + serveConduit(t, w, result) + })) + defer server.Close() + + uris := make([]string, 25) + for i := range uris { + diffID := i + 1 + revisionID := 1000 + i + uris[i] = fmt.Sprintf("phab://D%d/%d", revisionID, diffID) + } + + client := &http.Client{Transport: &testTransport{baseURL: server.URL}} + p := newTestProvider(t, client) + _, err := p.Get(context.Background(), entity.Request{Change: entity.Change{URIs: uris}}) + + require.Error(t, err) + assert.Equal(t, 2, callCount) +} diff --git a/submitqueue/extension/changeprovider/phabricator/validate.go b/submitqueue/extension/changeprovider/phabricator/validate.go new file mode 100644 index 00000000..d270fb11 --- /dev/null +++ b/submitqueue/extension/changeprovider/phabricator/validate.go @@ -0,0 +1,47 @@ +package phabricator + +import ( + "fmt" + + entityphab "github.com/uber/submitqueue/submitqueue/entity/phabricator" +) + +// validateChangeConsistency validates that all changeIDs in the stack are consistent. +// Stacked changes must have the same scheme, and each must have a unique revision and diff ID. +func validateChangeConsistency(changeIDs []entityphab.ChangeID) error { + expectedScheme := changeIDs[0].Scheme + + seenRevisions := make(map[int]bool, len(changeIDs)) + seenDiffs := make(map[int]bool, len(changeIDs)) + + for _, cid := range changeIDs { + if cid.Scheme != expectedScheme { + return fmt.Errorf("stacked changes must use same change provider: expected %s, got %s for %s", + expectedScheme, cid.Scheme, cid.Revision()) + } + + if seenRevisions[cid.RevisionID] { + return fmt.Errorf("duplicate revision %s in stacked changes", cid.Revision()) + } + seenRevisions[cid.RevisionID] = true + + if seenDiffs[cid.DiffID] { + return fmt.Errorf("duplicate diff %d in stacked changes", cid.DiffID) + } + seenDiffs[cid.DiffID] = true + } + + return nil +} + +// validateDiffResponse checks that the querydiffs result is well-formed: that +// it contains at least one file change and a parseable head SHA. +func validateDiffResponse(diffID int, diff *diffResult) error { + if len(diff.Changes) == 0 { + return fmt.Errorf("diff %d has no file changes", diffID) + } + if _, err := extractHeadSHA(diff); err != nil { + return fmt.Errorf("diff %d: %w", diffID, err) + } + return nil +} diff --git a/submitqueue/extension/changeprovider/phabricator/validate_test.go b/submitqueue/extension/changeprovider/phabricator/validate_test.go new file mode 100644 index 00000000..7ee64785 --- /dev/null +++ b/submitqueue/extension/changeprovider/phabricator/validate_test.go @@ -0,0 +1,125 @@ +package phabricator + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + entityphab "github.com/uber/submitqueue/submitqueue/entity/phabricator" +) + +func TestValidateChangeConsistency(t *testing.T) { + testCases := []struct { + name string + changeIDs []entityphab.ChangeID + wantErr string + }{ + { + name: "single change", + changeIDs: []entityphab.ChangeID{ + {Scheme: "phab", RevisionID: 100, DiffID: 1}, + }, + }, + { + name: "consistent stack", + changeIDs: []entityphab.ChangeID{ + {Scheme: "phab", RevisionID: 100, DiffID: 1}, + {Scheme: "phab", RevisionID: 101, DiffID: 2}, + {Scheme: "phab", RevisionID: 102, DiffID: 3}, + }, + }, + { + name: "different scheme", + changeIDs: []entityphab.ChangeID{ + {Scheme: "phab", RevisionID: 100, DiffID: 1}, + {Scheme: "other", RevisionID: 101, DiffID: 2}, + }, + wantErr: "same change provider", + }, + { + name: "duplicate revision", + changeIDs: []entityphab.ChangeID{ + {Scheme: "phab", RevisionID: 100, DiffID: 1}, + {Scheme: "phab", RevisionID: 100, DiffID: 2}, + }, + wantErr: "duplicate revision D100", + }, + { + name: "duplicate diff", + changeIDs: []entityphab.ChangeID{ + {Scheme: "phab", RevisionID: 100, DiffID: 1}, + {Scheme: "phab", RevisionID: 101, DiffID: 1}, + }, + wantErr: "duplicate diff 1", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := validateChangeConsistency(tc.changeIDs) + if tc.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + return + } + require.NoError(t, err) + }) + } +} + +func TestValidateDiffResponse(t *testing.T) { + testCases := []struct { + name string + diffID int + diff *diffResult + wantErr string + }{ + { + name: "valid response", + diffID: 100, + diff: &diffResult{ + Changes: []fileChange{{CurrentPath: "main.go"}}, + Properties: properties{ + LocalCommits: map[string]localCommit{ + "abc123": {Commit: "abc123"}, + }, + }, + }, + }, + { + name: "no file changes", + diffID: 100, + diff: &diffResult{ + Changes: []fileChange{}, + Properties: properties{ + LocalCommits: map[string]localCommit{ + "abc123": {Commit: "abc123"}, + }, + }, + }, + wantErr: "diff 100 has no file changes", + }, + { + name: "no local commits", + diffID: 100, + diff: &diffResult{ + Changes: []fileChange{{CurrentPath: "main.go"}}, + Properties: properties{}, + }, + wantErr: "no local commits found", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := validateDiffResponse(tc.diffID, tc.diff) + if tc.wantErr != "" { + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErr) + return + } + require.NoError(t, err) + }) + } +} From 4ab5382f45bb0129d52a0ed1541758630af167bd Mon Sep 17 00:00:00 2001 From: Adam Bettigole Date: Fri, 12 Jun 2026 13:37:35 -0700 Subject: [PATCH 2/3] removed unnecessary head SHA --- .../changeprovider/phabricator/conduit.go | 20 -------- .../phabricator/conduit_test.go | 48 ------------------- .../changeprovider/phabricator/provider.go | 2 - .../phabricator/provider_test.go | 31 +----------- .../changeprovider/phabricator/validate.go | 6 +-- .../phabricator/validate_test.go | 19 -------- 6 files changed, 2 insertions(+), 124 deletions(-) diff --git a/submitqueue/extension/changeprovider/phabricator/conduit.go b/submitqueue/extension/changeprovider/phabricator/conduit.go index b1911da9..b2c8f0b6 100644 --- a/submitqueue/extension/changeprovider/phabricator/conduit.go +++ b/submitqueue/extension/changeprovider/phabricator/conduit.go @@ -21,7 +21,6 @@ type conduitResponse struct { // diffResult represents a single diff from the differential.querydiffs response. type diffResult struct { Changes []fileChange `json:"changes"` - Properties properties `json:"properties"` AuthorName string `json:"authorName"` AuthorEmail string `json:"authorEmail"` } @@ -33,25 +32,6 @@ type fileChange struct { DelLines string `json:"delLines"` } -// properties holds diff metadata from the querydiffs response. -type properties struct { - LocalCommits map[string]localCommit `json:"local:commits"` -} - -// localCommit represents a commit entry from the local:commits property. -type localCommit struct { - Commit string `json:"commit"` -} - -// extractHeadSHA returns the head commit SHA from the diff's local:commits -// property. The first key of the map is the commit SHA. -func extractHeadSHA(diff *diffResult) (string, error) { - for sha := range diff.Properties.LocalCommits { - return sha, nil - } - return "", fmt.Errorf("no local commits found in diff properties") -} - // buildQueryDiffsRequest builds query parameters for a differential.querydiffs call. func buildQueryDiffsRequest(diffIDs []int, apiToken string) url.Values { form := url.Values{} diff --git a/submitqueue/extension/changeprovider/phabricator/conduit_test.go b/submitqueue/extension/changeprovider/phabricator/conduit_test.go index 5e5a68c6..96e00536 100644 --- a/submitqueue/extension/changeprovider/phabricator/conduit_test.go +++ b/submitqueue/extension/changeprovider/phabricator/conduit_test.go @@ -12,54 +12,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestExtractHeadSHA(t *testing.T) { - testCases := []struct { - name string - diff *diffResult - want string - wantErr string - }{ - { - name: "valid local commits", - diff: &diffResult{ - Properties: properties{ - LocalCommits: map[string]localCommit{ - "abc123def456": {Commit: "abc123def456"}, - }, - }, - }, - want: "abc123def456", - }, - { - name: "empty local commits", - diff: &diffResult{ - Properties: properties{ - LocalCommits: map[string]localCommit{}, - }, - }, - wantErr: "no local commits found", - }, - { - name: "nil local commits", - diff: &diffResult{}, - wantErr: "no local commits found", - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - sha, err := extractHeadSHA(tc.diff) - if tc.wantErr != "" { - require.Error(t, err) - assert.Contains(t, err.Error(), tc.wantErr) - return - } - require.NoError(t, err) - assert.Equal(t, tc.want, sha) - }) - } -} - func TestBuildQueryDiffsRequest(t *testing.T) { testCases := []struct { name string diff --git a/submitqueue/extension/changeprovider/phabricator/provider.go b/submitqueue/extension/changeprovider/phabricator/provider.go index babe6329..777bc9f4 100644 --- a/submitqueue/extension/changeprovider/phabricator/provider.go +++ b/submitqueue/extension/changeprovider/phabricator/provider.go @@ -91,12 +91,10 @@ func (p *provider) Get(ctx context.Context, request entity.Request) (_ []entity. changeInfo := convertToChangeInfo(cid, diff) changeInfos = append(changeInfos, changeInfo) - headSHA, _ := extractHeadSHA(diff) p.logger.Debugw("fetched diff data", "revision", cid.Revision(), "diff_id", cid.DiffID, "files_count", len(changeInfo.Details.ChangedFiles), - "head_sha", headSHA, ) } diff --git a/submitqueue/extension/changeprovider/phabricator/provider_test.go b/submitqueue/extension/changeprovider/phabricator/provider_test.go index 1471f069..e403f44a 100644 --- a/submitqueue/extension/changeprovider/phabricator/provider_test.go +++ b/submitqueue/extension/changeprovider/phabricator/provider_test.go @@ -17,8 +17,6 @@ import ( "github.com/uber/submitqueue/submitqueue/extension/changeprovider" ) -const testHeadSHA = "7c0ab82c9f074a8d60292fcd121dde3738e3fb7e" - func newTestProvider(t *testing.T, client *http.Client) changeprovider.ChangeProvider { t.Helper() return NewProvider(Params{ @@ -37,11 +35,6 @@ func validDiffResponse() map[string]*diffResult { {CurrentPath: "main.go", AddLines: "10", DelLines: "3"}, {CurrentPath: "test.go", AddLines: "20", DelLines: "0"}, }, - Properties: properties{ - LocalCommits: map[string]localCommit{ - testHeadSHA: {Commit: testHeadSHA}, - }, - }, }, } } @@ -79,27 +72,12 @@ func TestProvider_Get(t *testing.T) { serveConduit(t, w, map[string]*diffResult{ "100": { Changes: []fileChange{}, - Properties: properties{ - LocalCommits: map[string]localCommit{testHeadSHA: {Commit: testHeadSHA}}, - }, }, }) }, uris: []string{"phab://D200/100"}, wantErr: "diff 100 has no file changes", }, - { - name: "diff with no local commits returns error", - handler: func(w http.ResponseWriter, r *http.Request) { - serveConduit(t, w, map[string]*diffResult{ - "100": { - Changes: []fileChange{{CurrentPath: "main.go", AddLines: "1", DelLines: "0"}}, - }, - }) - }, - uris: []string{"phab://D200/100"}, - wantErr: "no local commits found", - }, { name: "duplicate revision returns error", uris: []string{"phab://D200/100", "phab://D200/101"}, @@ -149,13 +127,11 @@ func TestProvider_Get_MultipleDiffs(t *testing.T) { AuthorName: "Author A", AuthorEmail: "a@example.com", Changes: []fileChange{{CurrentPath: "file1.go", AddLines: "5", DelLines: "1"}}, - Properties: properties{LocalCommits: map[string]localCommit{"sha1": {Commit: "sha1"}}}, }, "101": { AuthorName: "Author B", AuthorEmail: "b@example.com", Changes: []fileChange{{CurrentPath: "file2.go", AddLines: "8", DelLines: "2"}}, - Properties: properties{LocalCommits: map[string]localCommit{"sha2": {Commit: "sha2"}}}, }, }) })) @@ -196,9 +172,6 @@ func TestProvider_Get_FileDetails(t *testing.T) { {CurrentPath: "modified.go", AddLines: "10", DelLines: "5"}, {CurrentPath: "deleted.go", AddLines: "0", DelLines: "30"}, }, - Properties: properties{ - LocalCommits: map[string]localCommit{testHeadSHA: {Commit: testHeadSHA}}, - }, }, } resultBytes, _ := json.Marshal(result) @@ -247,7 +220,6 @@ func TestProvider_Get_Batching(t *testing.T) { result[id] = &diffResult{ AuthorName: "Author " + id, Changes: []fileChange{{CurrentPath: id + ".go", AddLines: "1", DelLines: "0"}}, - Properties: properties{LocalCommits: map[string]localCommit{"sha": {Commit: "sha"}}}, } } serveConduit(t, w, result) @@ -283,8 +255,7 @@ func TestProvider_Get_BatchingStopsOnError(t *testing.T) { result := make(map[string]*diffResult, len(ids)) for _, id := range ids { result[id] = &diffResult{ - Changes: []fileChange{{CurrentPath: id + ".go", AddLines: "1", DelLines: "0"}}, - Properties: properties{LocalCommits: map[string]localCommit{"sha": {Commit: "sha"}}}, + Changes: []fileChange{{CurrentPath: id + ".go", AddLines: "1", DelLines: "0"}}, } } serveConduit(t, w, result) diff --git a/submitqueue/extension/changeprovider/phabricator/validate.go b/submitqueue/extension/changeprovider/phabricator/validate.go index d270fb11..8b809059 100644 --- a/submitqueue/extension/changeprovider/phabricator/validate.go +++ b/submitqueue/extension/changeprovider/phabricator/validate.go @@ -34,14 +34,10 @@ func validateChangeConsistency(changeIDs []entityphab.ChangeID) error { return nil } -// validateDiffResponse checks that the querydiffs result is well-formed: that -// it contains at least one file change and a parseable head SHA. +// validateDiffResponse checks that the querydiffs result is well-formed: that it contains at least one changed file. func validateDiffResponse(diffID int, diff *diffResult) error { if len(diff.Changes) == 0 { return fmt.Errorf("diff %d has no file changes", diffID) } - if _, err := extractHeadSHA(diff); err != nil { - return fmt.Errorf("diff %d: %w", diffID, err) - } return nil } diff --git a/submitqueue/extension/changeprovider/phabricator/validate_test.go b/submitqueue/extension/changeprovider/phabricator/validate_test.go index 7ee64785..11cb196c 100644 --- a/submitqueue/extension/changeprovider/phabricator/validate_test.go +++ b/submitqueue/extension/changeprovider/phabricator/validate_test.go @@ -80,11 +80,6 @@ func TestValidateDiffResponse(t *testing.T) { diffID: 100, diff: &diffResult{ Changes: []fileChange{{CurrentPath: "main.go"}}, - Properties: properties{ - LocalCommits: map[string]localCommit{ - "abc123": {Commit: "abc123"}, - }, - }, }, }, { @@ -92,23 +87,9 @@ func TestValidateDiffResponse(t *testing.T) { diffID: 100, diff: &diffResult{ Changes: []fileChange{}, - Properties: properties{ - LocalCommits: map[string]localCommit{ - "abc123": {Commit: "abc123"}, - }, - }, }, wantErr: "diff 100 has no file changes", }, - { - name: "no local commits", - diffID: 100, - diff: &diffResult{ - Changes: []fileChange{{CurrentPath: "main.go"}}, - Properties: properties{}, - }, - wantErr: "no local commits found", - }, } for _, tc := range testCases { From 127cb5606aafec6006865014363fca68ea0ca41c Mon Sep 17 00:00:00 2001 From: Adam Bettigole Date: Fri, 12 Jun 2026 13:56:08 -0700 Subject: [PATCH 3/3] updating for entity refactor --- .../changeprovider/phabricator/BUILD.bazel | 5 +++-- .../extension/changeprovider/phabricator/convert.go | 2 +- .../changeprovider/phabricator/convert_test.go | 2 +- .../changeprovider/phabricator/provider.go | 2 +- .../changeprovider/phabricator/provider_test.go | 13 +++++++------ .../changeprovider/phabricator/validate.go | 2 +- .../changeprovider/phabricator/validate_test.go | 2 +- 7 files changed, 15 insertions(+), 13 deletions(-) diff --git a/submitqueue/extension/changeprovider/phabricator/BUILD.bazel b/submitqueue/extension/changeprovider/phabricator/BUILD.bazel index 8a7c3818..9a65c6e8 100644 --- a/submitqueue/extension/changeprovider/phabricator/BUILD.bazel +++ b/submitqueue/extension/changeprovider/phabricator/BUILD.bazel @@ -12,8 +12,8 @@ go_library( visibility = ["//visibility:public"], deps = [ "//core/metrics", + "//entity/change/phabricator", "//submitqueue/entity", - "//submitqueue/entity/phabricator", "//submitqueue/extension/changeprovider", "@com_github_uber_go_tally//:tally", "@org_uber_go_zap//:zap", @@ -30,8 +30,9 @@ go_test( ], embed = [":phabricator"], deps = [ + "//entity/change", + "//entity/change/phabricator", "//submitqueue/entity", - "//submitqueue/entity/phabricator", "//submitqueue/extension/changeprovider", "@com_github_stretchr_testify//assert", "@com_github_stretchr_testify//require", diff --git a/submitqueue/extension/changeprovider/phabricator/convert.go b/submitqueue/extension/changeprovider/phabricator/convert.go index 07478fb2..9a21511c 100644 --- a/submitqueue/extension/changeprovider/phabricator/convert.go +++ b/submitqueue/extension/changeprovider/phabricator/convert.go @@ -3,8 +3,8 @@ package phabricator import ( "strconv" + entityphab "github.com/uber/submitqueue/entity/change/phabricator" "github.com/uber/submitqueue/submitqueue/entity" - entityphab "github.com/uber/submitqueue/submitqueue/entity/phabricator" ) // convertToChangeInfo converts a Phabricator diff result to an entity.ChangeInfo. diff --git a/submitqueue/extension/changeprovider/phabricator/convert_test.go b/submitqueue/extension/changeprovider/phabricator/convert_test.go index 64931e1e..45a711a5 100644 --- a/submitqueue/extension/changeprovider/phabricator/convert_test.go +++ b/submitqueue/extension/changeprovider/phabricator/convert_test.go @@ -5,8 +5,8 @@ import ( "github.com/stretchr/testify/assert" + entityphab "github.com/uber/submitqueue/entity/change/phabricator" "github.com/uber/submitqueue/submitqueue/entity" - entityphab "github.com/uber/submitqueue/submitqueue/entity/phabricator" ) func TestConvertToChangeInfo(t *testing.T) { diff --git a/submitqueue/extension/changeprovider/phabricator/provider.go b/submitqueue/extension/changeprovider/phabricator/provider.go index 777bc9f4..bd4ce8d9 100644 --- a/submitqueue/extension/changeprovider/phabricator/provider.go +++ b/submitqueue/extension/changeprovider/phabricator/provider.go @@ -9,8 +9,8 @@ import ( "go.uber.org/zap" coremetrics "github.com/uber/submitqueue/core/metrics" + entityphab "github.com/uber/submitqueue/entity/change/phabricator" "github.com/uber/submitqueue/submitqueue/entity" - entityphab "github.com/uber/submitqueue/submitqueue/entity/phabricator" "github.com/uber/submitqueue/submitqueue/extension/changeprovider" ) diff --git a/submitqueue/extension/changeprovider/phabricator/provider_test.go b/submitqueue/extension/changeprovider/phabricator/provider_test.go index e403f44a..452bec36 100644 --- a/submitqueue/extension/changeprovider/phabricator/provider_test.go +++ b/submitqueue/extension/changeprovider/phabricator/provider_test.go @@ -13,6 +13,7 @@ import ( "github.com/uber-go/tally" "go.uber.org/zap/zaptest" + "github.com/uber/submitqueue/entity/change" "github.com/uber/submitqueue/submitqueue/entity" "github.com/uber/submitqueue/submitqueue/extension/changeprovider" ) @@ -102,7 +103,7 @@ func TestProvider_Get(t *testing.T) { } p := newTestProvider(t, client) - infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{URIs: tc.uris}}) + infos, err := p.Get(context.Background(), entity.Request{Change: change.Change{URIs: tc.uris}}) if tc.wantErr != "" { require.Error(t, err) @@ -139,7 +140,7 @@ func TestProvider_Get_MultipleDiffs(t *testing.T) { client := &http.Client{Transport: &testTransport{baseURL: server.URL}} p := newTestProvider(t, client) - infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{ + infos, err := p.Get(context.Background(), entity.Request{Change: change.Change{ URIs: []string{"phab://D200/100", "phab://D201/101"}, }}) @@ -153,7 +154,7 @@ func TestProvider_Get_MultipleDiffs(t *testing.T) { func TestProvider_Get_ConnectionError(t *testing.T) { client := &http.Client{Transport: &testTransport{baseURL: "http://127.0.0.1:0"}} p := newTestProvider(t, client) - _, err := p.Get(context.Background(), entity.Request{Change: entity.Change{ + _, err := p.Get(context.Background(), entity.Request{Change: change.Change{ URIs: []string{"phab://D200/100"}, }}) @@ -183,7 +184,7 @@ func TestProvider_Get_FileDetails(t *testing.T) { client := &http.Client{Transport: &testTransport{baseURL: server.URL}} p := newTestProvider(t, client) - infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{ + infos, err := p.Get(context.Background(), entity.Request{Change: change.Change{ URIs: []string{"phab://D200/100"}, }}) @@ -235,7 +236,7 @@ func TestProvider_Get_Batching(t *testing.T) { client := &http.Client{Transport: &testTransport{baseURL: server.URL}} p := newTestProvider(t, client) - infos, err := p.Get(context.Background(), entity.Request{Change: entity.Change{URIs: uris}}) + infos, err := p.Get(context.Background(), entity.Request{Change: change.Change{URIs: uris}}) require.NoError(t, err) assert.Equal(t, 3, callCount) @@ -271,7 +272,7 @@ func TestProvider_Get_BatchingStopsOnError(t *testing.T) { client := &http.Client{Transport: &testTransport{baseURL: server.URL}} p := newTestProvider(t, client) - _, err := p.Get(context.Background(), entity.Request{Change: entity.Change{URIs: uris}}) + _, err := p.Get(context.Background(), entity.Request{Change: change.Change{URIs: uris}}) require.Error(t, err) assert.Equal(t, 2, callCount) diff --git a/submitqueue/extension/changeprovider/phabricator/validate.go b/submitqueue/extension/changeprovider/phabricator/validate.go index 8b809059..eb08ad19 100644 --- a/submitqueue/extension/changeprovider/phabricator/validate.go +++ b/submitqueue/extension/changeprovider/phabricator/validate.go @@ -3,7 +3,7 @@ package phabricator import ( "fmt" - entityphab "github.com/uber/submitqueue/submitqueue/entity/phabricator" + entityphab "github.com/uber/submitqueue/entity/change/phabricator" ) // validateChangeConsistency validates that all changeIDs in the stack are consistent. diff --git a/submitqueue/extension/changeprovider/phabricator/validate_test.go b/submitqueue/extension/changeprovider/phabricator/validate_test.go index 11cb196c..58bd0fbc 100644 --- a/submitqueue/extension/changeprovider/phabricator/validate_test.go +++ b/submitqueue/extension/changeprovider/phabricator/validate_test.go @@ -6,7 +6,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - entityphab "github.com/uber/submitqueue/submitqueue/entity/phabricator" + entityphab "github.com/uber/submitqueue/entity/change/phabricator" ) func TestValidateChangeConsistency(t *testing.T) {