Skip to content

Commit 603dcc4

Browse files
committed
feat: per-queue extension fakes with error injection in example
## Summary ### Why? Extensions over external systems (changeprovider, buildrunner, pusher, mergechecker) had no runnable stub to exercise their success — and especially failure — paths without standing up the real backend (GitHub/CI/git). scorer and conflict had deterministic stubs (heuristic, composite, all, none) but no way to inject errors. We needed to drive both happy and error paths end-to-end, including e2e, against a single running stack. We also wanted the example orchestrator to demonstrate SubmitQueue's core premise — per-queue pluggable extensions — instead of wiring one implementation for every queue. ### What? Fakes (test/example stubs, never production): - buildrunner/fake, changeprovider/fake, pusher/fake, mergechecker/fake: best-case by default; inject failures via an `sq-fake=<token>` marker in a change URI (e.g. build-fail, conflict, unmergeable, provider-error), so a single land request can drive the error path end-to-end. - scorer/fake, conflict/fake: decorators that wrap an existing impl (the "pick") and overlay error injection — scorer via the URI marker (score-error) over entity.BatchChanges, analyzer via a predicate, since Analyze sees batches, not change URIs. - Retired buildrunner/noop (subsumed by buildrunner/fake); swapped its unit-test and example callers. Wiring (example orchestrator main.go): - Queue-major registry: queueExtensions (one queue's full impl set) + queueRegistry (queue -> profile, with a default). Six thin Factory adapters resolve impls by Config.QueueName. - Per-queue profiles: test-queue (bucketed heuristic over lines changed), e2e-test-queue (composite scorer, no-conflict), plus a new e2e-conflict-error-queue that always fails conflict analysis. Edge integrations + build runner share a baseline. - Removed all per-impl NewFactory() from extension/*: factory implementations and queue routing now live only in the wiring layer. Conventions: - CLAUDE.md: documented that extensions hold contracts + impls only (no factory implementations or routing — that belongs in the wiring layer); added a TODO to evaluate promoting the registry into submitqueue/core later. Stacking / upstream accommodation (this PR is stacked on the scorer branch): adapted to the scorer branch's refactors — ChangeStore folded into storage (dropped the example's changestore wiring; change.sql is now applied by the existing storage schema step), and the scorer now scores over entity.BatchChanges (scorer/fake and the example value func use it). ## Test Plan - ✅ `make test` — 44/44 unit tests - ✅ `make e2e-test` — 1/1 - ✅ `make integration-test` — 7/7 - ✅ `make check-gazelle` / `make check-tidy` — clean - ✅ Live stack (`make local-submitqueue-start`): Ping OK on gateway and orchestrator; gateway rejects unknown queues; validated end-to-end — scorer passthrough (test-queue progressed past scoring), scorer error (`?sq-fake=score-error` -> "fake: marked score error"), and conflict error (e2e-conflict-error-queue -> "fake: injected analyze error").
1 parent 7f2a754 commit 603dcc4

26 files changed

Lines changed: 1315 additions & 176 deletions

File tree

CLAUDE.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -116,6 +116,8 @@ Vendor-agnostic, pluggable interfaces with implementations in subdirectories:
116116
2. Implementations at `extension/{ext}/{impl}/`
117117
3. Factory interface for dependency injection and lifecycle management
118118
119+
**Extensions hold contracts and impls only — never factory implementations or routing.** An `extension/{ext}` package owns the behavioral interface, its `Config`, the `Factory` *interface*, and impl constructors `New(...)` that return the interface. It must NOT contain `Factory` *implementations* (no `NewFactory()` constructors or factory structs) and no queue-selection/routing logic. The reason: an impl package (e.g. `scorer/heuristic`) cannot know the queue topology or the other impls, so any "which impl for which queue" decision baked there is wrong by construction. A `Factory` whose `For(cfg)` ignores `cfg.QueueName` is just an impl→Factory adapter — that adapting, and all per-queue routing, belongs in the **wiring/integrator layer** (e.g. `example/{domain}/{service}/server/main.go`), which is the one place that knows the queue set. There, adapt `New(...)` impls to the `Factory` contract and route on `Config.QueueName`. Keep this seam strict: if you're about to add a `NewFactory()` or a `map[queue]impl` under `extension/`, it belongs in the wiring layer instead.
120+
119121
**Design interfaces for the technology *space*, not the implementation in front of you.** The interface is a contract every backend will have to satisfy — SQL, key-value (DynamoDB, Bigtable), document, message queue, search, RPC, in-memory, mocks. If the contract assumes a capability that some plausible backend can't provide cheaply, you've baked the current impl's strengths into the API.
120122
121123
Common over-constraints to avoid:

