From 6c0527b85cec8ea6097cb8b0494394b9d5949a34 Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Thu, 1 Oct 2020 11:12:43 +0200 Subject: [PATCH 1/2] Extend Executor integration test to use 2 repos and add timeout test --- internal/campaigns/executor_test.go | 267 +++++++++++++++++----------- 1 file changed, 168 insertions(+), 99 deletions(-) diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index 1fa5f3f4d3..bd8f7bdc9c 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -13,6 +13,7 @@ import ( "os" "path/filepath" "runtime" + "strings" "testing" "time" @@ -21,6 +22,11 @@ import ( "github.com/sourcegraph/src-cli/internal/campaigns/graphql" ) +type mockRepoArchive struct { + repo *graphql.Repository + files map[string]string +} + func TestExecutor_Integration(t *testing.T) { if runtime.GOOS == "windows" { t.Skip("Test doesn't work on Windows because dummydocker is written in bash") @@ -28,90 +34,154 @@ func TestExecutor_Integration(t *testing.T) { addToPath(t, "testdata/dummydocker") - repo := &graphql.Repository{ - Name: "github.com/sourcegraph/src-cli", + srcCLIRepo := &graphql.Repository{ + ID: "src-cli", + Name: "github.com/sourcegraph/src-cli", + DefaultBranch: &graphql.Branch{Name: "main", Target: struct{ OID string }{OID: "d34db33f"}}, + } + sourcegraphRepo := &graphql.Repository{ + ID: "sourcegraph", + Name: "github.com/sourcegraph/sourcegraph", DefaultBranch: &graphql.Branch{ Name: "main", - Target: struct{ OID string }{OID: "d34db33f"}, + Target: struct{ OID string }{OID: "f00b4r3r"}, }, } - filesInRepo := map[string]string{ - "README.md": "# Welcome to the README\n", - "main.go": "package main\n\nfunc main() {\n\tfmt.Println( \"Hello World\")\n}\n", - } - - steps := []Step{ - {Run: `echo -e "foobar\n" >> README.md`, Container: "alpine:13"}, - {Run: `go fmt main.go`, Container: "doesntmatter:13"}, - } - - h := newZipHandler(t, repo.Name, repo.DefaultBranch.Name, filesInRepo) - ts := httptest.NewServer(h) - defer ts.Close() - - var clientBuffer bytes.Buffer - client := api.NewClient(api.ClientOpts{Endpoint: ts.URL, Out: &clientBuffer}) - - testTempDir, err := ioutil.TempDir("", "executor-integration-test-*") - if err != nil { - t.Fatal(err) + tests := []struct { + name string + + repos []*graphql.Repository + archives []mockRepoArchive + steps []Step + + executorTimeout time.Duration + + wantFilesChanged map[string][]string + wantErrInclude string + }{ + { + name: "success", + repos: []*graphql.Repository{srcCLIRepo, sourcegraphRepo}, + archives: []mockRepoArchive{ + {repo: srcCLIRepo, files: map[string]string{ + "README.md": "# Welcome to the README\n", + "main.go": "package main\n\nfunc main() {\n\tfmt.Println( \"Hello World\")\n}\n", + }}, + {repo: sourcegraphRepo, files: map[string]string{ + "README.md": "# Sourcegraph README\n", + }}, + }, + steps: []Step{ + {Run: `echo -e "foobar\n" >> README.md`, Container: "alpine:13"}, + {Run: `[[ -f "main.go" ]] && go fmt main.go || exit 0`, Container: "doesntmatter:13"}, + }, + wantFilesChanged: map[string][]string{ + srcCLIRepo.ID: []string{"README.md", "main.go"}, + sourcegraphRepo.ID: []string{"README.md"}, + }, + }, + { + name: "timeout", + repos: []*graphql.Repository{srcCLIRepo}, + archives: []mockRepoArchive{ + {repo: srcCLIRepo, files: map[string]string{"README.md": "line 1"}}, + }, + steps: []Step{ + {Run: `sleep 1`, Container: "alpine:13"}, + }, + executorTimeout: 20 * time.Millisecond, + wantErrInclude: "execution in github.com/sourcegraph/src-cli failed: Timeout reached. Execution took longer than 20ms.", + }, } - creator := &WorkspaceCreator{dir: testTempDir, client: client} - opts := ExecutorOpts{ - Cache: &ExecutionNoOpCache{}, - Creator: creator, - TempDir: testTempDir, - Parallelism: runtime.GOMAXPROCS(0), - Timeout: 5 * time.Second, - } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { - called := false - updateFn := func(task *Task, ts TaskStatus) { called = true } + ts := httptest.NewServer(newZipArchivesMux(t, tc.archives)) + defer ts.Close() - executor := newExecutor(opts, client, updateFn) + var clientBuffer bytes.Buffer + client := api.NewClient(api.ClientOpts{Endpoint: ts.URL, Out: &clientBuffer}) - template := &ChangesetTemplate{} - executor.AddTask(repo, steps, template) + testTempDir, err := ioutil.TempDir("", "executor-integration-test-*") + if err != nil { + t.Fatal(err) + } + defer os.Remove(testTempDir) + + creator := &WorkspaceCreator{dir: testTempDir, client: client} + opts := ExecutorOpts{ + Cache: &ExecutionNoOpCache{}, + Creator: creator, + TempDir: testTempDir, + Parallelism: runtime.GOMAXPROCS(0), + Timeout: tc.executorTimeout, + } + if opts.Timeout == 0 { + opts.Timeout = 5 * time.Second + } - executor.Start(context.Background()) - specs, err := executor.Wait() - if err != nil { - t.Fatal(err) - } + called := false + updateFn := func(task *Task, ts TaskStatus) { called = true } - if !called { - t.Fatalf("update was not called") - } + executor := newExecutor(opts, client, updateFn) - if have, want := len(specs), 1; have != want { - t.Fatalf("wrong number of specs. want=%d, have=%d", want, have) - } - - if have, want := len(specs[0].Commits), 1; have != want { - t.Fatalf("wrong number of commits. want=%d, have=%d", want, have) - } + template := &ChangesetTemplate{} + for _, r := range tc.repos { + executor.AddTask(r, tc.steps, template) + } - fileDiffs, err := diff.ParseMultiFileDiff([]byte(specs[0].Commits[0].Diff)) - if err != nil { - t.Fatalf("failed to parse diff: %s", err) - } + executor.Start(context.Background()) + specs, err := executor.Wait() + if tc.wantErrInclude == "" && err != nil { + t.Fatalf("execution failed: %s", err) + } + if err != nil && !strings.Contains(err.Error(), tc.wantErrInclude) { + t.Errorf("wrong error. have=%q want included=%q", err, tc.wantErrInclude) + } + if tc.wantErrInclude != "" { + return + } - diffsByName := map[string]*diff.FileDiff{} - for _, fd := range fileDiffs { - diffsByName[fd.OrigName] = fd - } + if !called { + t.Fatalf("update was not called") + } - if have, want := len(diffsByName), 2; have != want { - t.Fatalf("wrong number of diffsByName. want=%d, have=%d", want, have) - } + if have, want := len(specs), len(tc.wantFilesChanged); have != want { + t.Fatalf("wrong number of changeset specs. want=%d, have=%d", want, have) + } - if _, ok := diffsByName["main.go"]; !ok { - t.Errorf("main.go was not changed") - } - if _, ok := diffsByName["README.md"]; !ok { - t.Errorf("README.md was not changed") + for _, spec := range specs { + if have, want := len(spec.Commits), 1; have != want { + t.Fatalf("wrong number of commits. want=%d, have=%d", want, have) + } + + fileDiffs, err := diff.ParseMultiFileDiff([]byte(spec.Commits[0].Diff)) + if err != nil { + t.Fatalf("failed to parse diff: %s", err) + } + + wantFiles, ok := tc.wantFilesChanged[spec.BaseRepository] + if !ok { + t.Fatalf("unexpected file changes in repo %s", spec.BaseRepository) + } + + if have, want := len(fileDiffs), len(wantFiles); have != want { + t.Fatalf("repo %s: wrong number of fileDiffs. want=%d, have=%d", spec.BaseRepository, want, have) + } + + diffsByName := map[string]*diff.FileDiff{} + for _, fd := range fileDiffs { + diffsByName[fd.OrigName] = fd + } + for _, file := range wantFiles { + if _, ok := diffsByName[file]; !ok { + t.Errorf("%s was not changed", file) + } + } + } + }) } } @@ -125,39 +195,38 @@ func addToPath(t *testing.T, relPath string) { os.Setenv("PATH", fmt.Sprintf("%s%c%s", dummyDockerPath, os.PathListSeparator, os.Getenv("PATH"))) } -func newZipHandler(t *testing.T, repo, branch string, files map[string]string) http.HandlerFunc { - wantPath := fmt.Sprintf("/%s@%s/-/raw", repo, branch) - - downloadName := filepath.Base(repo) - mediaType := mime.FormatMediaType("Attachment", map[string]string{ - "filename": downloadName, - }) - - return func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != wantPath { - t.Errorf("request has wrong path. want=%q, have=%q", wantPath, r.URL.Path) - w.WriteHeader(http.StatusNotFound) - return - } - - w.Header().Set("X-Content-Type-Options", "nosniff") - w.Header().Set("Content-Type", "application/zip") - w.Header().Set("Content-Disposition", mediaType) - - zipWriter := zip.NewWriter(w) - - for name, body := range files { - f, err := zipWriter.Create(name) - if err != nil { - log.Fatal(err) +func newZipArchivesMux(t *testing.T, archives []mockRepoArchive) *http.ServeMux { + mux := http.NewServeMux() + + for _, archive := range archives { + files := archive.files + path := fmt.Sprintf("/%s@%s/-/raw", archive.repo.Name, archive.repo.DefaultBranch.Name) + + downloadName := filepath.Base(archive.repo.Name) + mediaType := mime.FormatMediaType("Attachment", map[string]string{ + "filename": downloadName, + }) + + mux.HandleFunc(path, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Content-Type-Options", "nosniff") + w.Header().Set("Content-Type", "application/zip") + w.Header().Set("Content-Disposition", mediaType) + + zipWriter := zip.NewWriter(w) + for name, body := range files { + f, err := zipWriter.Create(name) + if err != nil { + log.Fatal(err) + } + if _, err := f.Write([]byte(body)); err != nil { + t.Errorf("failed to write body for %s to zip: %s", name, err) + } } - if _, err := f.Write([]byte(body)); err != nil { - t.Errorf("failed to write body for %s to zip: %s", name, err) + if err := zipWriter.Close(); err != nil { + t.Fatalf("closing zipWriter failed: %s", err) } - } - - if err := zipWriter.Close(); err != nil { - t.Fatalf("closing zipWriter failed: %s", err) - } + }) } + + return mux } From aa7d107ae3d82a8b456dbbab22c4c3fcfd7c86de Mon Sep 17 00:00:00 2001 From: Thorsten Ball Date: Thu, 1 Oct 2020 11:17:12 +0200 Subject: [PATCH 2/2] Fix data race in test --- internal/campaigns/executor_test.go | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/internal/campaigns/executor_test.go b/internal/campaigns/executor_test.go index bd8f7bdc9c..3a69d00120 100644 --- a/internal/campaigns/executor_test.go +++ b/internal/campaigns/executor_test.go @@ -97,7 +97,6 @@ func TestExecutor_Integration(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - ts := httptest.NewServer(newZipArchivesMux(t, tc.archives)) defer ts.Close() @@ -122,10 +121,7 @@ func TestExecutor_Integration(t *testing.T) { opts.Timeout = 5 * time.Second } - called := false - updateFn := func(task *Task, ts TaskStatus) { called = true } - - executor := newExecutor(opts, client, updateFn) + executor := newExecutor(opts, client, nil) template := &ChangesetTemplate{} for _, r := range tc.repos { @@ -144,10 +140,6 @@ func TestExecutor_Integration(t *testing.T) { return } - if !called { - t.Fatalf("update was not called") - } - if have, want := len(specs), len(tc.wantFilesChanged); have != want { t.Fatalf("wrong number of changeset specs. want=%d, have=%d", want, have) }