internal/relui,task: expand testing coverage for releases Among other things, we add testing for idempotency behaviors in security coalescing, expected fail open errors per task, RC branch coalescing, and disclosure safety. Change-Id: I6ccdfb066e451b61ed80e87d43b038a0f99911ed Reviewed-on: https://go-review.googlesource.com/c/build/+/820471 Reviewed-by: Nicholas Husin <nsh@golang.org> LUCI-TryBot-Result: golang-scoped@luci-project-accounts.iam.gserviceaccount.com <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Reviewed-by: Nicholas Husin <husin@google.com> Reviewed-by: Neal Patel <nealpatel@google.com>
diff --git a/internal/relui/buildrelease_test.go b/internal/relui/buildrelease_test.go index 794d732..e5d9bb3 100644 --- a/internal/relui/buildrelease_test.go +++ b/internal/relui/buildrelease_test.go
@@ -730,6 +730,137 @@ t.Errorf("CL %s status = %q, want %q", clID, ci.Status, gerrit.ChangeStatusMerged) } } + + wantCLCount := 2 + for _, ib := range []string{ + "internal-release-branch.go1.26.1", + "internal-release-branch.go1.25.1", + } { + head, err := privGerrit.ReadBranchHead(deps.ctx, "go", ib) + if err != nil { + t.Fatalf("reading head of %s: %v", ib, err) + } + commits, err := privGerrit.ListCommits(deps.ctx, "go", head, publicHeadBefore) + if err != nil { + t.Fatalf("ListCommits on %s: %v", ib, err) + } + if got := len(commits); got != wantCLCount { + t.Errorf("branch %s has %d commits above public head, want %d", ib, got, wantCLCount) + } + wantPrefix := "[" + majorFromMinor(strings.TrimPrefix(ib, "internal-")) + "]" + var gotMessages []string + for _, ci := range commits { + gotMessages = append(gotMessages, ci.Message) + if !strings.HasPrefix(ci.Message, wantPrefix) { + t.Errorf("branch %s commit %s message %q does not start with %q", ib, ci.Commit[:8], ci.Message, wantPrefix) + } + } + } + + branchCPSets := map[string]map[string]bool{} + for _, ib := range []string{ + "internal-release-branch.go1.26.1", + "internal-release-branch.go1.25.1", + } { + head, err := privGerrit.ReadBranchHead(deps.ctx, "go", ib) + if err != nil { + t.Fatalf("reading head of %s: %v", ib, err) + } + commits, err := privGerrit.ListCommits(deps.ctx, "go", head, publicHeadBefore) + if err != nil { + t.Fatalf("ListCommits on %s: %v", ib, err) + } + msgs := map[string]bool{} + for _, ci := range commits { + bare := strings.SplitN(ci.Message, "] ", 2) + if len(bare) == 2 { + msgs[bare[1]] = true + } + } + branchCPSets[ib] = msgs + } + set26 := branchCPSets["internal-release-branch.go1.26.1"] + set25 := branchCPSets["internal-release-branch.go1.25.1"] + if len(set26) != len(set25) { + t.Errorf("cherry-pick set sizes differ: go1.26.1 has %d, go1.25.1 has %d", len(set26), len(set25)) + } + for msg := range set26 { + if !set25[msg] { + t.Errorf("cherry-pick %q on go1.26.1 but not go1.25.1", msg) + } + } +} + +func TestMinorReleaseSecurityCoalesceWithRC(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + + base, err := deps.gerrit.ReadBranchHead(deps.ctx, "go", "release-branch.go1.26") + if err != nil { + t.Fatal(err) + } + deps.goRepo.Branch("release-branch.go1.27", base) + + privGoRepo, err := privGerrit.ReadBranchHead(deps.ctx, "go", "public") + if err != nil { + t.Fatal(err) + } + if _, err := privGerrit.CreateBranch(deps.ctx, "go", "release-branch.go1.27", gerrit.BranchInput{Revision: privGoRepo}); err != nil { + t.Fatal(err) + } + + deps.buildTasks.ApproveAction = func(ctx *workflow.TaskContext) error { + if strings.Contains(ctx.TaskName, "Confirm PRIVATE-track security CLs") { + return nil + } + return fmt.Errorf("unexpected approval request for %q", ctx.TaskName) + } + + runCtx, stop := context.WithCancel(deps.ctx) + t.Cleanup(stop) + listener := &verboseListener{t: t, onStall: stop} + + publicHeadBefore, err := privGerrit.ReadBranchHead(deps.ctx, "go", "public") + if err != nil { + t.Fatalf("reading public head: %v", err) + } + + comm := task.CommunicationTasks{ + SecurityCommunicationTasks: task.SecurityCommunicationTasks{PrivateGerrit: privGerrit}, + } + wd, err := createMinorReleaseWorkflow(deps.buildTasks, deps.milestoneTasks, deps.versionTasks, comm, 25, 26) + if err != nil { + t.Fatal(err) + } + w, err := workflow.Start(wd, minorReleaseParams()) + if err != nil { + t.Fatal(err) + } + + if _, err := w.Run(runCtx, listener); err != nil && runCtx.Err() == nil { + t.Fatalf("workflow failed: %v", err) + } + + wantBranches := []string{ + "internal-release-branch.go1.27rc1", + "internal-release-branch.go1.26.1", + "internal-release-branch.go1.25.1", + } + for _, ib := range wantBranches { + head, err := privGerrit.ReadBranchHead(deps.ctx, "go", ib) + if err != nil { + t.Fatalf("reading head of %s: %v", ib, err) + } + if head == publicHeadBefore { + t.Errorf("internal branch %s head equals public head; cherry-picks did not land", ib) + } + commits, err := privGerrit.ListCommits(deps.ctx, "go", head, publicHeadBefore) + if err != nil { + t.Fatalf("ListCommits on %s: %v", ib, err) + } + if got := len(commits); got != 2 { + t.Errorf("branch %s has %d commits above public head, want 2", ib, got) + } + } } func TestMinorReleaseCoalesceNoPrivatePatches(t *testing.T) { @@ -860,6 +991,8 @@ func TestMinorReleaseSecurityCoalesceCherryPickConflict(t *testing.T) { deps, privGerrit := newMinorCoalesceTestDeps(t, true) + workflow.MaxRetries = 3 + privGerrit.AddChange("go", "1234", &gerrit.ChangeInfo{ ID: "1234", ChangeID: "1234", @@ -886,6 +1019,9 @@ return fmt.Errorf("unexpected approval request for %q", ctx.TaskName) } + runCtx, stop := context.WithCancel(deps.ctx) + t.Cleanup(stop) + comm := task.CommunicationTasks{ SecurityCommunicationTasks: task.SecurityCommunicationTasks{PrivateGerrit: privGerrit}, } @@ -898,8 +1034,8 @@ t.Fatal(err) } - tracker := &taskStartTracker{Listener: &verboseListener{t: t}} - errMsg := runToFailure(t, deps.ctx, w, "Create cherry-picks", tracker) + tracker := &taskStartTracker{Listener: &verboseListener{t: t, onStall: stop}} + errMsg := runToFailure(t, runCtx, w, "Create cherry-picks", tracker) var ( changes []*gerrit.ChangeInfo @@ -1037,6 +1173,346 @@ } } +func TestRestartInternalBranchesOpenCherryPicks(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "restart-opencp"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + branches, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatalf("first createInternalReleaseBranches: %v", err) + } + + cps, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, branches, cls) + if err != nil { + t.Fatalf("createSecurityCherryPicks: %v", err) + } + + headsBeforeRestart := map[string]string{} + for _, b := range branches { + head, err := privGerrit.ReadBranchHead(deps.ctx, "go", b) + if err != nil { + t.Fatalf("reading head of %s: %v", b, err) + } + headsBeforeRestart[b] = head + } + + branches2, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatalf("restart createInternalReleaseBranches with open CPs: %v", err) + } + if len(branches2) != len(branches) { + t.Fatalf("branch count: first=%d, restart=%d", len(branches), len(branches2)) + } + for i, b := range branches2 { + if b != branches[i] { + t.Errorf("branch[%d] = %q on restart, want %q (fixed name reuse)", i, b, branches[i]) + } + head, err := privGerrit.ReadBranchHead(deps.ctx, "go", b) + if err != nil { + t.Fatalf("reading head of %s after restart: %v", b, err) + } + if head != headsBeforeRestart[b] { + t.Errorf("branch %s head changed: got %s, want %s", b, head, headsBeforeRestart[b]) + } + } + + cps2, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, branches2, cls) + if err != nil { + t.Fatalf("restart createSecurityCherryPicks: %v", err) + } + if len(cps2) != len(cps) { + t.Fatalf("cherry-pick count: first=%d, restart=%d", len(cps), len(cps2)) + } + freshNums := map[int]bool{} + for _, cp := range cps { + freshNums[cp.ChangeNumber] = true + } + for _, cp := range cps2 { + if !freshNums[cp.ChangeNumber] { + t.Errorf("restart returned new cherry-pick CL %d; want reuse of existing CL", cp.ChangeNumber) + } + } +} + +func TestRestartInternalBranchesMergedCherryPicks(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "restart-mergedcp"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + branches, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatalf("first createInternalReleaseBranches: %v", err) + } + cps, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, branches, cls) + if err != nil { + t.Fatalf("first createSecurityCherryPicks: %v", err) + } + if _, err := deps.buildTasks.submitCherryPicks(taskCtx, cps); err != nil { + t.Fatalf("submitCherryPicks: %v", err) + } + + coalescedHeads := map[string]string{} + for _, b := range branches { + head, err := privGerrit.ReadBranchHead(deps.ctx, "go", b) + if err != nil { + t.Fatalf("reading head of %s: %v", b, err) + } + coalescedHeads[b] = head + } + + branches2, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatalf("restart createInternalReleaseBranches with merged CPs: %v", err) + } + if len(branches2) != len(branches) { + t.Fatalf("branch count mismatch: first=%d, restart=%d", len(branches), len(branches2)) + } + cps2, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, branches2, cls) + if err != nil { + t.Fatalf("restart createSecurityCherryPicks with merged CPs: %v", err) + } + if got, want := len(cps2), len(cls)*len(branches); got != want { + t.Fatalf("restart cherry-picks: got %d, want %d", got, want) + } + for _, cp := range cps2 { + if cp.Status != gerrit.ChangeStatusMerged { + t.Errorf("restart cherry-pick CL %d status = %q, want %q", cp.ChangeNumber, cp.Status, gerrit.ChangeStatusMerged) + } + } + if _, err := deps.buildTasks.submitCherryPicks(taskCtx, cps2); err != nil { + t.Fatalf("restart submitCherryPicks: %v", err) + } + + for _, b := range branches2 { + head, err := privGerrit.ReadBranchHead(deps.ctx, "go", b) + if err != nil { + t.Fatalf("reading head of %s after restart: %v", b, err) + } + if head != coalescedHeads[b] { + t.Errorf("branch %s head changed after restart: got %s, want %s", b, head, coalescedHeads[b]) + } + publicHead, err := privGerrit.ReadBranchHead(deps.ctx, "go", majorFromMinor(strings.TrimPrefix(b, "internal-"))) + if err != nil { + t.Fatal(err) + } + commits, err := privGerrit.ListCommits(deps.ctx, "go", head, publicHead) + if err != nil { + t.Fatalf("ListCommits(%s): %v", b, err) + } + if len(commits) != len(cls) { + t.Errorf("branch %s has %d security commits above public head, want %d", b, len(commits), len(cls)) + } + } +} + +func TestRestartNoCherryPickOrphan(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "restart-no-orphan"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + branches, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatal(err) + } + + cps, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, branches, cls) + if err != nil { + t.Fatal(err) + } + + branches2, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatal(err) + } + + for _, b := range branches2 { + existing, err := privGerrit.QueryChanges(deps.ctx, + fmt.Sprintf("project:go branch:%s -is:abandoned", b)) + if err != nil { + t.Fatalf("QueryChanges for %s: %v", b, err) + } + cpNums := map[int]bool{} + for _, cp := range cps { + cpNums[cp.ChangeNumber] = true + } + for _, ci := range existing { + if !cpNums[ci.ChangeNumber] { + t.Errorf("orphaned CL %d on branch %s after restart; fixed-name strategy must not orphan cherry-picks", ci.ChangeNumber, b) + } + } + } +} + +func TestRestartCherryPickDedupFixedNames(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "restart-dedup"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + branches, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatal(err) + } + + cps1, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, branches, cls) + if err != nil { + t.Fatal(err) + } + + branches2, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatal(err) + } + + cps2, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, branches2, cls) + if err != nil { + t.Fatal(err) + } + + cps3, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, branches2, cls) + if err != nil { + t.Fatal(err) + } + + if len(cps1) != len(cps2) || len(cps2) != len(cps3) { + t.Fatalf("cherry-pick counts diverge: run1=%d, run2=%d, run3=%d", len(cps1), len(cps2), len(cps3)) + } + + numsFrom1 := map[int]bool{} + for _, cp := range cps1 { + numsFrom1[cp.ChangeNumber] = true + } + for i, cp := range cps2 { + if !numsFrom1[cp.ChangeNumber] { + t.Errorf("run2 cherry-pick[%d] CL %d is new; want reuse under fixed branch names", i, cp.ChangeNumber) + } + } + for i, cp := range cps3 { + if !numsFrom1[cp.ChangeNumber] { + t.Errorf("run3 cherry-pick[%d] CL %d is new; want stable dedup", i, cp.ChangeNumber) + } + } +} + +func TestReadSecurityRefRestart(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "secref-restart"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + branches, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatal(err) + } + + for _, b := range branches { + version := strings.TrimPrefix(b, "internal-release-branch.") + commit, err := deps.buildTasks.readSecurityRef(taskCtx, version) + if err != nil { + t.Fatalf("readSecurityRef(%s): %v", version, err) + } + wantHead, err := privGerrit.ReadBranchHead(deps.ctx, "go", b) + if err != nil { + t.Fatalf("ReadBranchHead(%s): %v", b, err) + } + if commit != wantHead { + t.Errorf("readSecurityRef(%s) = %q, want %q (branch head of %s)", version, commit, wantHead, b) + } + } + + _, err = deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatal(err) + } + + for _, b := range branches { + version := strings.TrimPrefix(b, "internal-release-branch.") + commit, err := deps.buildTasks.readSecurityRef(taskCtx, version) + if err != nil { + t.Fatalf("readSecurityRef(%s) after restart: %v", version, err) + } + wantHead, err := privGerrit.ReadBranchHead(deps.ctx, "go", b) + if err != nil { + t.Fatalf("ReadBranchHead(%s) after restart: %v", b, err) + } + if commit != wantHead { + t.Errorf("readSecurityRef(%s) after restart = %q, want %q", version, commit, wantHead) + } + } + + commit, err := deps.buildTasks.readSecurityRef(taskCtx, "go1.99.99") + if err != nil { + t.Fatalf("readSecurityRef for nonexistent version: %v", err) + } + if commit != "" { + t.Errorf("readSecurityRef for nonexistent version = %q, want empty", commit) + } +} + func TestPublicizeIdempotent(t *testing.T) { if runtime.GOOS != "linux" && runtime.GOOS != "darwin" { t.Skip("Requires bash shell scripting support.") @@ -2534,3 +3010,492 @@ t.Fatalf("convertInternalChangelists error = %v, want Change-Id footer error", err) } } + +func TestSubmitCherryPicks(t *testing.T) { + t.Run("happy", func(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "submit-cp"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + checkpoint, err := deps.buildTasks.createSecurityCheckpoint(taskCtx, bi, cls) + if err != nil { + t.Fatalf("createSecurityCheckpoint: %v", err) + } + + cls, err = deps.buildTasks.moveAndRebasePrivateChanges(taskCtx, checkpoint, cls) + if err != nil { + t.Fatalf("moveAndRebasePrivateChanges: %v", err) + } + + submitted, err := deps.buildTasks.submitPrivateChanges(taskCtx, cls) + if err != nil { + t.Fatalf("submitPrivateChanges: %v", err) + } + + internalBranches, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, submitted) + if err != nil { + t.Fatalf("createInternalReleaseBranches: %v", err) + } + + cherryPicks, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, internalBranches, submitted) + if err != nil { + t.Fatalf("createSecurityCherryPicks: %v", err) + } + + result, err := deps.buildTasks.submitCherryPicks(taskCtx, cherryPicks) + if err != nil { + t.Fatalf("submitCherryPicks: %v", err) + } + if len(result) == 0 { + t.Fatal("submitCherryPicks returned empty map") + } + for branch, urls := range result { + if len(urls) == 0 { + t.Errorf("branch %s has no submitted CL URLs", branch) + } + for _, url := range urls { + if !strings.Contains(url, "go-internal-review.git.corp.google.com") { + t.Errorf("branch %s URL %q does not look like a private CL URL", branch, url) + } + } + } + + for _, cp := range cherryPicks { + ci, err := privGerrit.GetChange(deps.ctx, cp.ID) + if err != nil { + t.Fatalf("GetChange(%s): %v", cp.ID, err) + } + if ci.Status != gerrit.ChangeStatusMerged { + t.Errorf("cherry-pick %s status = %q, want %q", cp.ID, ci.Status, gerrit.ChangeStatusMerged) + } + } + }) + + t.Run("already_merged_skip", func(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "submit-cp-skip"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + checkpoint, err := deps.buildTasks.createSecurityCheckpoint(taskCtx, bi, cls) + if err != nil { + t.Fatalf("createSecurityCheckpoint: %v", err) + } + + cls, err = deps.buildTasks.moveAndRebasePrivateChanges(taskCtx, checkpoint, cls) + if err != nil { + t.Fatalf("moveAndRebasePrivateChanges: %v", err) + } + + submitted, err := deps.buildTasks.submitPrivateChanges(taskCtx, cls) + if err != nil { + t.Fatalf("submitPrivateChanges: %v", err) + } + + internalBranches, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, submitted) + if err != nil { + t.Fatalf("createInternalReleaseBranches: %v", err) + } + + cherryPicks, err := deps.buildTasks.createSecurityCherryPicks(taskCtx, internalBranches, submitted) + if err != nil { + t.Fatalf("createSecurityCherryPicks: %v", err) + } + + for _, cp := range cherryPicks { + stored, err := privGerrit.GetChange(deps.ctx, cp.ID) + if err != nil { + t.Fatalf("GetChange(%s): %v", cp.ID, err) + } + stored.Status = gerrit.ChangeStatusMerged + stored.Submittable = false + cp.Status = gerrit.ChangeStatusMerged + cp.Submittable = false + } + + result, err := deps.buildTasks.submitCherryPicks(taskCtx, cherryPicks) + if err != nil { + t.Fatalf("submitCherryPicks with pre-merged CPs: %v", err) + } + if len(result) == 0 { + t.Fatal("submitCherryPicks returned empty map") + } + }) +} + +func TestCheckPrivateChangesErrors(t *testing.T) { + t.Run("get_change_error", func(t *testing.T) { + deps, _ := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "check-err"}} + + deps.buildTasks.PrivateGerritClient = task.NewFakeGerrit(t, task.NewFakeRepo(t, "empty")) + + rm := &relmeta.ReleaseMilestone{ + Patches: []*relmeta.SecurityPatch{{ + Track: relmeta.Private, + Changelists: []string{"https://go-internal-review.git.corp.google.com/c/go/+/9999"}, + }}, + } + _, err := deps.buildTasks.checkPrivateChanges(taskCtx, rm) + if err == nil { + t.Fatal("expected error from GetChange on missing CL") + } + }) + + t.Run("not_submittable", func(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "check-notsub"}} + + stored, err := privGerrit.GetChange(deps.ctx, "1234") + if err != nil { + t.Fatal(err) + } + stored.Submittable = false + + rm := &relmeta.ReleaseMilestone{ + Patches: []*relmeta.SecurityPatch{{ + Track: relmeta.Private, + Changelists: []string{"https://go-internal-review.git.corp.google.com/c/go/+/1234"}, + }}, + } + _, err = deps.buildTasks.checkPrivateChanges(taskCtx, rm) + if err == nil { + t.Fatal("expected error for non-submittable CL") + } + if !strings.Contains(err.Error(), "not submittable") { + t.Errorf("error = %v, want 'not submittable'", err) + } + }) +} + +func TestMoveAndRebasePrivateChangesErrors(t *testing.T) { + t.Run("get_change_error", func(t *testing.T) { + deps, _ := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "move-err"}} + + fakeCL := &gerrit.ChangeInfo{ + ID: "nonexistent", + ChangeID: "nonexistent", + Branch: "public", + Submittable: true, + } + + _, err := deps.buildTasks.moveAndRebasePrivateChanges(taskCtx, "whatever", []*gerrit.ChangeInfo{fakeCL}) + if err == nil { + t.Fatal("expected error for nonexistent CL") + } + }) +} + +func TestSubmitPrivateChangesError(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "submit-err"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + checkpoint, err := deps.buildTasks.createSecurityCheckpoint(taskCtx, bi, cls) + if err != nil { + t.Fatalf("createSecurityCheckpoint: %v", err) + } + + cls, err = deps.buildTasks.moveAndRebasePrivateChanges(taskCtx, checkpoint, cls) + if err != nil { + t.Fatalf("moveAndRebasePrivateChanges: %v", err) + } + + for _, ci := range cls { + stored, err := privGerrit.GetChange(deps.ctx, ci.ID) + if err != nil { + t.Fatalf("GetChange(%s): %v", ci.ID, err) + } + stored.Submittable = false + } + + errCtx, cancel := context.WithTimeout(deps.ctx, 2*time.Second) + defer cancel() + errTaskCtx := &workflow.TaskContext{Context: errCtx, Logger: &testLogger{t: t, task: "submit-err"}} + + _, err = deps.buildTasks.submitPrivateChanges(errTaskCtx, cls) + if err == nil { + t.Fatal("expected error from submitPrivateChanges with non-submittable CLs") + } +} + +func TestCreateInternalReleaseBranchesError(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "ib-err"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + bi.PublicReleaseBranches = []string{"release-branch.go1.99"} + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + _, err = deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err == nil { + t.Fatal("expected error for nonexistent release branch") + } + _ = privGerrit +} + +func TestCreateSecurityCherryPicksConflictError(t *testing.T) { + deps, privGerrit := newMinorCoalesceTestDeps(t, true) + taskCtx := &workflow.TaskContext{Context: deps.ctx, Logger: &testLogger{t: t, task: "cp-conflict"}} + + bi, err := computeSecurityBranchInfo(taskCtx, deps.versionTasks, 26, mustGetNextMinors(t, deps)) + if err != nil { + t.Fatal(err) + } + + var cls []*gerrit.ChangeInfo + for _, num := range []string{"1234", "5678"} { + ci, err := privGerrit.GetChange(deps.ctx, num) + if err != nil { + t.Fatalf("GetChange(%s): %v", num, err) + } + cls = append(cls, ci) + } + + releaseBranches, err := deps.buildTasks.createInternalReleaseBranches(taskCtx, bi, cls) + if err != nil { + t.Fatal(err) + } + + privGerrit.AddChange("go", "1234", &gerrit.ChangeInfo{ + ID: "1234", + ChangeID: "1234", + ChangeNumber: 1234, + Branch: "public", + Submittable: true, + Mergeable: true, + ContainsGitConflicts: true, + }, "crypto/tls: fix something\n\nFixes CVE-1985-0703\nFixes golang/go#1") + + _, err = deps.buildTasks.createSecurityCherryPicks(taskCtx, releaseBranches, cls) + if err == nil { + t.Fatal("expected error from cherry-pick conflict") + } + if !strings.Contains(err.Error(), "merge conflicts") { + t.Errorf("error = %v, want 'merge conflicts'", err) + } +} + +func TestPublicizeErrors(t *testing.T) { + if runtime.GOOS != "linux" && runtime.GOOS != "darwin" { + t.Skip("Requires bash shell scripting support.") + } + + setup := func(t *testing.T) (*BuildReleaseTasks, *task.FakeGerrit, *task.FakeGerrit, string, string) { + t.Helper() + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + pubRepo := task.NewFakeRepo(t, "go") + base := pubRepo.Commit(map[string]string{"README": "hello"}) + pubRepo.Branch("release-branch.go1.26", base) + + privRepo := task.CloneFakeRepo(t, "go", pubRepo) + privRepo.Branch("internal-release-branch.go1.26.1", base) + privRepo.CommitOnBranchWithMessage("internal-release-branch.go1.26.1", + "crypto/tls: fix vuln\n\nChange-Id: I0000000000000000000000000000000000000001", + map[string]string{"security1.txt": "fix1"}) + + pubGerrit := task.NewFakeGerrit(t, pubRepo) + privGerrit := task.NewFakeGerrit(t, privRepo) + + securityCommit, err := privGerrit.ReadBranchHead(ctx, "go", "internal-release-branch.go1.26.1") + if err != nil { + t.Fatal(err) + } + + build := &BuildReleaseTasks{ + GerritClient: pubGerrit, + GerritProject: "go", + PrivateGerritClient: privGerrit, + PrivateGerritProject: "go", + Git: new(task.Git), + } + return build, pubGerrit, privGerrit, base, securityCommit + } + + t.Run("public_head_mismatch", func(t *testing.T) { + build, pubGerrit, _, base, securityCommit := setup(t) + taskCtx := &workflow.TaskContext{Context: context.Background(), Logger: &testLogger{t: t, task: "pub-mismatch"}} + + repo, err := pubGerrit.ReadBranchHead(context.Background(), "go", "release-branch.go1.26") + if err != nil { + t.Fatal(err) + } + _ = repo + + fakeOldHead := "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" + _, err = build.publicizePrivateSecurityCLs(taskCtx, + "go1.26.1", "release-branch.go1.26", fakeOldHead, securityCommit, nil) + if err == nil { + t.Fatal("expected error for public head mismatch") + } + if !strings.Contains(err.Error(), "restart the release workflow") { + t.Errorf("error = %v, want mention of restarting workflow", err) + } + _ = base + }) + + t.Run("private_head_mismatch", func(t *testing.T) { + build, _, _, base, _ := setup(t) + taskCtx := &workflow.TaskContext{Context: context.Background(), Logger: &testLogger{t: t, task: "priv-mismatch"}} + + fakeOldCommit := "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb" + _, err := build.publicizePrivateSecurityCLs(taskCtx, + "go1.26.1", "release-branch.go1.26", base, fakeOldCommit, nil) + if err == nil { + t.Fatal("expected error for private head mismatch") + } + if !strings.Contains(err.Error(), "restart the release workflow") { + t.Errorf("error = %v, want mention of restarting workflow", err) + } + }) + + t.Run("public_branch_read_error", func(t *testing.T) { + build, _, _, _, securityCommit := setup(t) + taskCtx := &workflow.TaskContext{Context: context.Background(), Logger: &testLogger{t: t, task: "pub-read-err"}} + + build.GerritClient = task.NewFakeGerrit(t, task.NewFakeRepo(t, "empty")) + + _, err := build.publicizePrivateSecurityCLs(taskCtx, + "go1.26.1", "release-branch.go1.26", "anything", securityCommit, nil) + if err == nil { + t.Fatal("expected error for missing public branch") + } + if !strings.Contains(err.Error(), "reading public branch head") { + t.Errorf("error = %v, want mention of reading public branch head", err) + } + }) + + t.Run("private_branch_read_error", func(t *testing.T) { + build, _, _, base, _ := setup(t) + taskCtx := &workflow.TaskContext{Context: context.Background(), Logger: &testLogger{t: t, task: "priv-read-err"}} + + build.PrivateGerritClient = task.NewFakeGerrit(t, task.NewFakeRepo(t, "empty")) + + _, err := build.publicizePrivateSecurityCLs(taskCtx, + "go1.26.1", "release-branch.go1.26", base, "anything", nil) + if err == nil { + t.Fatal("expected error for missing private branch") + } + if !strings.Contains(err.Error(), "reading private branch head") { + t.Errorf("error = %v, want mention of reading private branch head", err) + } + }) + + t.Run("empty_security_commit_no_error", func(t *testing.T) { + build, _, _, base, _ := setup(t) + taskCtx := &workflow.TaskContext{Context: context.Background(), Logger: &testLogger{t: t, task: "pub-noop"}} + + cls, err := build.publicizePrivateSecurityCLs(taskCtx, + "go1.26.1", "release-branch.go1.26", base, "", nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(cls) != 0 { + t.Errorf("got %d CL IDs, want 0", len(cls)) + } + }) +} + +func TestMoveAndRebaseRebaseSuccess(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + taskCtx := &workflow.TaskContext{Context: ctx, Logger: &testLogger{t: t, task: "rebase-success"}} + + pubRepo := task.NewFakeRepo(t, "go") + base := pubRepo.Commit(map[string]string{"README": "hello"}) + pubRepo.Branch("public", base) + + privGerrit := task.NewFakeGerrit(t, pubRepo) + + privGerrit.AddChange("go", "rebase-cl", &gerrit.ChangeInfo{ + ID: "rebase-cl", + ChangeID: "rebase-cl", + Branch: "public", + Submittable: true, + Mergeable: true, + }, "test: rebase target") + + pubRepo.CommitOnBranch("public", map[string]string{"advance.txt": "advance"}) + + newHead, err := privGerrit.ReadBranchHead(ctx, "go", "public") + if err != nil { + t.Fatal(err) + } + if _, err := privGerrit.CreateBranch(ctx, "go", "checkpoint-rebase-test", gerrit.BranchInput{Revision: newHead}); err != nil { + t.Fatal(err) + } + + build := &BuildReleaseTasks{ + PrivateGerritClient: privGerrit, + PrivateGerritProject: "go", + } + + ci, err := privGerrit.GetChange(ctx, "rebase-cl") + if err != nil { + t.Fatal(err) + } + + moved, err := build.moveAndRebasePrivateChanges(taskCtx, "checkpoint-rebase-test", []*gerrit.ChangeInfo{ci}) + if err != nil { + t.Fatalf("moveAndRebasePrivateChanges: %v", err) + } + if len(moved) != 1 { + t.Fatalf("got %d CLs, want 1", len(moved)) + } + if moved[0].Branch != "checkpoint-rebase-test" { + t.Errorf("CL branch = %q, want %q", moved[0].Branch, "checkpoint-rebase-test") + } +}
diff --git a/internal/task/fakes.go b/internal/task/fakes.go index adfba36..ad70ce1 100644 --- a/internal/task/fakes.go +++ b/internal/task/fakes.go
@@ -116,10 +116,12 @@ commitMessages: make(map[string]string), clProjects: make(map[string]string), clBases: make(map[string]string), + changeKeys: make(map[string]string), } for _, r := range repos { result.repos[r.name] = r } + mux := http.NewServeMux() mux.HandleFunc("GET /a/{repo}/+archive/{archive}", result.serveArchive) // Serve a revision tarball (.tar.gz) like Gerrit does. mux.HandleFunc("GET /a/{repo}/+/{rev}/{path...}", result.serveGitiles) @@ -129,7 +131,6 @@ server := httptest.NewServer(mux) result.serverURL = server.URL t.Cleanup(server.Close) - return result } @@ -144,6 +145,7 @@ commitMessages map[string]string // CL ID → commit message. clProjects map[string]string // CL ID → project name. clBases map[string]string + changeKeys map[string]string nextCL int // Counter for generated cherry-pick CL IDs. LastReviewers []string @@ -537,8 +539,12 @@ } g.commitMessages[id] = commitMsg g.clProjects[id] = project - if ci != nil && ci.Branch != "" { - if repo, err := g.repo(project); err == nil { + if ci != nil { + repo, err := g.repo(project) + if held, ok := g.indexChange(id); !ok && err == nil { + repo.t.Fatalf("FakeGerrit.AddChange: change %q duplicates Change-Id %s of change %q on %s/%s", id, ci.ChangeID, held, project, ci.Branch) + } + if err == nil && ci.Branch != "" { if head, err := repo.dir.RunCommand(context.Background(), "rev-parse", ci.Branch); err == nil { g.clBases[id] = strings.TrimSpace(string(head)) } @@ -546,6 +552,28 @@ } } +func (g *FakeGerrit) changeKey(project, branch, changeID string) string { + return project + "~" + branch + "~" + changeID +} + +func (g *FakeGerrit) indexChange(id string) (held string, ok bool) { + ci := g.cls[id] + if ci.ChangeID == "" || ci.Branch == "" { + return "", true + } + key := g.changeKey(g.clProjects[id], ci.Branch, ci.ChangeID) + if held, exists := g.changeKeys[key]; exists && held != id { + return held, false + } + for k, v := range g.changeKeys { + if v == id { + delete(g.changeKeys, k) + } + } + g.changeKeys[key] = id + return "", true +} + func (g *FakeGerrit) Submitted(ctx context.Context, changeID, baseCommit string) (string, bool, error) { g.changesMu.Lock() commit, ok := g.changes[changeID] @@ -843,6 +871,17 @@ if !ok { return gerrit.ChangeInfo{}, false, NewGerritHTTPError(http.StatusNotFound, fmt.Sprintf("change %s not found\n", changeID)) } + project := g.clProjects[changeID] + for id, ci := range g.cls { + if ci.ChangeID != orig.ChangeID || ci.Branch != branch || g.clProjects[id] != project { + continue + } + if ci.Status == gerrit.ChangeStatusMerged || ci.Status == gerrit.ChangeStatusAbandoned { + return gerrit.ChangeInfo{}, false, NewGerritHTTPError(http.StatusBadRequest, fmt.Sprintf("Cherry-pick with Change-Id %s could not update the existing change %d in destination branch %s of project %s, because the change was closed (%s)\n", ci.ChangeID, ci.ChangeNumber, branch, project, ci.Status)) + } + g.commitMessages[id] = message + return *ci, ci.ContainsGitConflicts, nil + } g.nextCL++ cpID := fmt.Sprintf("cp-%d", g.nextCL) conflicts := orig.ContainsGitConflicts @@ -858,8 +897,8 @@ } g.cls[cpID] = cp g.commitMessages[cpID] = message - project := g.clProjects[changeID] g.clProjects[cpID] = project + g.indexChange(cpID) repo, err := g.repo(project) if err != nil { return gerrit.ChangeInfo{}, false, err @@ -885,7 +924,11 @@ if ci.Branch == branch { return gerrit.ChangeInfo{}, NewGerritHTTPError(http.StatusConflict, "Change is already destined for the specified branch\n") } + if held, exists := g.changeKeys[g.changeKey(g.clProjects[changeID], branch, ci.ChangeID)]; exists && held != changeID { + return gerrit.ChangeInfo{}, NewGerritHTTPError(http.StatusConflict, fmt.Sprintf("Destination %s has a different change with same change key %s\n", branch, ci.ChangeID)) + } ci.Branch = branch + g.indexChange(changeID) return *ci, nil }
diff --git a/internal/task/privx_test.go b/internal/task/privx_test.go index b67411b..863bfe4 100644 --- a/internal/task/privx_test.go +++ b/internal/task/privx_test.go
@@ -7,6 +7,7 @@ import ( "bytes" "context" + "errors" "fmt" "net/http" "net/mail" @@ -749,6 +750,278 @@ }) } +func TestMoveAndRebaseAllRebaseSuccess(t *testing.T) { + netRepo := NewFakeRepo(t, "net") + netHead := netRepo.History()[0] + netRepo.Branch("public", netHead) + + privGerrit := &fakePrivXGerrit{ + FakeGerrit: NewFakeGerrit(t, netRepo), + changes: map[string]*gerrit.ChangeInfo{}, + } + publicHead, err := netRepo.dir.RunCommand(context.Background(), "rev-parse", "public") + if err != nil { + t.Fatal(err) + } + + privGerrit.changes["1111"] = &gerrit.ChangeInfo{ + ID: "1111", + ChangeID: "1111", + Project: "net", + Branch: "public", + Submittable: true, + } + privGerrit.changesMu.Lock() + privGerrit.clBases["1111"] = strings.TrimSpace(string(publicHead)) + privGerrit.changesMu.Unlock() + + netRepo.CommitOnBranch("public", map[string]string{"advance.txt": "advance"}) + + newHead, err := netRepo.dir.RunCommand(context.Background(), "rev-parse", "public") + if err != nil { + t.Fatal(err) + } + newHeadStr := strings.TrimSpace(string(newHead)) + + netRepo.Branch("checkpoint-test", newHeadStr) + + privGerrit.changes["1111"].Branch = "checkpoint-test" + + ctx := &wf.TaskContext{Context: context.Background(), Logger: &testLogger{t: t}} + p := &PrivXPatch{PrivateGerrit: privGerrit} + + patches := []*ref{{ + Changes: []*gerrit.ChangeInfo{privGerrit.changes["1111"]}, + }} + + result, err := p.MoveAndRebaseAll(ctx, "checkpoint-test", patches) + if err != nil { + t.Fatalf("MoveAndRebaseAll: %v", err) + } + if len(result) != 1 || len(result[0].Changes) != 1 { + t.Fatalf("unexpected result shape: %v", result) + } +} + +func TestMoveAndRebaseAllMoveAlreadyDestined(t *testing.T) { + netRepo := NewFakeRepo(t, "net") + netHead := netRepo.History()[0] + netRepo.Branch("public", netHead) + netRepo.Branch("checkpoint-test", netHead) + + privGerrit := &fakePrivXGerrit{ + FakeGerrit: NewFakeGerrit(t, netRepo), + changes: map[string]*gerrit.ChangeInfo{}, + } + + privGerrit.changes["1111"] = &gerrit.ChangeInfo{ + ID: "1111", + ChangeID: "1111", + Project: "net", + Branch: "checkpoint-test", + Submittable: true, + } + publicHead, err := netRepo.dir.RunCommand(context.Background(), "rev-parse", "public") + if err != nil { + t.Fatal(err) + } + privGerrit.changesMu.Lock() + privGerrit.clBases["1111"] = strings.TrimSpace(string(publicHead)) + privGerrit.changesMu.Unlock() + + ctx := &wf.TaskContext{Context: context.Background(), Logger: &testLogger{t: t}} + p := &PrivXPatch{PrivateGerrit: privGerrit} + + patches := []*ref{{ + Changes: []*gerrit.ChangeInfo{privGerrit.changes["1111"]}, + }} + + result, err := p.MoveAndRebaseAll(ctx, "checkpoint-test", patches) + if err != nil { + t.Fatalf("MoveAndRebaseAll with already-destined CL: %v", err) + } + if len(result) != 1 || len(result[0].Changes) != 1 { + t.Fatalf("unexpected result shape: %v", result) + } +} + +func TestDeleteBranch409Refusal(t *testing.T) { + repo := NewFakeRepo(t, "test") + head := repo.History()[0] + repo.Branch("my-branch", head) + + fg := NewFakeGerrit(t, repo) + fg.AddChange("test", "open-cl", &gerrit.ChangeInfo{ + ID: "open-cl", + ChangeID: "open-cl", + Branch: "my-branch", + Status: "NEW", + Submittable: true, + }, "open change") + + ctx := context.Background() + err := fg.DeleteBranch(ctx, "test", "my-branch") + if err == nil { + t.Fatal("expected 409 error for branch with open CLs") + } + var httpErr *gerrit.HTTPError + if !errors.As(err, &httpErr) { + t.Fatalf("error type = %T, want *gerrit.HTTPError", err) + } + if httpErr.Res.StatusCode != http.StatusConflict { + t.Errorf("status = %d, want %d", httpErr.Res.StatusCode, http.StatusConflict) + } +} + +func TestCreateCherryPickChangeIDUniqueness(t *testing.T) { + repo := NewFakeRepo(t, "test") + head := repo.History()[0] + repo.Branch("dest", head) + repo.Branch("other-dest", head) + + fg := NewFakeGerrit(t, repo) + fg.AddChange("test", "orig", &gerrit.ChangeInfo{ + ID: "orig", + ChangeID: "I0123456789abcdef0123456789abcdef01234567", + Branch: "public", + Status: "NEW", + Submittable: true, + }, "fix\n\nChange-Id: I0123456789abcdef0123456789abcdef01234567\n") + + ctx := context.Background() + first, _, err := fg.CreateCherryPick(ctx, "orig", "dest", "msg") + if err != nil { + t.Fatalf("first CreateCherryPick: %v", err) + } + again, _, err := fg.CreateCherryPick(ctx, "orig", "dest", "msg2") + if err != nil { + t.Fatalf("CreateCherryPick onto open change: %v", err) + } + if again.ChangeNumber != first.ChangeNumber { + t.Errorf("open change: got CL %d, want new patch set on CL %d", again.ChangeNumber, first.ChangeNumber) + } + if got, _ := fg.GetCommitMessage(ctx, first.ID); got != "msg2" { + t.Errorf("open change message = %q, want %q", got, "msg2") + } + + if _, err := fg.SubmitChange(ctx, first.ID); err != nil { + t.Fatalf("SubmitChange: %v", err) + } + for _, status := range []string{gerrit.ChangeStatusMerged, gerrit.ChangeStatusAbandoned} { + ci, err := fg.GetChange(ctx, first.ID) + if err != nil { + t.Fatal(err) + } + ci.Status = status + _, _, err = fg.CreateCherryPick(ctx, "orig", "dest", "msg3") + if err == nil { + t.Fatalf("CreateCherryPick onto %s change: got nil error, want rejection", status) + } + var httpErr *gerrit.HTTPError + if !errors.As(err, &httpErr) { + t.Fatalf("error type = %T, want *gerrit.HTTPError", err) + } + if httpErr.Res.StatusCode != http.StatusBadRequest { + t.Errorf("%s: status = %d, want %d", status, httpErr.Res.StatusCode, http.StatusBadRequest) + } + } + + other, _, err := fg.CreateCherryPick(ctx, "orig", "other-dest", "msg") + if err != nil { + t.Fatalf("CreateCherryPick onto a different branch: %v", err) + } + if other.ChangeNumber == first.ChangeNumber { + t.Errorf("different branch reused CL %d", first.ChangeNumber) + } +} + +func TestMoveChangeDuplicateChangeID(t *testing.T) { + repo := NewFakeRepo(t, "test") + head := repo.History()[0] + repo.Branch("a", head) + repo.Branch("b", head) + + fg := NewFakeGerrit(t, repo) + const key = "I0123456789abcdef0123456789abcdef01234567" + fg.AddChange("test", "on-a", &gerrit.ChangeInfo{ID: "on-a", ChangeID: key, Branch: "a", Status: "NEW"}, "a") + fg.AddChange("test", "on-b", &gerrit.ChangeInfo{ID: "on-b", ChangeID: key, Branch: "b", Status: "NEW"}, "b") + + ctx := context.Background() + _, err := fg.MoveChange(ctx, "on-a", "b") + if err == nil { + t.Fatal("MoveChange onto a branch holding the same Change-Id: got nil error, want 409") + } + var httpErr *gerrit.HTTPError + if !errors.As(err, &httpErr) { + t.Fatalf("error type = %T, want *gerrit.HTTPError", err) + } + if httpErr.Res.StatusCode != http.StatusConflict { + t.Errorf("status = %d, want %d", httpErr.Res.StatusCode, http.StatusConflict) + } + if want := "Destination b has a different change with same change key " + key + "\n"; string(httpErr.Body) != want { + t.Errorf("body = %q, want %q", httpErr.Body, want) + } + + if _, err := fg.MoveChange(ctx, "on-a", "c"); err != nil { + t.Fatalf("MoveChange onto a free branch: %v", err) + } + if _, err := fg.MoveChange(ctx, "on-b", "a"); err != nil { + t.Fatalf("MoveChange onto the vacated branch: %v", err) + } +} + +func TestQueryChangesProjectFilter(t *testing.T) { + repoA := NewFakeRepo(t, "alpha") + repoB := NewFakeRepo(t, "beta") + fg := NewFakeGerrit(t, repoA, repoB) + + fg.AddChange("alpha", "a-cl", &gerrit.ChangeInfo{ + ID: "a-cl", + ChangeID: "a-cl", + Branch: "master", + Status: "NEW", + }, "change in alpha") + + fg.AddChange("beta", "b-cl", &gerrit.ChangeInfo{ + ID: "b-cl", + ChangeID: "b-cl", + Branch: "master", + Status: "NEW", + }, "change in beta") + + ctx := context.Background() + + alphaResults, err := fg.QueryChanges(ctx, "project:alpha branch:master") + if err != nil { + t.Fatal(err) + } + if len(alphaResults) != 1 { + t.Fatalf("project:alpha returned %d results, want 1", len(alphaResults)) + } + if alphaResults[0].ID != "a-cl" { + t.Errorf("project:alpha returned CL %q, want %q", alphaResults[0].ID, "a-cl") + } + + betaResults, err := fg.QueryChanges(ctx, "project:beta branch:master") + if err != nil { + t.Fatal(err) + } + if len(betaResults) != 1 { + t.Fatalf("project:beta returned %d results, want 1", len(betaResults)) + } + if betaResults[0].ID != "b-cl" { + t.Errorf("project:beta returned CL %q, want %q", betaResults[0].ID, "b-cl") + } + + allResults, err := fg.QueryChanges(ctx, "branch:master") + if err != nil { + t.Fatal(err) + } + if len(allResults) != 2 { + t.Fatalf("unfiltered query returned %d results, want 2", len(allResults)) + } +} + func TestRepoName(t *testing.T) { tests := []struct { name string
diff --git a/internal/task/vulndb_test.go b/internal/task/vulndb_test.go index 2fbbfb1..64e913e 100644 --- a/internal/task/vulndb_test.go +++ b/internal/task/vulndb_test.go
@@ -107,6 +107,11 @@ targets: []string{"go1"}, wantErr: true, }, + { + name: "non-numeric minor rejected", + targets: []string{"go1.2a.3"}, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -279,6 +284,18 @@ } }) + t.Run("empty summary after trim", func(t *testing.T) { + p := &relmeta.SecurityPatch{ + Package: "golang.org/x/net/http2", + GitHubIssueID: 12345, + Changelists: []string{"https://go.dev/cl/111"}, + ReleaseNote: "net/http2: .\n\nDetails.", + } + if _, err := VulnReport(p, mod, announceURL); err == nil { + t.Fatal("expected error for empty summary after trim") + } + }) + t.Run("non-ascii summary", func(t *testing.T) { p := &relmeta.SecurityPatch{ Package: "golang.org/x/net/http2", @@ -368,6 +385,11 @@ targets: []string{"go1"}, wantErr: true, }, + { + name: "non-numeric minor rejected", + targets: []string{"go1.2a.3"}, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -398,23 +420,70 @@ } func TestStdVulnModuleInfo(t *testing.T) { - p := &relmeta.SecurityPatch{ - Package: "net/http", - TargetReleases: []string{"go1.25.10", "go1.26.3"}, + tests := []struct { + name string + p *relmeta.SecurityPatch + wantMod string + wantVer string + wantVers report.Versions + wantErr bool + }{ + { + name: "std package", + p: &relmeta.SecurityPatch{ + Package: "net/http", + TargetReleases: []string{"go1.25.10", "go1.26.3"}, + }, + wantMod: "std", + wantVer: "1.26.2", + wantVers: report.Versions{report.Fixed("1.25.10"), report.Introduced("1.26.0-0"), report.Fixed("1.26.3")}, + }, + { + name: "cmd package", + p: &relmeta.SecurityPatch{ + Package: "cmd/go", + TargetReleases: []string{"go1.26.3"}, + }, + wantMod: "cmd", + wantVer: "1.26.2", + wantVers: report.Versions{report.Fixed("1.26.3")}, + }, + { + name: "invalid target releases", + p: &relmeta.SecurityPatch{ + Package: "net/http", + TargetReleases: []string{"not-a-version"}, + }, + wantErr: true, + }, + { + name: "empty target releases", + p: &relmeta.SecurityPatch{ + Package: "net/http", + TargetReleases: nil, + }, + wantErr: true, + }, } - mod, err := StdVulnModuleInfo(p) - if err != nil { - t.Fatal(err) - } - if mod.Module != "std" { - t.Errorf("got %q, want std", mod.Module) - } - if mod.VulnerableAt == nil || mod.VulnerableAt.Version != "1.26.2" { - t.Errorf("got %v, want 1.26.2", mod.VulnerableAt) - } - want := report.Versions{report.Fixed("1.25.10"), report.Introduced("1.26.0-0"), report.Fixed("1.26.3")} - if !reflect.DeepEqual(mod.Versions, want) { - t.Errorf("got %v, want %v", mod.Versions, want) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mod, err := StdVulnModuleInfo(tt.p) + if (err != nil) != tt.wantErr { + t.Fatalf("got %v, want %v", err, tt.wantErr) + } + if err != nil { + return + } + if mod.Module != tt.wantMod { + t.Errorf("Module = %q, want %q", mod.Module, tt.wantMod) + } + if mod.VulnerableAt == nil || mod.VulnerableAt.Version != tt.wantVer { + t.Errorf("VulnerableAt = %v, want %q", mod.VulnerableAt, tt.wantVer) + } + if !reflect.DeepEqual(mod.Versions, tt.wantVers) { + t.Errorf("Versions = %v, want %v", mod.Versions, tt.wantVers) + } + }) } } @@ -519,7 +588,6 @@ t.Error("expected no CL to be created when open CL exists") } }) - } func TestConvertInternalChangelists(t *testing.T) {