example/submitqueue/gateway/server/queues.yaml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,7 @@ queues:
77
- name: test-queue
88
- name: e2e-test-queue
99
- name: e2e-cancel-queue
10+
# Routes to an analyzer that always errors (conflictfake.FailAlways) so e2e can
11+
# exercise the conflict-analysis error path. See newQueueRegistry in the
12+
# orchestrator example server.
13+
- name: e2e-conflict-error-queue

example/submitqueue/orchestrator/server/BUILD.bazel

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,16 +21,23 @@ go_library(
2121
"//submitqueue/core/consumer",
2222
"//submitqueue/entity",
2323
"//submitqueue/extension/buildrunner",
24-
"//submitqueue/extension/buildrunner/noop",
24+
"//submitqueue/extension/buildrunner/fake",
2525
"//submitqueue/extension/changeprovider",
26+
"//submitqueue/extension/changeprovider/fake",
2627
"//submitqueue/extension/changeprovider/github",
2728
"//submitqueue/extension/conflict",
2829
"//submitqueue/extension/conflict/all",
30+
"//submitqueue/extension/conflict/fake",
31+
"//submitqueue/extension/conflict/none",
2932
"//submitqueue/extension/mergechecker",
33+
"//submitqueue/extension/mergechecker/fake",
3034
"//submitqueue/extension/mergechecker/github",
3135
"//submitqueue/extension/pusher",
36+
"//submitqueue/extension/pusher/fake",
3237
"//submitqueue/extension/pusher/git",
3338
"//submitqueue/extension/scorer",
39+
"//submitqueue/extension/scorer/composite",
40+
"//submitqueue/extension/scorer/fake",
3441
"//submitqueue/extension/scorer/heuristic",
3542
"//submitqueue/extension/storage",
3643
"//submitqueue/extension/storage/mysql",

example/submitqueue/orchestrator/server/main.go

Lines changed: 216 additions & 93 deletions
Large diffs are not rendered by default.

submitqueue/extension/buildrunner/noop/BUILD.bazel renamed to submitqueue/extension/buildrunner/fake/BUILD.bazel

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,9 @@
11
load("@rules_go//go:def.bzl", "go_library", "go_test")
22

33
go_library(
4-
name = "noop",
5-
srcs = ["noop.go"],
6-
importpath = "github.com/uber/submitqueue/submitqueue/extension/buildrunner/noop",
4+
name = "fake",
5+
srcs = ["fake.go"],
6+
importpath = "github.com/uber/submitqueue/submitqueue/extension/buildrunner/fake",
77
visibility = ["//visibility:public"],
88
deps = [
99
"//submitqueue/entity",
@@ -12,9 +12,9 @@ go_library(
1212
)
1313

1414
go_test(
15-
name = "noop_test",
16-
srcs = ["noop_test.go"],
17-
embed = [":noop"],
15+
name = "fake_test",
16+
srcs = ["fake_test.go"],
17+
embed = [":fake"],
1818
deps = [
1919
"//submitqueue/entity",
2020
"//submitqueue/extension/buildrunner",
Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,143 @@
1+
// Copyright (c) 2025 Uber Technologies, Inc.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
15+
// Package fake provides a buildrunner.BuildRunner whose outcome is driven by the
16+
// triggered changes. With no marker every build immediately succeeds, behaving
17+
// as a best-case stub for wiring and baselines. A failure can be injected
18+
// end-to-end (e.g. from an e2e land request) by embedding a marker token in a
19+
// head change URI of the form "sq-fake=<token>":
20+
//
21+
// sq-fake=build-fail -> Status reports BuildStatusFailed
22+
// sq-fake=build-error -> Status returns a non-nil error
23+
//
24+
// Because the contract reports a build's result via Status (which receives only
25+
// a BuildID), the runner records the desired outcome per build ID at Trigger
26+
// time and looks it up at Status time. This lets a single running stack exercise
27+
// negative paths purely by varying request payloads. It is intended for examples
28+
// and tests only, never production.
29+
package fake
30+
31+
import (
32+
"context"
33+
"fmt"
34+
"strings"
35+
"sync"
36+
"sync/atomic"
37+
38+
"github.com/uber/submitqueue/submitqueue/entity"
39+
"github.com/uber/submitqueue/submitqueue/extension/buildrunner"
40+
)
41+
42+
// Recognized marker tokens. See the package doc for the convention.
43+
const (
44+
tokenFail = "build-fail"
45+
tokenError = "build-error"
46+
)
47+
48+
// outcome is the result a build will report at Status time, decided from the
49+
// head changes at Trigger time.
50+
type outcome int
51+
52+
const (
53+
outcomeSucceeded outcome = iota
54+
outcomeFailed
55+
outcomeError
56+
)
57+
58+
// runner is a buildrunner.BuildRunner that reports every build as succeeded
59+
// unless a marker token in a head change URI requests otherwise. The desired
60+
// outcome is recorded per build ID at Trigger and read back at Status. The
61+
// atomic counter hands out unique build IDs; the mutex guards the outcome map.
62+
type runner struct {
63+
counter atomic.Uint64
64+
mu sync.Mutex
65+
outcomes map[string]outcome
66+
}
67+
68+
// New returns a buildrunner.BuildRunner that defaults to succeeding and honors
69+
// marker tokens embedded in head change URIs.
70+
func New() buildrunner.BuildRunner {
71+
return &runner{outcomes: make(map[string]outcome)}
72+
}
73+
74+
// Trigger records the desired outcome for a new build (decided from a marker
75+
// token in the head changes) and returns its unique build ID. The base changes
76+
// and metadata are ignored.
77+
func (r *runner) Trigger(_ context.Context, _ []entity.Change, head []entity.Change, _ entity.BuildMetadata) (entity.BuildID, error) {
78+
o := outcomeSucceeded
79+
switch markerToken(head) {
80+
case tokenFail:
81+
o = outcomeFailed
82+
case tokenError:
83+
o = outcomeError
84+
}
85+
86+
id := fmt.Sprintf("fake-%d", r.counter.Add(1))
87+
r.mu.Lock()
88+
r.outcomes[id] = o
89+
r.mu.Unlock()
90+
return entity.BuildID{ID: id}, nil
91+
}
92+
93+
// Status returns the outcome recorded for the build at Trigger time. Unknown
94+
// build IDs default to succeeded, keeping the runner best-case for builds it did
95+
// not create.
96+
func (r *runner) Status(_ context.Context, buildID entity.BuildID) (entity.BuildStatus, entity.BuildMetadata, error) {
97+
r.mu.Lock()
98+
o := r.outcomes[buildID.ID]
99+
r.mu.Unlock()
100+
101+
switch o {
102+
case outcomeFailed:
103+
return entity.BuildStatusFailed, nil, nil
104+
case outcomeError:
105+
return entity.BuildStatusUnknown, nil, fmt.Errorf("fake: marked build error")
106+
default:
107+
return entity.BuildStatusSucceeded, nil, nil
108+
}
109+
}
110+
111+
// Cancel is a no-op and always succeeds.
112+
func (r *runner) Cancel(_ context.Context, _ entity.BuildID) error {
113+
return nil
114+
}
115+
116+
// markerToken returns the first marker token found across all changes' URIs,
117+
// or "" if none carry one.
118+
func markerToken(changes []entity.Change) string {
119+
for _, change := range changes {
120+
if tok := fakeToken(change.URIs); tok != "" {
121+
return tok
122+
}
123+
}
124+
return ""
125+
}
126+
127+
// markerPrefix introduces a marker token in a change URI: "sq-fake=<token>".
128+
const markerPrefix = "sq-fake="
129+
130+
// fakeToken returns the marker token embedded in the first URI that carries
131+
// one, or "" if none do. The token ends at the first "&" or "#" delimiter.
132+
func fakeToken(uris []string) string {
133+
for _, u := range uris {
134+
if i := strings.Index(u, markerPrefix); i >= 0 {
135+
rest := u[i+len(markerPrefix):]
136+
if j := strings.IndexAny(rest, "&#"); j >= 0 {
137+
rest = rest[:j]
138+
}
139+
return rest
140+
}
141+
}
142+
return ""
143+
}

submitqueue/extension/buildrunner/noop/noop_test.go renamed to submitqueue/extension/buildrunner/fake/fake_test.go

Lines changed: 48 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
package noop
15+
package fake
1616

1717
import (
1818
"context"
@@ -28,34 +28,70 @@ func TestNew_ImplementsInterface(t *testing.T) {
2828
var _ buildrunner.BuildRunner = New()
2929
}
3030

31-
func TestRunner_Trigger(t *testing.T) {
31+
func TestRunner_Trigger_UniqueIDs(t *testing.T) {
3232
r := New()
3333
ctx := context.Background()
3434

35-
id1, err := r.Trigger(ctx,
36-
[]entity.Change{{URIs: []string{"github://owner/repo/pull/1"}}},
37-
[]entity.Change{{URIs: []string{"github://owner/repo/pull/2"}}},
38-
entity.BuildMetadata{"requester": "alice"},
39-
)
35+
id1, err := r.Trigger(ctx, nil, []entity.Change{{URIs: []string{"github://o/r/pull/1/a"}}}, nil)
4036
require.NoError(t, err)
4137
assert.NotEmpty(t, id1.ID)
4238

43-
// IDs are unique across calls, even with empty inputs.
4439
id2, err := r.Trigger(ctx, nil, nil, nil)
4540
require.NoError(t, err)
4641
assert.NotEqual(t, id1, id2)
4742
}
4843

4944
func TestRunner_Status(t *testing.T) {
50-
r := New()
45+
ctx := context.Background()
46+
47+
tests := []struct {
48+
name string
49+
headURIs []string
50+
wantStatus entity.BuildStatus
51+
wantErr bool
52+
}{
53+
{
54+
name: "no marker succeeds",
55+
headURIs: []string{"github://o/r/pull/1/a"},
56+
wantStatus: entity.BuildStatusSucceeded,
57+
},
58+
{
59+
name: "build-fail marker fails",
60+
headURIs: []string{"github://o/r/pull/1/a?sq-fake=build-fail"},
61+
wantStatus: entity.BuildStatusFailed,
62+
},
63+
{
64+
name: "build-error marker errors",
65+
headURIs: []string{"github://o/r/pull/1/a?sq-fake=build-error"},
66+
wantErr: true,
67+
},
68+
}
69+
70+
for _, tt := range tests {
71+
t.Run(tt.name, func(t *testing.T) {
72+
r := New()
73+
id, err := r.Trigger(ctx, nil, []entity.Change{{URIs: tt.headURIs}}, nil)
74+
require.NoError(t, err)
5175

52-
status, meta, err := r.Status(context.Background(), entity.BuildID{ID: "any-id"})
76+
status, _, err := r.Status(ctx, id)
77+
if tt.wantErr {
78+
require.Error(t, err)
79+
return
80+
}
81+
require.NoError(t, err)
82+
assert.Equal(t, tt.wantStatus, status)
83+
})
84+
}
85+
}
86+
87+
func TestRunner_Status_UnknownBuildSucceeds(t *testing.T) {
88+
r := New()
89+
status, _, err := r.Status(context.Background(), entity.BuildID{ID: "never-triggered"})
5390
require.NoError(t, err)
5491
assert.Equal(t, entity.BuildStatusSucceeded, status)
55-
assert.Empty(t, meta)
5692
}
5793

5894
func TestRunner_Cancel(t *testing.T) {
5995
r := New()
60-
assert.NoError(t, r.Cancel(context.Background(), entity.BuildID{ID: "any-id"}))
96+
assert.NoError(t, r.Cancel(context.Background(), entity.BuildID{ID: "any"}))
6197
}

submitqueue/extension/buildrunner/noop/noop.go

Lines changed: 0 additions & 56 deletions
This file was deleted.
Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
load("@rules_go//go:def.bzl", "go_library", "go_test")
2+
3+
go_library(
4+
name = "fake",
5+
srcs = ["fake.go"],
6+
importpath = "github.com/uber/submitqueue/submitqueue/extension/changeprovider/fake",
7+
visibility = ["//visibility:public"],
8+
deps = [
9+
"//submitqueue/entity",
10+
"//submitqueue/extension/changeprovider",
11+
],
12+
)
13+
14+
go_test(
15+
name = "fake_test",
16+
srcs = ["fake_test.go"],
17+
embed = [":fake"],
18+
deps = [
19+
"//submitqueue/entity",
20+
"//submitqueue/extension/changeprovider",
21+
"@com_github_stretchr_testify//assert",
22+
"@com_github_stretchr_testify//require",
23+
],
24+
)

0 commit comments

Comments
 (0)