From 5ffac7b123bcd266fe32665f1608f7e9aa13cf1e Mon Sep 17 00:00:00 2001 From: Preetam Dwivedi Date: Mon, 5 Oct 2026 14:20:03 -0700 Subject: [PATCH] fix(demo): keep fake file metadata out of change URIs ## Summary ### Why? Changed-file hints are demo metadata, not change identity. Encoding them in URIs leaks fake-provider details into storage keys and links. ### What? Remove the sq-files URI convention and generate approximate synthetic files directly in the fake provider, seeded by each URI so processes and retries agree without catalogs or shared state. Use a small directory pool to retain overlap, and document that FOLDERS/FILES only control git/github demos. ## Test Plan - Passed all 127 unit-test targets with `make test`. - Passed the real-stack independent/stacked fake demo E2E with `./tool/bazel test //test/e2e/submitqueue:go_default_test --test_filter=TestE2EIntegration/TestFakeDemo_IndependentAndStackedChanges --strategy=TestRunner=local --test_output=errors`. - Passed `make fmt`, `make lint`, `make check-tidy`, and `make check-gazelle`. - The broader Git E2E run encountered Docker's git dubious-ownership check while fetching the bind-mounted sandbox repository. ## Issue Part of CODEM-556 --- doc/howto/QUICKSTART.md | 20 ++--- platform/fakemarker/fakemarker.go | 40 ---------- platform/fakemarker/fakemarker_test.go | 64 ---------------- .../demo/provider/fake/profiles.yaml | 9 +-- service/submitqueue/demo/requests/BUILD.bazel | 2 - service/submitqueue/demo/requests/fake.go | 9 +-- service/submitqueue/demo/requests/main.go | 12 ++- service/submitqueue/demo/requests/source.go | 53 +------------ .../submitqueue/demo/requests/source_test.go | 36 +-------- .../orchestrator/server/profiles.go | 2 +- .../extension/changeprovider/fake/fake.go | 47 ++++-------- .../changeprovider/fake/fake_test.go | 60 +++++++++------ test/e2e/submitqueue/BUILD.bazel | 5 ++ test/e2e/submitqueue/fake_demo_test.go | 75 +++++++++++++++++++ 14 files changed, 162 insertions(+), 272 deletions(-) create mode 100644 test/e2e/submitqueue/fake_demo_test.go diff --git a/doc/howto/QUICKSTART.md b/doc/howto/QUICKSTART.md index a094f8a0c..ffe0d6bff 100644 --- a/doc/howto/QUICKSTART.md +++ b/doc/howto/QUICKSTART.md @@ -6,7 +6,7 @@ The stack always runs the same way. What changes is where the changes come from | `PROVIDER` | A change is | Read from | Building it | Landing it | Needs | |---|---|---|---|---|---| -| **`fake`** (default) | a URI, and nothing else | the URI itself | instant fake pass | reports success without touching a repository | nothing | +| **`fake`** (default) | a URI, and nothing else | deterministic synthetic files | instant fake pass | reports success without touching a repository | nothing | | **`git`** | a branch in a bare repository on disk | the repository | instant fake pass | a real fetch, cherry-pick and push | nothing | | **`github`** | a real pull request | GitHub's API | a real GitHub Actions run per batch | a real push to a real repository | a repository, a token, and CI minutes | @@ -14,7 +14,7 @@ They are a ladder, not alternatives: the same commands work on each rung, so you The queue's own logic is real on every rung; what changes is how much of the world around it is. The one thing to keep in mind before reading a `landed` as more than it is: on `fake` and `git` **the build is faked**, so it means the pipeline ran, not that anything was tested. -"Read from" is what the queue knows about a change — which files it touches, how large it is — and it is what conflict analysis and scoring are computed from. Only `fake` invents it: a change there is a URI pointing at nothing, so `make demo-requests` states the paths on the URI itself (`sq-files=`) and the fake reads them back, which means a change submitted by hand on that rung conflicts with nothing. On `git` the orchestrator keeps its own copy of the repository and reads the commits, so a change pushed by anyone is described correctly. +"Read from" is what the queue knows about a change — which files it touches, how large it is — and it is what conflict analysis and scoring are computed from. Only `fake` invents it: the provider generates pseudo-random files from each clean change URI, using a small directory pool so some changes overlap. The same URI always resolves to the same files, including across processes and retries; no file hints or shared fixtures are needed. `FOLDERS` and `FILES` control the git/github generators, not this synthetic metadata. On `git` the orchestrator keeps its own copy of the repository and reads the commits, so a change pushed by anyone is described correctly. ## Start the stack @@ -43,7 +43,7 @@ make demo-requests `demo-requests` creates changes, enqueues each the moment it exists, and watches them all until they settle: ``` -Creating 3 change(s) across 8 folder(s) via fake changes (no repository) — independent, 5 at a time, each enqueued as soon as it is created +Creating 3 synthetic change(s) via fake changes (no repository) — independent, 5 at a time, each enqueued as soon as it is created REQUEST CHANGES ELAPSED STAGE ──────────── ────────────────── ─────── ───────────────────────────────────────────── @@ -58,9 +58,9 @@ Each row shows the states its request passed through, not just the one it is in, ```bash make demo-requests COUNT=8 # more traffic -make demo-requests FOLDERS=1 # every change in one folder: all of them conflict -make demo-requests FOLDERS=50 # a folder each: none of them conflict -make demo-requests FILES=8 # wider changes, more files each +make demo-requests FOLDERS=1 # git/github: every change in one folder +make demo-requests FOLDERS=50 # git/github: spread changes across more folders +make demo-requests FILES=8 # git/github: wider changes, more files each make demo-requests CONCURRENCY=1 # create them one at a time make demo-requests STACKED=true # one stack, enqueued as a single request make demo-requests LAND=false # create only, print the command to enqueue them @@ -68,11 +68,11 @@ make demo-requests LAND=false # create only, print the command to enqu Independent changes are created **five at a time** by default (`CONCURRENCY`), because creating them serially is most of what a large run spends its time on and it delays the overlap the demo exists to show. A stack ignores the setting: each of its changes is based on the branch before it, so the next cannot be cut until the previous head exists. -A change touches several files rather than one, each committed separately, so it arrives as a multi-file, multi-commit change — closer to a real one, and enough to exercise replaying a range of commits. `FILES` sets the floor (default 3); the actual count varies a little above it, derived from the run tag so replaying a tag reproduces the same run. +On git/github, a change touches several files rather than one, each committed separately, so it arrives as a multi-file, multi-commit change — closer to a real one, and enough to exercise replaying a range of commits. `FILES` sets the floor (default 3); the actual count varies a little above it, derived from the run tag so replaying a tag reproduces the same run. -Every change writes all of its files into one folder under `demo/`, and `FOLDERS` decides how many folders there are to land in — by default a number between five and ten, picked per run. That is what makes a run interesting rather than uniform, because `demo-queue` uses the `pathoverlap` analyzer keyed on the directory: two changes landing in the same folder are batched in order and the second speculates on the first, while changes in different folders go out beside each other. A run prints the number it picked, and repeating a run tag reproduces the same collisions. +On git/github, every change writes all of its files into one folder under `demo/`, and `FOLDERS` decides how many folders there are to land in — by default a number between five and ten, picked per run. That is what makes a run interesting rather than uniform, because `demo-queue` uses the `pathoverlap` analyzer keyed on the directory: two changes landing in the same folder are batched in order and the second speculates on the first, while changes in different folders go out beside each other. A run prints the number it picked, and repeating a run tag reproduces the same collisions. -Set it deliberately when you want a run to show one thing. `FOLDERS=1` puts every change in the same place, so the queue serializes the lot and each change speculates on the one before it. A number well above `COUNT` keeps them all apart, so they go out together. +In those modes, set it deliberately when you want a run to show one thing. `FOLDERS=1` puts every change in the same place, so the queue serializes the lot and each change speculates on the one before it. A number well above `COUNT` keeps them all apart, so they go out together. How much speculation that turns into is capped by the queue's **build budget** — how many builds it may have occupying CI at once, counted across every in-flight batch rather than per batch. It defaults to 4 and is set per queue in the provider's `profiles.yaml`. The demo also sets **evidence scorer factors** there so speculation ranking revises the base price when a path passes or fails or a batch is merging or cancelling; omitting `factors` leaves every factor at `1`, which is a no-op and ranks on the nested base alone. @@ -375,7 +375,7 @@ That request walks the same path as far as `speculating`, records `building`, an A hand-written URI like the one above belongs to the `fake` rung alone. On `git` it names a commit the merger cannot fetch, and on `github` the change provider tries to resolve it as a pull request — both fail, but for reasons that have nothing to do with the marker. -Submit a good change into the **same folder** as a failing one and you can watch what makes a queue worth having: the two are batched in order, and the second speculates on the first landing. When the first fails, that guess is contradicted, the second re-plans, and it lands anyway. +Submit a good change whose synthetic directory overlaps a failing one and you can watch what makes a queue worth having: the two are batched in order, and the second speculates on the first landing. When the first fails, that guess is contradicted, the second re-plans, and it lands anyway. ## Clean up diff --git a/platform/fakemarker/fakemarker.go b/platform/fakemarker/fakemarker.go index c86f9a9c3..741c5976a 100644 --- a/platform/fakemarker/fakemarker.go +++ b/platform/fakemarker/fakemarker.go @@ -21,7 +21,6 @@ package fakemarker import ( - "net/url" "strings" "github.com/uber/submitqueue/platform/base/change" @@ -57,42 +56,3 @@ func TokenInChanges(changes []change.Change) string { } return "" } - -// FilesPrefix introduces the paths a change touches: "sq-files=a.txt,b/c.txt". -// -// A real provider is asked what a change changed. A fake one has no repository -// to ask, so the caller states it — which is what lets a conflict analyzer that -// keys on paths do its actual job against changes that were never pushed -// anywhere. -const FilesPrefix = "sq-files=" - -// Files returns the paths listed by the first URI carrying a file marker, or nil -// if none do. Paths are comma-separated and percent-decoded, and the list ends -// at the first "&" or "#" so it can sit among other query parameters. -func Files(uris []string) []string { - for _, u := range uris { - i := strings.Index(u, FilesPrefix) - if i < 0 { - continue - } - rest := u[i+len(FilesPrefix):] - if j := strings.IndexAny(rest, "&#"); j >= 0 { - rest = rest[:j] - } - - var paths []string - for _, raw := range strings.Split(rest, ",") { - decoded, err := url.QueryUnescape(raw) - if err != nil { - // A path that will not decode is not worth failing a demo over; - // the marker is a convenience, not a contract. - decoded = raw - } - if trimmed := strings.TrimSpace(decoded); trimmed != "" { - paths = append(paths, trimmed) - } - } - return paths - } - return nil -} diff --git a/platform/fakemarker/fakemarker_test.go b/platform/fakemarker/fakemarker_test.go index 1fd4bc465..90e354789 100644 --- a/platform/fakemarker/fakemarker_test.go +++ b/platform/fakemarker/fakemarker_test.go @@ -21,70 +21,6 @@ import ( "github.com/uber/submitqueue/platform/base/change" ) -func TestFiles(t *testing.T) { - tests := []struct { - name string - uris []string - want []string - }{ - { - name: "no uris", - uris: nil, - want: nil, - }, - { - name: "no marker", - uris: []string{"git://git.example.com/r/refs%2Fheads%2Fa/abc"}, - want: nil, - }, - { - name: "one path", - uris: []string{"git://git.example.com/r/x/y?sq-files=demo/alpha/one.txt"}, - want: []string{"demo/alpha/one.txt"}, - }, - { - name: "several paths", - uris: []string{"git://git.example.com/r/x/y?sq-files=demo/alpha/one.txt,demo/beta/two.txt"}, - want: []string{"demo/alpha/one.txt", "demo/beta/two.txt"}, - }, - { - name: "percent-encoded path", - uris: []string{"git://git.example.com/r/x/y?sq-files=demo%2Falpha%2Fone.txt"}, - want: []string{"demo/alpha/one.txt"}, - }, - { - name: "trimmed at the next parameter", - uris: []string{"git://git.example.com/r/x/y?sq-files=demo/alpha/one.txt&sq-fake=build-fail"}, - want: []string{"demo/alpha/one.txt"}, - }, - { - // The two markers are independent, and one change may carry both. - name: "found after another parameter", - uris: []string{"git://git.example.com/r/x/y?sq-fake=build-fail&sq-files=demo/alpha/one.txt"}, - want: []string{"demo/alpha/one.txt"}, - }, - { - name: "empty entries are dropped", - uris: []string{"git://git.example.com/r/x/y?sq-files=demo/alpha/one.txt,,"}, - want: []string{"demo/alpha/one.txt"}, - }, - { - name: "marker on a later uri", - uris: []string{ - "git://git.example.com/r/x/y", - "git://git.example.com/r/x/z?sq-files=demo/gamma/three.txt", - }, - want: []string{"demo/gamma/three.txt"}, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, Files(tt.uris)) - }) - } -} - func TestToken(t *testing.T) { tests := []struct { name string diff --git a/service/submitqueue/demo/provider/fake/profiles.yaml b/service/submitqueue/demo/provider/fake/profiles.yaml index ffca1bac4..aca691dae 100644 --- a/service/submitqueue/demo/provider/fake/profiles.yaml +++ b/service/submitqueue/demo/provider/fake/profiles.yaml @@ -11,7 +11,7 @@ # difference from ../git and ../github a diff rather than an explanation. defaults: - # No provider to ask, so the fake echoes back each URI it is given. + # The fake generates stable pseudo-random files for each URI. changeProvider: {type: fake} # Every build succeeds immediately, so a land completes in seconds. buildRunner: {type: fake} @@ -43,10 +43,7 @@ queues: # github modes use, so what a run shows does not depend on which one it ran # against. # - # This keys on the files a change reports, and no provider can be asked about - # a change that exists nowhere. `make demo-requests` therefore states the - # paths on the change URI (`sq-files=`) and the fake change provider reports - # them back. A change submitted by hand carries no such marker, touches - # nothing as far as the analyzer can tell, and so never conflicts. + # Fake files use a small fixed directory pool to produce overlap. The + # generator's FOLDERS and FILES knobs only affect the git/github modes. - name: demo-queue analyzer: {type: pathoverlap, by: directory} diff --git a/service/submitqueue/demo/requests/BUILD.bazel b/service/submitqueue/demo/requests/BUILD.bazel index e5a0dc656..5c4cf9eec 100644 --- a/service/submitqueue/demo/requests/BUILD.bazel +++ b/service/submitqueue/demo/requests/BUILD.bazel @@ -15,7 +15,6 @@ go_library( "//api/base/mergestrategy/protopb:go_default_library", "//platform/base/change/git:go_default_library", "//platform/base/change/github:go_default_library", - "//platform/fakemarker:go_default_library", "//platform/git/exec:go_default_library", "//submitqueue/client:go_default_library", "@org_golang_x_sync//errgroup:go_default_library", @@ -48,7 +47,6 @@ go_test( deps = [ "//api/base/mergestrategy/protopb:go_default_library", "//platform/base/change/git:go_default_library", - "//platform/fakemarker:go_default_library", "//platform/git/exec:go_default_library", "//platform/git/exectest:go_default_library", "//submitqueue/client:go_default_library", diff --git a/service/submitqueue/demo/requests/fake.go b/service/submitqueue/demo/requests/fake.go index ee4c6a005..caee80e38 100644 --- a/service/submitqueue/demo/requests/fake.go +++ b/service/submitqueue/demo/requests/fake.go @@ -37,7 +37,7 @@ const fakeRepo = "demo" // This is what the default provider submits. It is the fastest way to watch the // queue work, and the reason the quickstart needs neither a repository nor a // credential — at the cost of the URIs pointing at nothing, which is only -// sound because the fake change provider echoes back whatever it is handed and +// sound because the fake change provider invents metadata and // the noop merger never tries to fetch it. type fakeSource struct{} @@ -52,13 +52,10 @@ func (fakeSource) open(_ context.Context, spec changeSpec) (openedChange, error) headSHA := syntheticSHA("head", spec.branch) return openedChange{ headSHA: headSHA, - // No files are written anywhere, but the change still says which paths - // it would have touched, so the conflict analyzer has something to key - // on. It is the only claim in this mode that is not backed by anything. - uri: withFiles(gitchange.ChangeID{ + uri: gitchange.ChangeID{ Scheme: "git", Remote: fakeRemote, Repo: fakeRepo, Ref: "refs/heads/" + spec.branch, CommitSHA: headSHA, - }.String(), spec.files), + }.String(), // There is no pull request to number and nothing to link to, so the // branch name is what identifies the change. An empty URL renders as // plain text rather than as a link that goes nowhere. diff --git a/service/submitqueue/demo/requests/main.go b/service/submitqueue/demo/requests/main.go index e978177b9..a43807b27 100644 --- a/service/submitqueue/demo/requests/main.go +++ b/service/submitqueue/demo/requests/main.go @@ -115,9 +115,9 @@ func parseFlags() config { flag.StringVar(&c.base, "base", "main", "branch the changes target") flag.IntVar(&c.count, "count", 3, "how many changes to create") flag.IntVar(&c.folders, "folders", 0, - "how many folders to spread the changes across; 0 picks one per run. Changes sharing a folder are batched in order, changes in different folders go out together") + "git/github: how many folders to spread the changes across; 0 picks one per run. Changes sharing a folder are batched in order, changes in different folders go out together") flag.IntVar(&c.files, "files", 3, - "fewest files each change touches; the actual count varies a little above it. Ignored by -provider fake, which writes none") + "fewest files each change touches; the actual count varies a little above it. Ignored by -provider fake, which synthesizes its own paths") flag.IntVar(&c.concurrency, "concurrency", 5, "how many changes to create at once; a stack ignores it, being sequential by nature, and -provider git serializes its git commands") flag.BoolVar(&c.stacked, "stacked", false, "chain the changes and enqueue them as one stack") @@ -178,8 +178,12 @@ func run(ctx context.Context, cfg config) error { // Resolved once, so every change in the run is dealt into the same tree and // the number can be reported rather than inferred from the paths. cfg.folders = resolveFolders(tag, cfg.folders) - fmt.Printf("Creating %d change(s) across %d folder(s) via %s — %s\n\n", - cfg.count, cfg.folders, target(cfg), shape(cfg)) + if cfg.provider == providerFake { + fmt.Printf("Creating %d synthetic change(s) via %s — %s\n\n", cfg.count, target(cfg), shape(cfg)) + } else { + fmt.Printf("Creating %d change(s) across %d folder(s) via %s — %s\n\n", + cfg.count, cfg.folders, target(cfg), shape(cfg)) + } // Every row is known before anything is created: one per change, or a // single one for a stack, since the whole chain lands as one request. The diff --git a/service/submitqueue/demo/requests/source.go b/service/submitqueue/demo/requests/source.go index b51d109d9..44973cd13 100644 --- a/service/submitqueue/demo/requests/source.go +++ b/service/submitqueue/demo/requests/source.go @@ -16,10 +16,7 @@ package main import ( "context" - "net/url" - "path" - "github.com/uber/submitqueue/platform/fakemarker" "github.com/uber/submitqueue/submitqueue/client" ) @@ -47,8 +44,8 @@ type changeSource interface { } // changeSpec is one change to create: a branch cut from a parent, carrying -// files. The caller decides what a change is made of, so every provider -// produces the same shape of change. +// files. Real providers commit these files; the fake synthesizes its own +// metadata instead. type changeSpec struct { // branch is the ref to create. branch string @@ -74,52 +71,6 @@ type changeFile struct { message string } -// maxChangeURIBytes is the longest change URI the gateway accepts, because a -// URI is also a storage key. -const maxChangeURIBytes = 255 - -// withFiles appends to a change URI the paths it touches, for sources whose -// changes no provider can be asked about. -// -// The orchestrator's conflict analyzer keys on the files a change reports, and -// gets them from the change provider. With no provider behind fake and git -// changes, the run that authored them is the only thing that knows — so it says -// so on the URI, and the fake provider reads it back. Without this the analyzer -// sees a change that touches nothing, and a batch that touches nothing conflicts -// with nothing. -// -// One path per directory, not all of them. The demo's analyzer keys on the -// directory, so a second file in a directory already named adds a key that is -// already there — while the URI has a fixed byte budget that a change touching -// eight files would blow straight through. Paths that do not fit are dropped -// rather than truncated: a shortened path is a different directory, which would -// be worse than an unreported one. -func withFiles(base string, files []changeFile) string { - seen := make(map[string]struct{}, len(files)) - marker := "?" + fakemarker.FilesPrefix - - for _, f := range files { - dir := path.Dir(f.path) - if _, ok := seen[dir]; ok { - continue - } - entry := url.QueryEscape(f.path) - if len(seen) > 0 { - entry = "," + entry - } - if len(base)+len(marker)+len(entry) > maxChangeURIBytes { - break - } - seen[dir] = struct{}{} - marker += entry - } - - if len(seen) == 0 { - return base - } - return base + marker -} - // openedChange is what a run needs back about a change that now exists. type openedChange struct { // headSHA is the commit the change's URI pins, and what the next change in diff --git a/service/submitqueue/demo/requests/source_test.go b/service/submitqueue/demo/requests/source_test.go index 913e48aec..49bec3d59 100644 --- a/service/submitqueue/demo/requests/source_test.go +++ b/service/submitqueue/demo/requests/source_test.go @@ -26,7 +26,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" gitchange "github.com/uber/submitqueue/platform/base/change/git" - "github.com/uber/submitqueue/platform/fakemarker" gitexec "github.com/uber/submitqueue/platform/git/exec" gitexectest "github.com/uber/submitqueue/platform/git/exectest" ) @@ -44,38 +43,6 @@ func quietSpec(branch, parentBranch, parentSHA string, files ...changeFile) chan } } -func TestWithFiles_NamesOnePathPerDirectory(t *testing.T) { - uri := withFiles("git://git.example.com/r/x/y", []changeFile{ - {path: "demo/alpha/one.txt"}, - {path: "demo/alpha/two.txt"}, - {path: "demo/beta/three.txt"}, - }) - - assert.Equal(t, []string{"demo/alpha/one.txt", "demo/beta/three.txt"}, fakemarker.Files([]string{uri}), - "a second file in a directory already named adds no key the analyzer does not have") -} - -// The gateway rejects a change URI over 255 bytes, so a change touching many -// directories must lose paths rather than produce a request that cannot be -// submitted at all. -func TestWithFiles_StaysWithinTheURIBudget(t *testing.T) { - base := "git://git.example.com/sandbox/refs%2Fheads%2Fdemo%2F0814-154238%2F1/" + strings.Repeat("a", 40) - files := make([]changeFile, 0, 20) - for i := range 20 { - files = append(files, changeFile{path: fmt.Sprintf("demo/area-%02d/0814-154238-1-%d.txt", i, i)}) - } - - uri := withFiles(base, files) - assert.LessOrEqual(t, len(uri), maxChangeURIBytes) - assert.NotEmpty(t, fakemarker.Files([]string{uri}), "some paths must still be reported") -} - -func TestWithFiles_LeavesTheURIAloneWithNoFiles(t *testing.T) { - assert.Equal(t, "git://git.example.com/r/x/y", withFiles("git://git.example.com/r/x/y", nil)) -} - -// Every URI a run submits has to survive the gateway's validation, marker and -// all — which is what the first attempt at this got wrong. func TestSources_ProduceSubmittableURIs(t *testing.T) { files := make([]changeFile, 0, 8) for k := 1; k <= 8; k++ { @@ -85,7 +52,8 @@ func TestSources_ProduceSubmittableURIs(t *testing.T) { opened, err := fakeSource{}.open(context.Background(), spec) require.NoError(t, err) - assert.LessOrEqual(t, len(opened.uri), maxChangeURIBytes) + assert.NotContains(t, opened.uri, "?") + assert.LessOrEqual(t, len(opened.uri), 255) _, err = gitchange.ParseChangeID(opened.uri) assert.NoError(t, err) } diff --git a/service/submitqueue/orchestrator/server/profiles.go b/service/submitqueue/orchestrator/server/profiles.go index 73d2ad7be..83c1ec3a5 100644 --- a/service/submitqueue/orchestrator/server/profiles.go +++ b/service/submitqueue/orchestrator/server/profiles.go @@ -480,7 +480,7 @@ func (b *profileBuilder) newRoutingChangeProviderFactory(cfg changeProviderConfi } if makeGitHub == nil && makePhab == nil { - b.logger.Warn("no change provider tokens set; using fake change provider (empty change info unless URI-marked)", + b.logger.Warn("no change provider tokens set; using fake change provider (synthetic file metadata)", zap.String("profile", where)) return changeProviderFunc(func(c changeprovider.Config) (changeprovider.ChangeProvider, error) { return cpfake.New(c), nil diff --git a/submitqueue/extension/changeprovider/fake/fake.go b/submitqueue/extension/changeprovider/fake/fake.go index b98c07793..39ae60405 100644 --- a/submitqueue/extension/changeprovider/fake/fake.go +++ b/submitqueue/extension/changeprovider/fake/fake.go @@ -12,26 +12,16 @@ // See the License for the specific language governing permissions and // limitations under the License. -// Package fake provides a changeprovider.ChangeProvider whose outcome is driven -// by the input change. With no marker it returns one empty ChangeInfo per URI, -// behaving as a best-case stub for wiring and baselines. A failure can be -// injected end-to-end (e.g. from an e2e land request) by embedding a marker -// token in a change URI of the form "sq-fake=": -// -// sq-fake=provider-error -> non-nil error -// -// A URI may also carry the paths its change touches, which a real provider would -// have reported from the repository: -// -// sq-files=pkg/a/one.go,pkg/b/two.go -// -// This lets a single running stack exercise negative paths purely by varying -// request payloads. It is intended for examples and tests only, never -// production. +// Package fake provides a changeprovider.ChangeProvider with synthetic file +// metadata. File paths are pseudo-random but stable for each URI, so retries +// and separate processes resolve the same change without shared state. +// A change-URI marker "sq-fake=provider-error" injects a provider failure. +// This provider is intended for examples and tests only, never production. package fake import ( "context" + "crypto/sha256" "fmt" "github.com/uber/submitqueue/platform/fakemarker" @@ -42,27 +32,18 @@ import ( // Recognized marker tokens. See the package doc for the convention. const tokenError = "provider-error" -// provider is a changeprovider.ChangeProvider that returns empty change info -// unless a marker token in a change URI requests a failure. type provider struct { // cfg is the per-queue identity this provider was built for. cfg changeprovider.Config } -// New returns a changeprovider.ChangeProvider bound to the queue named in cfg -// that defaults to returning one empty ChangeInfo per URI and honors marker -// tokens embedded in change URIs. +// New returns a fake provider bound to the queue named in cfg. func New(cfg changeprovider.Config) changeprovider.ChangeProvider { return provider{cfg: cfg} } -// Get returns one ChangeInfo per URI in the request's change, unless a recognized -// marker token requests a failure. The "one ChangeInfo per URI" contract is preserved. -// -// A URI may also state the paths it touches, which is what lets a path-keyed -// conflict analyzer work against changes no provider knows about. Line counts -// are reported as a single added line each: the analyzers that read paths do not -// weigh them, and inventing a number would only look like data. +// Get returns one synthetic ChangeInfo per URI, in input order, unless a +// recognized marker requests a failure. func (provider) Get(_ context.Context, request entity.Request) ([]entity.ChangeInfo, error) { change := request.Change if fakemarker.Token(change.URIs) == tokenError { @@ -72,9 +53,13 @@ func (provider) Get(_ context.Context, request entity.Request) ([]entity.ChangeI infos := make([]entity.ChangeInfo, 0, len(change.URIs)) for _, uri := range change.URIs { info := entity.ChangeInfo{URI: uri} - for _, path := range fakemarker.Files([]string{uri}) { - info.Details.ChangedFiles = append(info.Details.ChangedFiles, - entity.ChangedFile{Path: path, LinesAdded: 1}) + // A small directory pool makes distinct changes overlap in the demo. + sum := sha256.Sum256([]byte(uri)) + for i := range 1 + int(sum[1]%4) { + info.Details.ChangedFiles = append(info.Details.ChangedFiles, entity.ChangedFile{ + Path: fmt.Sprintf("demo/%02d/%x-%d.txt", sum[0]%8, sum[2:10], i), + LinesAdded: 1, + }) } infos = append(infos, info) } diff --git a/submitqueue/extension/changeprovider/fake/fake_test.go b/submitqueue/extension/changeprovider/fake/fake_test.go index 1bb04b54c..a630b890c 100644 --- a/submitqueue/extension/changeprovider/fake/fake_test.go +++ b/submitqueue/extension/changeprovider/fake/fake_test.go @@ -16,6 +16,8 @@ package fake import ( "context" + "fmt" + "path" "testing" "github.com/stretchr/testify/assert" @@ -69,32 +71,44 @@ func TestProvider_Get_ErrorMarker(t *testing.T) { require.Error(t, err) } -// Without this the path-keyed conflict analyzers see a change that touches -// nothing, and a batch that touches nothing conflicts with nothing — so a queue -// configured to serialize on overlap silently runs everything in parallel. -func TestProvider_Get_ReportsFilesFromTheURI(t *testing.T) { - p := New(testCfg) - - infos, err := p.Get(context.Background(), entity.Request{Change: change.Change{ - URIs: []string{"git://git.example.com/sandbox/refs%2Fheads%2Fa/abc?sq-files=demo/alpha/one.txt,demo/alpha/two.txt"}, - }}) +func TestProvider_Get_SyntheticFilesAreStable(t *testing.T) { + ctx := context.Background() + request := entity.Request{Change: change.Change{URIs: []string{ + "git://demo.example.com/demo/refs%2Fheads%2Fa/abc", + "git://demo.example.com/demo/refs%2Fheads%2Fb/def", + }}} + first, err := New(testCfg).Get(ctx, request) require.NoError(t, err) - require.Len(t, infos, 1) - - paths := make([]string, 0, len(infos[0].Details.ChangedFiles)) - for _, f := range infos[0].Details.ChangedFiles { - paths = append(paths, f.Path) + for _, info := range first { + require.NotEmpty(t, info.Details.ChangedFiles) + for _, file := range info.Details.ChangedFiles { + assert.NotEmpty(t, file.Path) + assert.Positive(t, file.LinesAdded) + } + } + for range 2 { + again, err := New(testCfg).Get(ctx, request) + require.NoError(t, err) + assert.Equal(t, first, again) } - assert.Equal(t, []string{"demo/alpha/one.txt", "demo/alpha/two.txt"}, paths) + request.Change.URIs[0], request.Change.URIs[1] = request.Change.URIs[1], request.Change.URIs[0] + reordered, err := New(testCfg).Get(ctx, request) + require.NoError(t, err) + assert.Equal(t, []entity.ChangeInfo{first[1], first[0]}, reordered) } -func TestProvider_Get_ReportsNoFilesWithoutTheMarker(t *testing.T) { - p := New(testCfg) - - infos, err := p.Get(context.Background(), entity.Request{Change: change.Change{ - URIs: []string{"git://git.example.com/sandbox/refs%2Fheads%2Fa/abc"}, - }}) +func TestProvider_Get_ProducesOverlappingAndIndependentDirectories(t *testing.T) { + var uris []string + for i := range 64 { + uris = append(uris, fmt.Sprintf("git://demo.example.com/demo/ref/%040d", i)) + } + infos, err := New(testCfg).Get(context.Background(), entity.Request{Change: change.Change{URIs: uris}}) require.NoError(t, err) - require.Len(t, infos, 1) - assert.Empty(t, infos[0].Details.ChangedFiles) + directories := make(map[string]int) + for _, info := range infos { + require.NotEmpty(t, info.Details.ChangedFiles) + directories[path.Dir(info.Details.ChangedFiles[0].Path)]++ + } + assert.Greater(t, len(directories), 1, "some changes must be independent") + assert.Less(t, len(directories), len(uris), "some changes must overlap") } diff --git a/test/e2e/submitqueue/BUILD.bazel b/test/e2e/submitqueue/BUILD.bazel index c2ea850cc..cbbe11849 100644 --- a/test/e2e/submitqueue/BUILD.bazel +++ b/test/e2e/submitqueue/BUILD.bazel @@ -3,6 +3,7 @@ load("@rules_go//go:def.bzl", "go_test") go_test( name = "go_default_test", srcs = [ + "fake_demo_test.go", "git_suite_test.go", "harness_test.go", "suite_test.go", @@ -14,6 +15,7 @@ go_test( "//service/submitqueue:docker-compose.git.yml", "//service/submitqueue:docker-compose.yml", "//service/submitqueue/demo/provider/git:config", + "//service/submitqueue/demo/requests", "//service/submitqueue/gateway/server:docker_test_context", "//service/submitqueue/orchestrator/server:docker_test_context", "//submitqueue/extension/storage:schema", @@ -39,6 +41,7 @@ go_test( "//api/runway/messagequeue:go_default_library", "//api/submitqueue/gateway/protopb:go_default_library", "//api/submitqueue/orchestrator/protopb:go_default_library", + "//platform/base/change:go_default_library", "//platform/base/messagequeue:go_default_library", "//platform/consumer:go_default_library", "//platform/extension/consumergate:go_default_library", @@ -49,6 +52,8 @@ go_test( "//submitqueue/core/messagequeue:go_default_library", "//submitqueue/core/topickey:go_default_library", "//submitqueue/entity:go_default_library", + "//submitqueue/extension/changeprovider:go_default_library", + "//submitqueue/extension/changeprovider/fake:go_default_library", "//submitqueue/extension/storage:go_default_library", "//submitqueue/orchestrator/core/batch:go_default_library", "//submitqueue/orchestrator/extension/storage/mysql:go_default_library", diff --git a/test/e2e/submitqueue/fake_demo_test.go b/test/e2e/submitqueue/fake_demo_test.go new file mode 100644 index 000000000..921951df5 --- /dev/null +++ b/test/e2e/submitqueue/fake_demo_test.go @@ -0,0 +1,75 @@ +// Copyright (c) 2026 Uber Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package e2e_test + +import ( + "fmt" + "os/exec" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + gatewaypb "github.com/uber/submitqueue/api/submitqueue/gateway/protopb" + "github.com/uber/submitqueue/platform/base/change" + "github.com/uber/submitqueue/submitqueue/entity" + "github.com/uber/submitqueue/submitqueue/extension/changeprovider" + cpfake "github.com/uber/submitqueue/submitqueue/extension/changeprovider/fake" + "github.com/uber/submitqueue/test/testutil" +) + +func (s *E2EIntegrationSuite) TestFakeDemo_IndependentAndStackedChanges() { + t := s.T() + const queue = "e2e-test-queue" + started := time.Now().UnixMilli() + addr, err := s.stack.ServiceHost("gateway-service", 8080) + require.NoError(t, err) + for _, stacked := range []bool{false, true} { + cmd := exec.CommandContext(s.ctx, testutil.Runfile("service/submitqueue/demo/requests/requests_/requests"), + "-provider=fake", "-addr="+addr, "-queue="+queue, "-count=3", "-watch=false", + fmt.Sprintf("-stacked=%t", stacked), fmt.Sprintf("-prefix=fake-e2e-%t", stacked)) + output, err := cmd.CombinedOutput() + require.NoError(t, err, "%s", output) + } + var summaries []*gatewaypb.RequestSummary + pollUntil(persistPollInterval, func() bool { + resp, err := s.gatewayClient.List(s.ctx, &gatewaypb.ListRequest{ + Queue: queue, ReceivedAtOrAfterMs: started, ReceivedBeforeMs: time.Now().UnixMilli() + 1, PageSize: 10, + }) + require.NoError(t, err) + summaries = resp.Requests + return len(summaries) == 4 + }) + store, err := s.appStorage.For(queue) + require.NoError(t, err) + var sizes []int + for _, summary := range summaries { + req := request{queue: queue, sqid: summary.Sqid} + require.Equal(t, entity.RequestStatusLanded, s.awaitTerminal(req)) + sizes = append(sizes, len(summary.ChangeUris)) + infos, err := cpfake.New(changeprovider.Config{QueueName: queue}).Get(s.ctx, + entity.Request{Change: change.Change{URIs: summary.ChangeUris}}) + require.NoError(t, err) + for _, info := range infos { + assert.NotContains(t, info.URI, "?") + require.NotEmpty(t, info.Details.ChangedFiles) + records, err := store.GetChangeStore().GetByURI(s.ctx, info.URI) + require.NoError(t, err) + require.Len(t, records, 1) + assert.Equal(t, info.Details, records[0].Details, + "separate processes must resolve the same synthetic files") + } + } + assert.ElementsMatch(t, []int{1, 1, 1, 3}, sizes) +}