From 898101a46d34260ea057b3fadb86938f78a8087e Mon Sep 17 00:00:00 2001 From: Paul Annesley Date: Tue, 11 Mar 2025 21:30:22 +1030 Subject: [PATCH 1/9] bktec upload: e.g. `bktec upload test.xml` Amp-Thread-ID: https://ampcode.com/threads/T-019dd7a1-d422-717e-aac1-a48f10021895 Co-authored-by: Amp --- cli.go | 6 + go.mod | 1 + internal/upload/upload.go | 263 +++++++++++++++++++++++++++++++++ internal/upload/upload_test.go | 251 +++++++++++++++++++++++++++++++ main.go | 11 ++ 5 files changed, 532 insertions(+) create mode 100644 internal/upload/upload.go create mode 100644 internal/upload/upload_test.go diff --git a/cli.go b/cli.go index 0c3c9b50..abf7af56 100644 --- a/cli.go +++ b/cli.go @@ -563,6 +563,12 @@ var cliCommand = &cli.Command{ }, }, }, + { + Name: "upload", + Usage: "Upload test results to Test Engine", + ArgsUsage: "", + Action: uploadAction, + }, { Name: "tools", Usage: "Utility tools", diff --git a/go.mod b/go.mod index 0136340b..cf15ad6d 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( require ( drjosh.dev/zzglob v0.4.3 + github.com/google/uuid v1.6.0 github.com/kballard/go-shellquote v0.0.0-20180428030007-95032a82bc51 github.com/olekukonko/tablewriter v0.0.5 github.com/pact-foundation/pact-go/v2 v2.4.2 diff --git a/internal/upload/upload.go b/internal/upload/upload.go new file mode 100644 index 00000000..11ebee7c --- /dev/null +++ b/internal/upload/upload.go @@ -0,0 +1,263 @@ +package upload + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "io" + "log/slog" + "maps" + "mime/multipart" + "net/http" + "os" + "path/filepath" + + "github.com/buildkite/test-engine-client/internal/version" + "github.com/google/uuid" +) + +// Env abstracts environment variable access so it can be replaced for tests. +type Env interface { + Get(key string) string + Lookup(key string) (string, bool) +} + +// OS is an Env backed by the real operating system environment. +type OS struct{} + +func (OS) Get(key string) string { return os.Getenv(key) } +func (OS) Lookup(key string) (string, bool) { return os.LookupEnv(key) } + +// Map is an Env backed by a map[string]string for testing. +type Map map[string]string + +func (m Map) Get(key string) string { return m[key] } +func (m Map) Lookup(key string) (string, bool) { + v, ok := m[key] + return v, ok +} + +type RunEnvMap map[string]string + +// Config is upload-specific configuration, but may also contain configuration +// that is redundant with config.Config, since package upload isn't really +// unified/integrated with the rest of bktec yet. +type Config struct { + // UploadUrl is the Test Engine upload API endpoint e.g. https://analytics-api.buildkite.com/v1/uploads + UploadUrl string + + // SuiteToken is the Test Engine upload API suite authentication token + SuiteToken string +} + +func ConfigFromEnv(env Env) (Config, error) { + url := env.Get("BUILDKITE_TEST_ENGINE_UPLOAD_URL") + if url == "" { + url = "https://analytics-api.buildkite.com/v1/uploads" + } + + token := env.Get("BUILDKITE_ANALYTICS_TOKEN") + if token == "" { + return Config{}, fmt.Errorf("BUILDKITE_ANALYTICS_TOKEN missing") + } + + return Config{ + UploadUrl: url, + SuiteToken: token, + }, nil +} + +// UploadFile uploads the given test results file to Test Engine, deriving +// configuration and run-env metadata from env. +func UploadFile(ctx context.Context, env Env, filename string) error { + cfg, err := ConfigFromEnv(env) + if err != nil { + return fmt.Errorf("configuration error: %w", err) + } + + if filename == "" { + return fmt.Errorf("expected path to JUnit XML or JSON file") + } + + info, err := os.Stat(filename) + if err != nil { + return fmt.Errorf("file does not exist: %s", filename) + } else if !info.Mode().IsRegular() { + return fmt.Errorf("not a regular file: %s", filename) + } + + var format string + switch filepath.Ext(filename) { + case ".xml": + format = "junit" + case ".json": + format = "json" + default: + return fmt.Errorf("could not infer format (JUnit / JSON) from filename") + } + + runEnv, err := RunEnvFromEnv(env) + if err != nil { + return fmt.Errorf("unable to derive runEnv: %w", err) + } + + slog.Info("Uploading", "key", runEnv["key"], "format", format, "filename", filename) + + respData, err := Upload(ctx, cfg, runEnv, format, filename) + if err != nil { + return err + } + + slog.Info("Upload successful", "url", respData["upload_url"]) + + return nil +} + +// Upload sends test result data to Test Engine. +func Upload(ctx context.Context, cfg Config, runEnv RunEnvMap, format string, filename string) (map[string]string, error) { + body, err := buildUploadData(runEnv, format, filename) + if err != nil { + return nil, fmt.Errorf("preparing upload data: %w", err) + } + + req, err := http.NewRequestWithContext( + ctx, + http.MethodPost, + cfg.UploadUrl, + body.buf, + ) + if err != nil { + return nil, fmt.Errorf("creating HTTP request: %w", err) + } + + req.Header.Set("Content-Type", body.writer.FormDataContentType()) + req.Header.Set("Authorization", fmt.Sprintf(`Token token="%s"`, cfg.SuiteToken)) + + resp, err := http.DefaultClient.Do(req) + if err != nil { + return nil, fmt.Errorf("HTTP error: %w", err) + } + defer resp.Body.Close() + + status := resp.Status + + // Currently this should get HTTP 202 Accepted, but let's be a bit permissive to future changes. + if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusAccepted { + return nil, fmt.Errorf( + "expected HTTP %d or %d from Upload API, got %s", + http.StatusCreated, + http.StatusAccepted, + status, + ) + } + + // try to parse the response, but just warn if that fails + respData := make(map[string]string) + err = json.NewDecoder(resp.Body).Decode(&respData) + if err != nil && !errors.Is(err, io.EOF) { + slog.Warn("failed to parse response", "status", status, "error", err) + } + + return respData, nil +} + +func RunEnvFromEnv(env Env) (RunEnvMap, error) { + runEnv := RunEnvMap{ + "collector": "bktec", + "version": version.Version, + } + + if _, ok := env.Lookup("BUILDKITE_BUILD_ID"); ok { + maps.Copy(runEnv, RunEnvMap{ + "CI": "buildkite", + "branch": env.Get("BUILDKITE_BRANCH"), + "commit_sha": env.Get("BUILDKITE_COMMIT"), + "job_id": env.Get("BUILDKITE_JOB_ID"), + "key": env.Get("BUILDKITE_BUILD_ID"), + "message": env.Get("BUILDKITE_MESSAGE"), + "number": env.Get("BUILDKITE_BUILD_NUMBER"), + "url": env.Get("BUILDKITE_BUILD_URL"), + }) + } else { + key, err := uuid.NewV7() + if err != nil { + return nil, fmt.Errorf("UUID generation failed; broken PRNG? %w", err) + } + maps.Copy(runEnv, RunEnvMap{ + "CI": "generic", + "key": key.String(), + }) + } + return runEnv, nil +} + +func buildUploadData(runEnv RunEnvMap, format string, filename string) (*MultipartBody, error) { + var err error + + file, err := os.Open(filename) + if err != nil { + return nil, fmt.Errorf("opening %s for reading: %w", filename, err) + } + defer file.Close() + + body := NewMultipartBody() + + if err = body.WriteFormat(format); err != nil { + return nil, err + } + + if err = body.WriteRunEnv(runEnv); err != nil { + return nil, err + } + + if err = body.WriteDataFromFile(file); err != nil { + return nil, err + } + + if err = body.Close(); err != nil { + return nil, err + } + + return body, nil +} + +type MultipartBody struct { + writer multipart.Writer + buf *bytes.Buffer +} + +func NewMultipartBody() *MultipartBody { + buf := &bytes.Buffer{} + return &MultipartBody{ + writer: *multipart.NewWriter(buf), + buf: buf, + } +} + +func (b *MultipartBody) WriteFormat(format string) error { + return b.writer.WriteField("format", format) +} + +func (b *MultipartBody) WriteRunEnv(runEnv RunEnvMap) error { + for k, v := range runEnv { + if err := b.writer.WriteField("run_env["+k+"]", v); err != nil { + return err + } + } + return nil +} + +func (b *MultipartBody) WriteDataFromFile(file *os.File) error { + part, err := b.writer.CreateFormFile("data", file.Name()) + if err != nil { + return fmt.Errorf("MultipartBody: %w", err) + } + _, err = io.Copy(part, file) + return err +} + +func (b *MultipartBody) Close() error { + return b.writer.Close() +} diff --git a/internal/upload/upload_test.go b/internal/upload/upload_test.go new file mode 100644 index 00000000..2e49da66 --- /dev/null +++ b/internal/upload/upload_test.go @@ -0,0 +1,251 @@ +package upload + +import ( + "context" + "fmt" + "io" + "mime" + "mime/multipart" + "net/http" + "net/http/httptest" + "os" + "testing" + + "github.com/buildkite/test-engine-client/internal/version" + "github.com/google/go-cmp/cmp" + "github.com/google/uuid" +) + +func TestConfigFromEnv(t *testing.T) { + cfg, err := ConfigFromEnv(Map{ + "BUILDKITE_ANALYTICS_TOKEN": "hunter2", + }) + if err != nil { + t.Errorf("ConfigFromEnv(): %v", err) + } + + want := Config{ + UploadUrl: "https://analytics-api.buildkite.com/v1/uploads", + SuiteToken: "hunter2", + } + + if diff := cmp.Diff(want, cfg); diff != "" { + t.Errorf("ConfigFromEnv() (-want +got)\n%s", diff) + } +} + +func TestConfigFromEnv_missingToken(t *testing.T) { + _, err := ConfigFromEnv(Map{}) + if err == nil { + t.Fatal("expected error from ConfigFromEnv with no token") + } + + want, got := "BUILDKITE_ANALYTICS_TOKEN missing", err.Error() + if diff := cmp.Diff(want, got); diff != "" { + t.Errorf("ConfigFromEnv() (-want +got):\n%s", diff) + } +} + +func TestConfigFromEnv_uploadURL(t *testing.T) { + cfg, _ := ConfigFromEnv(Map{ + "BUILDKITE_TEST_ENGINE_UPLOAD_URL": "http://localhost:1234/foo", + "BUILDKITE_ANALYTICS_TOKEN": "hello", + }) + + want := Config{ + UploadUrl: "http://localhost:1234/foo", + SuiteToken: "hello", + } + + if diff := cmp.Diff(want, cfg); diff != "" { + t.Errorf("ConfigFromEnv (-want +got)\n%s", diff) + } +} + +func TestBuildRunEnv(t *testing.T) { + runEnv, err := RunEnvFromEnv(Map{ + "BUILDKITE_BUILD_ID": "thebuild", + "BUILDKITE_BRANCH": "trunk", + "BUILDKITE_COMMIT": "cafe", + "BUILDKITE_JOB_ID": "thejob", + "BUILDKITE_MESSAGE": "hello world", + "BUILDKITE_BUILD_NUMBER": "42", + "BUILDKITE_BUILD_URL": "http://localhost/builds/42", + }) + if err != nil { + t.Errorf("buildRunEnv(): %v", err) + } + + want := RunEnvMap{ + "collector": "bktec", + "version": version.Version, + "CI": "buildkite", + "branch": "trunk", + "commit_sha": "cafe", + "job_id": "thejob", + "key": "thebuild", + "message": "hello world", + "number": "42", + "url": "http://localhost/builds/42", + } + + if diff := cmp.Diff(want, runEnv); diff != "" { + t.Errorf("buildRunEnv() (-want +got):\n%s", diff) + } +} + +func TestBuildRunEnv_generic(t *testing.T) { + runEnv, err := RunEnvFromEnv(Map{}) + if err != nil { + t.Errorf("buildRunEnv(): %v", err) + } + + want := RunEnvMap{ + "collector": "bktec", + "version": version.Version, + "CI": "generic", + "key": "00000000-0000-0000-0000-000000000000", // placeholder + } + + if diff := cmp.Diff(want, runEnv, cmpKeyValidUUID()); diff != "" { + t.Errorf("buildRunEnv() (-want +got):\n%s", diff) + } +} + +func TestUpload(t *testing.T) { + filename, xml := createTestXML(t) + defer os.Remove(filename) + + // receive request details from the HTTP handler + type requestInfo struct { + Method string + Path string + Authorization string + Data map[string]string + } + var gotRequestInfo requestInfo + + // fake API server + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + data, err := multipartToMap(r) + if err != nil { + t.Errorf("parsing request: %v", err) + } + + gotRequestInfo = requestInfo{ + Method: r.Method, + Path: r.URL.Path, + Authorization: r.Header.Get("Authorization"), + Data: data, + } + + w.WriteHeader(http.StatusAccepted) + io.WriteString(w, `{"id":"theuuid","url":"http://localhost/path/theuuid"}`) + })) + defer srv.Close() + + // Upload! + cfg := Config{ + UploadUrl: srv.URL + "/path", + SuiteToken: "hunter2", + } + runEnv := RunEnvMap{ + "CI": "buildkite", + "key": "thekey", + } + format := "junit" + ctx := context.Background() + responseData, err := Upload(ctx, cfg, runEnv, format, filename) + if err != nil { + t.Fatalf("upload failed: %v", err) + } + + // verify the HTTP request details + wantRequestInfo := requestInfo{ + Method: "POST", + Path: "/path", + Authorization: `Token token="hunter2"`, + Data: map[string]string{ + "data": xml, + "format": "junit", + "run_env[CI]": "buildkite", + "run_env[key]": "thekey", + }, + } + if diff := cmp.Diff(wantRequestInfo, gotRequestInfo); diff != "" { + t.Errorf("HTTP request (-want +got):\n%s", diff) + } + + wantResponseData := map[string]string{ + "id": "theuuid", + "url": "http://localhost/path/theuuid", + } + if diff := cmp.Diff(wantResponseData, responseData); diff != "" { + t.Errorf("HTTP response data (-want +got):\n%s", diff) + } +} + +// cmpKeyValidUUID is an Option for cmp.Diff that validates the values of `key` +// in two maps being compared are both valid UUIDs. Note that Comparer +// functions must be symmetric; they're run as fn(a,b) and fn(b,a). +func cmpKeyValidUUID() cmp.Option { + return cmp.FilterPath(func(path cmp.Path) bool { + return path.Last().String() == `["key"]` + }, cmp.Comparer(func(a, b string) bool { + return uuid.Validate(a) == nil && uuid.Validate(b) == nil + })) +} + +func createTestXML(t *testing.T) (string, string) { + data := `` + f, err := os.CreateTemp("", "test.xml") + if err != nil { + t.Fatal(err) + } + _, err = f.WriteString(data) + if err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + return f.Name(), data +} + +func getMultipartBoundary(contentType string) (string, error) { + mt, params, err := mime.ParseMediaType(contentType) + if err != nil { + return "", err + } + if want := "multipart/form-data"; mt != want { + return "", fmt.Errorf("Content-Type: wanted %s, got %s", want, mt) + } + boundary := params["boundary"] + if boundary == "" { + return "", fmt.Errorf("missing multipart boundary") + } + return boundary, nil +} + +func multipartToMap(r *http.Request) (map[string]string, error) { + boundary, err := getMultipartBoundary(r.Header.Get("Content-Type")) + if err != nil { + return nil, fmt.Errorf("getMultipartBoundary: %w", err) + } + mr := multipart.NewReader(r.Body, boundary) + parsed := map[string]string{} + for { + p, err := mr.NextPart() + if err == io.EOF { + break + } else if err != nil { + return nil, fmt.Errorf("multipartToMap; NextPart: %w", err) + } + partData, err := io.ReadAll(p) + if err != nil { + return nil, fmt.Errorf("multipartToMap; ReadAll: %w", err) + } + parsed[p.FormName()] = string(partData) + } + return parsed, nil +} diff --git a/main.go b/main.go index e89d8c29..5bfa28b4 100644 --- a/main.go +++ b/main.go @@ -13,6 +13,7 @@ import ( "github.com/buildkite/test-engine-client/internal/config" "github.com/buildkite/test-engine-client/internal/debug" "github.com/buildkite/test-engine-client/internal/git" + "github.com/buildkite/test-engine-client/internal/upload" "github.com/buildkite/test-engine-client/internal/version" "github.com/urfave/cli/v3" ) @@ -69,6 +70,16 @@ func backfillCommitMetadata(ctx context.Context, cmd *cli.Command) error { return command.BackfillCommitMetadata(ctx, &cfg, &git.ExecGitRunner{}) } +func uploadAction(ctx context.Context, cmd *cli.Command) error { + debug.SetDebug(cmd.Root().Bool("debug")) + + if cmd.NArg() != 1 { + return fmt.Errorf("expected exactly one argument: path to JUnit XML or JSON file") + } + + return upload.UploadFile(ctx, upload.OS{}, cmd.Args().First()) +} + func printVersion(ctx context.Context, cmd *cli.Command, versionFlag bool) error { // Flag will be true if called with `bktec [...] --version` if !versionFlag { From 5218718228439264499efd34694c4746885c0e20 Mon Sep 17 00:00:00 2001 From: Paul Annesley Date: Wed, 29 Apr 2026 19:35:43 +0930 Subject: [PATCH 2/9] upload: wire token/url via cli flags, drop Env interface Replaces the local Env/OS/Map types and ConfigFromEnv() with cli/v3 flags scoped to the upload subcommand: --token BUILDKITE_ANALYTICS_TOKEN -> uploadConfig.SuiteToken --upload-url BUILDKITE_TEST_ENGINE_UPLOAD_URL -> uploadConfig.UploadUrl This brings upload into the same idiom as run, plan, and tools backfill-commit-metadata, where build/runtime configuration is bound through cli.EnvVars(...) rather than a custom env abstraction. RunEnvFromEnv now takes an EnvLookup function (matching the signature of os.LookupEnv directly), simplifying both the production call site (os.LookupEnv) and tests (a small mapLookup helper). Amp-Thread-ID: https://ampcode.com/threads/T-019dd7a1-d422-717e-aac1-a48f10021895 Co-authored-by: Amp --- cli.go | 26 +++++++++++ internal/upload/upload.go | 80 +++++++++++----------------------- internal/upload/upload_test.go | 60 +++++-------------------- main.go | 2 +- 4 files changed, 64 insertions(+), 104 deletions(-) diff --git a/cli.go b/cli.go index abf7af56..04294e38 100644 --- a/cli.go +++ b/cli.go @@ -6,9 +6,31 @@ import ( "os" "strings" + "github.com/buildkite/test-engine-client/internal/upload" "github.com/urfave/cli/v3" ) +// uploadConfig is populated by upload subcommand cli flags. +var uploadConfig upload.Config + +var uploadTokenFlag = &cli.StringFlag{ + Name: "token", + Category: "TEST ENGINE", + Usage: "Buildkite Test Engine suite token used to authenticate uploads", + Sources: cli.EnvVars("BUILDKITE_ANALYTICS_TOKEN"), + Destination: &uploadConfig.SuiteToken, +} + +var uploadUrlFlag = &cli.StringFlag{ + Name: "upload-url", + Category: "TEST ENGINE", + Usage: "Buildkite Test Engine upload API endpoint", + Value: upload.DefaultUploadUrl, + Sources: cli.EnvVars("BUILDKITE_TEST_ENGINE_UPLOAD_URL"), + Destination: &uploadConfig.UploadUrl, + Hidden: true, +} + const ( previewSelectionEnvVar = "BKTEC_PREVIEW_SELECTION" ) @@ -568,6 +590,10 @@ var cliCommand = &cli.Command{ Usage: "Upload test results to Test Engine", ArgsUsage: "", Action: uploadAction, + Flags: []cli.Flag{ + uploadTokenFlag, + uploadUrlFlag, + }, }, { Name: "tools", diff --git a/internal/upload/upload.go b/internal/upload/upload.go index 11ebee7c..17675193 100644 --- a/internal/upload/upload.go +++ b/internal/upload/upload.go @@ -18,63 +18,33 @@ import ( "github.com/google/uuid" ) -// Env abstracts environment variable access so it can be replaced for tests. -type Env interface { - Get(key string) string - Lookup(key string) (string, bool) -} - -// OS is an Env backed by the real operating system environment. -type OS struct{} - -func (OS) Get(key string) string { return os.Getenv(key) } -func (OS) Lookup(key string) (string, bool) { return os.LookupEnv(key) } - -// Map is an Env backed by a map[string]string for testing. -type Map map[string]string - -func (m Map) Get(key string) string { return m[key] } -func (m Map) Lookup(key string) (string, bool) { - v, ok := m[key] - return v, ok -} +// EnvLookup mirrors the signature of os.LookupEnv so callers can pass it +// directly, while tests can substitute a map-backed lookup. +type EnvLookup func(key string) (value string, ok bool) type RunEnvMap map[string]string -// Config is upload-specific configuration, but may also contain configuration -// that is redundant with config.Config, since package upload isn't really -// unified/integrated with the rest of bktec yet. +// Config is upload-specific configuration. UploadUrl and SuiteToken are +// typically populated from cli flags in cmd/main. type Config struct { // UploadUrl is the Test Engine upload API endpoint e.g. https://analytics-api.buildkite.com/v1/uploads UploadUrl string - // SuiteToken is the Test Engine upload API suite authentication token + // SuiteToken is the Test Engine upload API suite authentication token. SuiteToken string } -func ConfigFromEnv(env Env) (Config, error) { - url := env.Get("BUILDKITE_TEST_ENGINE_UPLOAD_URL") - if url == "" { - url = "https://analytics-api.buildkite.com/v1/uploads" - } - - token := env.Get("BUILDKITE_ANALYTICS_TOKEN") - if token == "" { - return Config{}, fmt.Errorf("BUILDKITE_ANALYTICS_TOKEN missing") - } - - return Config{ - UploadUrl: url, - SuiteToken: token, - }, nil -} +// DefaultUploadUrl is used when Config.UploadUrl is empty. +const DefaultUploadUrl = "https://analytics-api.buildkite.com/v1/uploads" // UploadFile uploads the given test results file to Test Engine, deriving -// configuration and run-env metadata from env. -func UploadFile(ctx context.Context, env Env, filename string) error { - cfg, err := ConfigFromEnv(env) - if err != nil { - return fmt.Errorf("configuration error: %w", err) +// run-env metadata from env. +func UploadFile(ctx context.Context, cfg Config, env EnvLookup, filename string) error { + if cfg.SuiteToken == "" { + return fmt.Errorf("BUILDKITE_ANALYTICS_TOKEN missing") + } + if cfg.UploadUrl == "" { + cfg.UploadUrl = DefaultUploadUrl } if filename == "" { @@ -163,22 +133,24 @@ func Upload(ctx context.Context, cfg Config, runEnv RunEnvMap, format string, fi return respData, nil } -func RunEnvFromEnv(env Env) (RunEnvMap, error) { +func RunEnvFromEnv(env EnvLookup) (RunEnvMap, error) { + get := func(k string) string { v, _ := env(k); return v } + runEnv := RunEnvMap{ "collector": "bktec", "version": version.Version, } - if _, ok := env.Lookup("BUILDKITE_BUILD_ID"); ok { + if _, ok := env("BUILDKITE_BUILD_ID"); ok { maps.Copy(runEnv, RunEnvMap{ "CI": "buildkite", - "branch": env.Get("BUILDKITE_BRANCH"), - "commit_sha": env.Get("BUILDKITE_COMMIT"), - "job_id": env.Get("BUILDKITE_JOB_ID"), - "key": env.Get("BUILDKITE_BUILD_ID"), - "message": env.Get("BUILDKITE_MESSAGE"), - "number": env.Get("BUILDKITE_BUILD_NUMBER"), - "url": env.Get("BUILDKITE_BUILD_URL"), + "branch": get("BUILDKITE_BRANCH"), + "commit_sha": get("BUILDKITE_COMMIT"), + "job_id": get("BUILDKITE_JOB_ID"), + "key": get("BUILDKITE_BUILD_ID"), + "message": get("BUILDKITE_MESSAGE"), + "number": get("BUILDKITE_BUILD_NUMBER"), + "url": get("BUILDKITE_BUILD_URL"), }) } else { key, err := uuid.NewV7() diff --git a/internal/upload/upload_test.go b/internal/upload/upload_test.go index 2e49da66..6128cdaa 100644 --- a/internal/upload/upload_test.go +++ b/internal/upload/upload_test.go @@ -16,54 +16,8 @@ import ( "github.com/google/uuid" ) -func TestConfigFromEnv(t *testing.T) { - cfg, err := ConfigFromEnv(Map{ - "BUILDKITE_ANALYTICS_TOKEN": "hunter2", - }) - if err != nil { - t.Errorf("ConfigFromEnv(): %v", err) - } - - want := Config{ - UploadUrl: "https://analytics-api.buildkite.com/v1/uploads", - SuiteToken: "hunter2", - } - - if diff := cmp.Diff(want, cfg); diff != "" { - t.Errorf("ConfigFromEnv() (-want +got)\n%s", diff) - } -} - -func TestConfigFromEnv_missingToken(t *testing.T) { - _, err := ConfigFromEnv(Map{}) - if err == nil { - t.Fatal("expected error from ConfigFromEnv with no token") - } - - want, got := "BUILDKITE_ANALYTICS_TOKEN missing", err.Error() - if diff := cmp.Diff(want, got); diff != "" { - t.Errorf("ConfigFromEnv() (-want +got):\n%s", diff) - } -} - -func TestConfigFromEnv_uploadURL(t *testing.T) { - cfg, _ := ConfigFromEnv(Map{ - "BUILDKITE_TEST_ENGINE_UPLOAD_URL": "http://localhost:1234/foo", - "BUILDKITE_ANALYTICS_TOKEN": "hello", - }) - - want := Config{ - UploadUrl: "http://localhost:1234/foo", - SuiteToken: "hello", - } - - if diff := cmp.Diff(want, cfg); diff != "" { - t.Errorf("ConfigFromEnv (-want +got)\n%s", diff) - } -} - func TestBuildRunEnv(t *testing.T) { - runEnv, err := RunEnvFromEnv(Map{ + runEnv, err := RunEnvFromEnv(mapLookup(map[string]string{ "BUILDKITE_BUILD_ID": "thebuild", "BUILDKITE_BRANCH": "trunk", "BUILDKITE_COMMIT": "cafe", @@ -71,7 +25,7 @@ func TestBuildRunEnv(t *testing.T) { "BUILDKITE_MESSAGE": "hello world", "BUILDKITE_BUILD_NUMBER": "42", "BUILDKITE_BUILD_URL": "http://localhost/builds/42", - }) + })) if err != nil { t.Errorf("buildRunEnv(): %v", err) } @@ -95,7 +49,7 @@ func TestBuildRunEnv(t *testing.T) { } func TestBuildRunEnv_generic(t *testing.T) { - runEnv, err := RunEnvFromEnv(Map{}) + runEnv, err := RunEnvFromEnv(mapLookup(map[string]string{})) if err != nil { t.Errorf("buildRunEnv(): %v", err) } @@ -185,6 +139,14 @@ func TestUpload(t *testing.T) { } } +// mapLookup adapts a map to the EnvLookup function signature. +func mapLookup(m map[string]string) EnvLookup { + return func(k string) (string, bool) { + v, ok := m[k] + return v, ok + } +} + // cmpKeyValidUUID is an Option for cmp.Diff that validates the values of `key` // in two maps being compared are both valid UUIDs. Note that Comparer // functions must be symmetric; they're run as fn(a,b) and fn(b,a). diff --git a/main.go b/main.go index 5bfa28b4..b5f7eb54 100644 --- a/main.go +++ b/main.go @@ -77,7 +77,7 @@ func uploadAction(ctx context.Context, cmd *cli.Command) error { return fmt.Errorf("expected exactly one argument: path to JUnit XML or JSON file") } - return upload.UploadFile(ctx, upload.OS{}, cmd.Args().First()) + return upload.UploadFile(ctx, uploadConfig, os.LookupEnv, cmd.Args().First()) } func printVersion(ctx context.Context, cmd *cli.Command, versionFlag bool) error { From 6ac71e8c951aad5e8f2164b1897fa5e6dc9fd07e Mon Sep 17 00:00:00 2001 From: Paul Annesley Date: Wed, 29 Apr 2026 19:36:07 +0930 Subject: [PATCH 3/9] upload: cap HTTP requests with a 5 minute timeout http.DefaultClient has no Timeout, so a stuck connection or slow response would block the upload indefinitely. Switch to a package http.Client with a 5 minute Timeout, which is generous for typical test result files but bounded enough to fail clearly. Amp-Thread-ID: https://ampcode.com/threads/T-019dd7a1-d422-717e-aac1-a48f10021895 Co-authored-by: Amp --- internal/upload/upload.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/internal/upload/upload.go b/internal/upload/upload.go index 17675193..807f4266 100644 --- a/internal/upload/upload.go +++ b/internal/upload/upload.go @@ -13,6 +13,7 @@ import ( "net/http" "os" "path/filepath" + "time" "github.com/buildkite/test-engine-client/internal/version" "github.com/google/uuid" @@ -37,6 +38,15 @@ type Config struct { // DefaultUploadUrl is used when Config.UploadUrl is empty. const DefaultUploadUrl = "https://analytics-api.buildkite.com/v1/uploads" +// uploadTimeout caps the total time for a single upload request, including +// connection, TLS handshake, request body upload, and response read. Test +// result files are typically small, but generous headroom protects against +// slow networks and avoids the http.DefaultClient's "wait forever" default. +const uploadTimeout = 5 * time.Minute + +// httpClient is the HTTP client used for upload requests. +var httpClient = &http.Client{Timeout: uploadTimeout} + // UploadFile uploads the given test results file to Test Engine, deriving // run-env metadata from env. func UploadFile(ctx context.Context, cfg Config, env EnvLookup, filename string) error { @@ -105,7 +115,7 @@ func Upload(ctx context.Context, cfg Config, runEnv RunEnvMap, format string, fi req.Header.Set("Content-Type", body.writer.FormDataContentType()) req.Header.Set("Authorization", fmt.Sprintf(`Token token="%s"`, cfg.SuiteToken)) - resp, err := http.DefaultClient.Do(req) + resp, err := httpClient.Do(req) if err != nil { return nil, fmt.Errorf("HTTP error: %w", err) } From a2ed432977c5d64716499bbf5f918533ba88748a Mon Sep 17 00:00:00 2001 From: Paul Annesley Date: Wed, 29 Apr 2026 19:36:46 +0930 Subject: [PATCH 4/9] upload: add --format flag to override extension-based detection Filename-extension inference is brittle: results files can have unconventional names (no extension, .junit, .results, .out, etc). --format junit --format json When --format is empty, behaviour is unchanged: format is inferred from .xml -> junit and .json -> json. Invalid formats produce a clear error rather than a server-side rejection. Amp-Thread-ID: https://ampcode.com/threads/T-019dd7a1-d422-717e-aac1-a48f10021895 Co-authored-by: Amp --- cli.go | 8 ++++++++ internal/upload/upload.go | 42 +++++++++++++++++++++++++++++---------- main.go | 2 +- 3 files changed, 41 insertions(+), 11 deletions(-) diff --git a/cli.go b/cli.go index 04294e38..c0bd8ee9 100644 --- a/cli.go +++ b/cli.go @@ -21,6 +21,13 @@ var uploadTokenFlag = &cli.StringFlag{ Destination: &uploadConfig.SuiteToken, } +var uploadFormatFlag = &cli.StringFlag{ + Name: "format", + Category: "TEST ENGINE", + Usage: "Upload format: junit or json. When unset, inferred from filename extension.", + Sources: cli.EnvVars("BUILDKITE_TEST_ENGINE_UPLOAD_FORMAT"), +} + var uploadUrlFlag = &cli.StringFlag{ Name: "upload-url", Category: "TEST ENGINE", @@ -592,6 +599,7 @@ var cliCommand = &cli.Command{ Action: uploadAction, Flags: []cli.Flag{ uploadTokenFlag, + uploadFormatFlag, uploadUrlFlag, }, }, diff --git a/internal/upload/upload.go b/internal/upload/upload.go index 807f4266..a6eb7b64 100644 --- a/internal/upload/upload.go +++ b/internal/upload/upload.go @@ -47,9 +47,32 @@ const uploadTimeout = 5 * time.Minute // httpClient is the HTTP client used for upload requests. var httpClient = &http.Client{Timeout: uploadTimeout} +// validFormats are the upload formats accepted by Test Engine. +var validFormats = map[string]bool{"junit": true, "json": true} + +// inferFormat picks an upload format based on the filename extension. +func inferFormat(filename string) (string, error) { + switch filepath.Ext(filename) { + case ".xml": + return "junit", nil + case ".json": + return "json", nil + default: + return "", fmt.Errorf("could not infer format from filename %q; pass --format junit|json", filename) + } +} + +func validateFormat(format string) error { + if !validFormats[format] { + return fmt.Errorf("invalid format %q; must be one of: junit, json", format) + } + return nil +} + // UploadFile uploads the given test results file to Test Engine, deriving -// run-env metadata from env. -func UploadFile(ctx context.Context, cfg Config, env EnvLookup, filename string) error { +// run-env metadata from env. If format is empty, it is inferred from the +// filename extension. +func UploadFile(ctx context.Context, cfg Config, env EnvLookup, filename string, format string) error { if cfg.SuiteToken == "" { return fmt.Errorf("BUILDKITE_ANALYTICS_TOKEN missing") } @@ -68,14 +91,13 @@ func UploadFile(ctx context.Context, cfg Config, env EnvLookup, filename string) return fmt.Errorf("not a regular file: %s", filename) } - var format string - switch filepath.Ext(filename) { - case ".xml": - format = "junit" - case ".json": - format = "json" - default: - return fmt.Errorf("could not infer format (JUnit / JSON) from filename") + if format == "" { + format, err = inferFormat(filename) + if err != nil { + return err + } + } else if err := validateFormat(format); err != nil { + return err } runEnv, err := RunEnvFromEnv(env) diff --git a/main.go b/main.go index b5f7eb54..c92ff50e 100644 --- a/main.go +++ b/main.go @@ -77,7 +77,7 @@ func uploadAction(ctx context.Context, cmd *cli.Command) error { return fmt.Errorf("expected exactly one argument: path to JUnit XML or JSON file") } - return upload.UploadFile(ctx, uploadConfig, os.LookupEnv, cmd.Args().First()) + return upload.UploadFile(ctx, uploadConfig, os.LookupEnv, cmd.Args().First(), cmd.String("format")) } func printVersion(ctx context.Context, cmd *cli.Command, versionFlag bool) error { From 4c8b43e4b202d43baed3d7e5eeddd61374052912 Mon Sep 17 00:00:00 2001 From: Paul Annesley Date: Wed, 29 Apr 2026 19:37:51 +0930 Subject: [PATCH 5/9] upload: add User-Agent header and retry on transient failures Brings upload requests in line with internal/api: User-Agent: Buildkite Test Engine Client/ (/) and adds an exponential-backoff retry loop (5 attempts, ~500ms base delay, jitter) for network errors, 429, and 5xx responses. The upload API has been seen returning transient 5xx, and previously the client would fail the whole job on the first hiccup. Body bytes are snapshotted up front so each retry attempt rebuilds a fresh request with a re-readable body. Per-attempt timeouts are still bounded by the package httpClient.Timeout (5 minutes). Amp-Thread-ID: https://ampcode.com/threads/T-019dd7a1-d422-717e-aac1-a48f10021895 Co-authored-by: Amp --- internal/upload/upload.go | 104 ++++++++++++++++++++++++++------------ 1 file changed, 71 insertions(+), 33 deletions(-) diff --git a/internal/upload/upload.go b/internal/upload/upload.go index a6eb7b64..81eb054c 100644 --- a/internal/upload/upload.go +++ b/internal/upload/upload.go @@ -13,12 +13,22 @@ import ( "net/http" "os" "path/filepath" + "runtime" "time" + "github.com/buildkite/roko" + "github.com/buildkite/test-engine-client/internal/debug" "github.com/buildkite/test-engine-client/internal/version" "github.com/google/uuid" ) +// userAgent matches the format used by internal/api so all bktec HTTP +// traffic is identifiable in server logs. +var userAgent = fmt.Sprintf( + "Buildkite Test Engine Client/%s (%s/%s)", + version.Version, runtime.GOOS, runtime.GOARCH, +) + // EnvLookup mirrors the signature of os.LookupEnv so callers can pass it // directly, while tests can substitute a map-backed lookup. type EnvLookup func(key string) (value string, ok bool) @@ -117,52 +127,80 @@ func UploadFile(ctx context.Context, cfg Config, env EnvLookup, filename string, return nil } -// Upload sends test result data to Test Engine. +// Upload sends test result data to Test Engine. Transient failures (network +// errors, 429, 5xx) are retried with exponential backoff, matching the +// behaviour of the internal/api client. func Upload(ctx context.Context, cfg Config, runEnv RunEnvMap, format string, filename string) (map[string]string, error) { body, err := buildUploadData(runEnv, format, filename) if err != nil { return nil, fmt.Errorf("preparing upload data: %w", err) } - req, err := http.NewRequestWithContext( - ctx, - http.MethodPost, - cfg.UploadUrl, - body.buf, + // Snapshot the body bytes and content type so each retry attempt can + // build a fresh request with a re-readable body. + bodyBytes := body.buf.Bytes() + contentType := body.writer.FormDataContentType() + + r := roko.NewRetrier( + roko.WithMaxAttempts(5), + roko.WithStrategy(roko.ExponentialSubsecond(500*time.Millisecond)), + roko.WithJitter(), ) - if err != nil { - return nil, fmt.Errorf("creating HTTP request: %w", err) - } - req.Header.Set("Content-Type", body.writer.FormDataContentType()) - req.Header.Set("Authorization", fmt.Sprintf(`Token token="%s"`, cfg.SuiteToken)) + var respData map[string]string + err = r.DoWithContext(ctx, func(r *roko.Retrier) error { + if r.AttemptCount() > 0 { + debug.Printf("Retrying upload, attempt %d", r.AttemptCount()) + } - resp, err := httpClient.Do(req) - if err != nil { - return nil, fmt.Errorf("HTTP error: %w", err) - } - defer resp.Body.Close() + req, err := http.NewRequestWithContext( + ctx, + http.MethodPost, + cfg.UploadUrl, + bytes.NewReader(bodyBytes), + ) + if err != nil { + r.Break() + return fmt.Errorf("creating HTTP request: %w", err) + } - status := resp.Status + req.Header.Set("Content-Type", contentType) + req.Header.Set("Authorization", fmt.Sprintf(`Token token="%s"`, cfg.SuiteToken)) + req.Header.Set("User-Agent", userAgent) - // Currently this should get HTTP 202 Accepted, but let's be a bit permissive to future changes. - if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusAccepted { - return nil, fmt.Errorf( - "expected HTTP %d or %d from Upload API, got %s", - http.StatusCreated, - http.StatusAccepted, - status, - ) - } + resp, err := httpClient.Do(req) + if err != nil { + // Network errors are retryable. + return fmt.Errorf("HTTP error: %w", err) + } + defer resp.Body.Close() - // try to parse the response, but just warn if that fails - respData := make(map[string]string) - err = json.NewDecoder(resp.Body).Decode(&respData) - if err != nil && !errors.Is(err, io.EOF) { - slog.Warn("failed to parse response", "status", status, "error", err) - } + // Retryable server-side conditions. + if resp.StatusCode == http.StatusTooManyRequests || resp.StatusCode >= 500 { + return fmt.Errorf("server returned %s", resp.Status) + } + + // Currently this should get HTTP 202 Accepted, but let's be a bit + // permissive to future changes. + if resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusAccepted { + r.Break() + return fmt.Errorf( + "expected HTTP %d or %d from Upload API, got %s", + http.StatusCreated, + http.StatusAccepted, + resp.Status, + ) + } + + // try to parse the response, but just warn if that fails + respData = make(map[string]string) + if err := json.NewDecoder(resp.Body).Decode(&respData); err != nil && !errors.Is(err, io.EOF) { + slog.Warn("failed to parse response", "status", resp.Status, "error", err) + } + return nil + }) - return respData, nil + return respData, err } func RunEnvFromEnv(env EnvLookup) (RunEnvMap, error) { From 9633694405439e262053c310b911881aad475309 Mon Sep 17 00:00:00 2001 From: Paul Annesley Date: Wed, 29 Apr 2026 19:38:08 +0930 Subject: [PATCH 6/9] CHANGELOG: add entry for bktec upload Amp-Thread-ID: https://ampcode.com/threads/T-019dd7a1-d422-717e-aac1-a48f10021895 Co-authored-by: Amp --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 44977928..91022d3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # Changelog +## Unreleased +- Add `bktec upload ` to upload JUnit XML or JSON test results to Buildkite Test Engine. Authenticated via `--token` (env: `BUILDKITE_ANALYTICS_TOKEN`); upload format is inferred from the filename extension or set explicitly with `--format junit|json`. Requests carry the standard bktec User-Agent and retry on transient network errors and 5xx responses. + ## 2.4.0 - 2026-04-17 - Automatically collect git commit metadata on `bktec plan` when `--selection-strategy` is set. Commit info, diff stats, and context fields are sent with the plan request so test selection has the signal it needs without the caller shelling out to git. Preview; gated behind `BKTEC_PREVIEW_SELECTION`. - Add `--collect-git-metadata` flag (env: `BUILDKITE_TEST_ENGINE_COLLECT_GIT_METADATA`) to `bktec plan` so pipelines can opt in to git metadata collection without using test selection. Preview; gated behind `BKTEC_PREVIEW_SELECTION`. From 2e596264bcb424715dde92d8b964b2e5948a1469 Mon Sep 17 00:00:00 2001 From: Paul Annesley Date: Wed, 29 Apr 2026 19:39:10 +0930 Subject: [PATCH 7/9] upload: add tests for UploadFile, retries, 4xx, User-Agent, --format Adds: - TestUpload_RetriesOn5xxThenSucceeds: confirms transient 5xx responses are retried and the eventual 202 is returned cleanly. - TestUpload_DoesNotRetryOn4xx: confirms client errors short-circuit the retry loop (no wasted retries on bad token / bad request). - TestUpload_SetsUserAgent: confirms the bktec User-Agent header is attached to upload requests. - TestUploadFile_EndToEnd: covers the integration of token validation, format inference, env-derived run metadata, and the HTTP send. - TestUploadFile_MissingToken: confirms missing SuiteToken errors out with the expected message. - TestUploadFile_FormatOverride: confirms an explicit --format value is used even when the filename has no usable extension. - TestUploadFile_FormatInferenceFailsWithoutExtension: confirms the helpful error when neither --format nor a recognised extension is available. Also fixes createTestXML to produce filenames ending in .xml (the previous os.CreateTemp("", "test.xml") put the random suffix *after* the extension, hiding format inference). Amp-Thread-ID: https://ampcode.com/threads/T-019dd7a1-d422-717e-aac1-a48f10021895 Co-authored-by: Amp --- internal/upload/upload_test.go | 149 ++++++++++++++++++++++++++++++++- 1 file changed, 148 insertions(+), 1 deletion(-) diff --git a/internal/upload/upload_test.go b/internal/upload/upload_test.go index 6128cdaa..c3063b59 100644 --- a/internal/upload/upload_test.go +++ b/internal/upload/upload_test.go @@ -9,6 +9,7 @@ import ( "net/http" "net/http/httptest" "os" + "strings" "testing" "github.com/buildkite/test-engine-client/internal/version" @@ -139,6 +140,152 @@ func TestUpload(t *testing.T) { } } +func TestUpload_RetriesOn5xxThenSucceeds(t *testing.T) { + filename, _ := createTestXML(t) + defer os.Remove(filename) + + var attempts int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts++ + if attempts < 3 { + w.WriteHeader(http.StatusBadGateway) + return + } + w.WriteHeader(http.StatusAccepted) + io.WriteString(w, `{"upload_url":"http://example/uploads/abc"}`) + })) + defer srv.Close() + + cfg := Config{UploadUrl: srv.URL, SuiteToken: "t"} + resp, err := Upload(context.Background(), cfg, RunEnvMap{"key": "k"}, "junit", filename) + if err != nil { + t.Fatalf("Upload after retries: %v", err) + } + if attempts != 3 { + t.Errorf("attempts = %d, want 3", attempts) + } + if resp["upload_url"] != "http://example/uploads/abc" { + t.Errorf("response = %v", resp) + } +} + +func TestUpload_DoesNotRetryOn4xx(t *testing.T) { + filename, _ := createTestXML(t) + defer os.Remove(filename) + + var attempts int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts++ + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + cfg := Config{UploadUrl: srv.URL, SuiteToken: "t"} + _, err := Upload(context.Background(), cfg, RunEnvMap{"key": "k"}, "junit", filename) + if err == nil { + t.Fatal("expected error from 401 response") + } + if attempts != 1 { + t.Errorf("attempts = %d, want 1 (no retry on 4xx)", attempts) + } +} + +func TestUpload_SetsUserAgent(t *testing.T) { + filename, _ := createTestXML(t) + defer os.Remove(filename) + + var gotUA string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotUA = r.Header.Get("User-Agent") + w.WriteHeader(http.StatusAccepted) + })) + defer srv.Close() + + cfg := Config{UploadUrl: srv.URL, SuiteToken: "t"} + _, _ = Upload(context.Background(), cfg, RunEnvMap{"key": "k"}, "junit", filename) + if !strings.HasPrefix(gotUA, "Buildkite Test Engine Client/") { + t.Errorf("User-Agent = %q, want prefix %q", gotUA, "Buildkite Test Engine Client/") + } +} + +func TestUploadFile_EndToEnd(t *testing.T) { + filename, _ := createTestXML(t) + defer os.Remove(filename) + + var gotData map[string]string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotData, _ = multipartToMap(r) + w.WriteHeader(http.StatusAccepted) + io.WriteString(w, `{}`) + })) + defer srv.Close() + + cfg := Config{UploadUrl: srv.URL, SuiteToken: "tok"} + env := mapLookup(map[string]string{ + "BUILDKITE_BUILD_ID": "build-1", + "BUILDKITE_BRANCH": "main", + }) + if err := UploadFile(context.Background(), cfg, env, filename, ""); err != nil { + t.Fatalf("UploadFile: %v", err) + } + if got, want := gotData["format"], "junit"; got != want { + t.Errorf("format = %q, want %q", got, want) + } + if got, want := gotData["run_env[key]"], "build-1"; got != want { + t.Errorf("run_env[key] = %q, want %q", got, want) + } +} + +func TestUploadFile_MissingToken(t *testing.T) { + cfg := Config{} + err := UploadFile(context.Background(), cfg, mapLookup(nil), "any.xml", "") + if err == nil || !strings.Contains(err.Error(), "BUILDKITE_ANALYTICS_TOKEN") { + t.Errorf("err = %v, want missing-token error", err) + } +} + +func TestUploadFile_FormatOverride(t *testing.T) { + // File has no extension, but explicit --format wins. + f, err := os.CreateTemp("", "results") + if err != nil { + t.Fatal(err) + } + io.WriteString(f, `{"ok":true}`) + f.Close() + defer os.Remove(f.Name()) + + var gotFormat string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + data, _ := multipartToMap(r) + gotFormat = data["format"] + w.WriteHeader(http.StatusAccepted) + })) + defer srv.Close() + + cfg := Config{UploadUrl: srv.URL, SuiteToken: "t"} + if err := UploadFile(context.Background(), cfg, mapLookup(nil), f.Name(), "json"); err != nil { + t.Fatalf("UploadFile: %v", err) + } + if gotFormat != "json" { + t.Errorf("format = %q, want json", gotFormat) + } +} + +func TestUploadFile_FormatInferenceFailsWithoutExtension(t *testing.T) { + f, err := os.CreateTemp("", "results") + if err != nil { + t.Fatal(err) + } + f.Close() + defer os.Remove(f.Name()) + + cfg := Config{UploadUrl: "http://unused", SuiteToken: "t"} + err = UploadFile(context.Background(), cfg, mapLookup(nil), f.Name(), "") + if err == nil || !strings.Contains(err.Error(), "could not infer format") { + t.Errorf("err = %v, want infer-format error", err) + } +} + // mapLookup adapts a map to the EnvLookup function signature. func mapLookup(m map[string]string) EnvLookup { return func(k string) (string, bool) { @@ -160,7 +307,7 @@ func cmpKeyValidUUID() cmp.Option { func createTestXML(t *testing.T) (string, string) { data := `` - f, err := os.CreateTemp("", "test.xml") + f, err := os.CreateTemp("", "test*.xml") if err != nil { t.Fatal(err) } From d41edcce9da2357599637db2f096c44ef378ed8f Mon Sep 17 00:00:00 2001 From: Paul Annesley Date: Wed, 29 Apr 2026 19:39:38 +0930 Subject: [PATCH 8/9] upload: wrap os.Stat error to expose underlying cause The previous "file does not exist: %s" message obscured permission errors, EACCES on parent dirs, EIO, etc. Wrapping with %w preserves errors.Is(err, fs.ErrNotExist) for callers that want to distinguish missing files from other failure modes. Amp-Thread-ID: https://ampcode.com/threads/T-019dd7a1-d422-717e-aac1-a48f10021895 Co-authored-by: Amp --- internal/upload/upload.go | 2 +- internal/upload/upload_test.go | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/internal/upload/upload.go b/internal/upload/upload.go index 81eb054c..2f36f52d 100644 --- a/internal/upload/upload.go +++ b/internal/upload/upload.go @@ -96,7 +96,7 @@ func UploadFile(ctx context.Context, cfg Config, env EnvLookup, filename string, info, err := os.Stat(filename) if err != nil { - return fmt.Errorf("file does not exist: %s", filename) + return fmt.Errorf("cannot stat %s: %w", filename, err) } else if !info.Mode().IsRegular() { return fmt.Errorf("not a regular file: %s", filename) } diff --git a/internal/upload/upload_test.go b/internal/upload/upload_test.go index c3063b59..fa9cc0bd 100644 --- a/internal/upload/upload_test.go +++ b/internal/upload/upload_test.go @@ -2,8 +2,10 @@ package upload import ( "context" + "errors" "fmt" "io" + "io/fs" "mime" "mime/multipart" "net/http" @@ -271,6 +273,17 @@ func TestUploadFile_FormatOverride(t *testing.T) { } } +func TestUploadFile_StatErrorIsWrapped(t *testing.T) { + cfg := Config{UploadUrl: "http://unused", SuiteToken: "t"} + err := UploadFile(context.Background(), cfg, mapLookup(nil), "/no/such/file.xml", "") + if err == nil { + t.Fatal("expected error for missing file") + } + if !errors.Is(err, fs.ErrNotExist) { + t.Errorf("err = %v, want errors.Is(err, fs.ErrNotExist)", err) + } +} + func TestUploadFile_FormatInferenceFailsWithoutExtension(t *testing.T) { f, err := os.CreateTemp("", "results") if err != nil { From 953c6b38144dbc4c03f65fe321f0e07ecbc82401 Mon Sep 17 00:00:00 2001 From: Paul Annesley Date: Wed, 29 Apr 2026 19:42:26 +0930 Subject: [PATCH 9/9] upload: read build environment from cfg via cli flags Replaces the EnvLookup function-typed parameter with a typed BuildEnv struct populated from cli flags, sharing the existing buildIDFlag, branchFlag, and jobIDFlag with bktec run / plan, and adding commitFlag, messageFlag, buildNumberFlag, buildUrlFlag bound to new config.Config fields (Commit, Message, BuildNumber, BuildUrl). The upload package no longer touches os.Environ at all: env access is fully consolidated in cli.go where every other command reads its inputs. RunEnvFromEnv is replaced with RunEnvFromBuildEnv that takes the typed struct directly, which also makes the Buildkite-vs-generic detection more obvious (build.BuildId != ""). Amp-Thread-ID: https://ampcode.com/threads/T-019dd7a1-d422-717e-aac1-a48f10021895 Co-authored-by: Amp --- cli.go | 44 +++++++++++++++++++++++++++++ internal/config/config.go | 9 ++++-- internal/upload/upload.go | 51 +++++++++++++++++++++------------- internal/upload/upload_test.go | 43 +++++++++++----------------- main.go | 12 +++++++- 5 files changed, 109 insertions(+), 50 deletions(-) diff --git a/cli.go b/cli.go index c0bd8ee9..b90608c0 100644 --- a/cli.go +++ b/cli.go @@ -135,6 +135,42 @@ var jobIDFlag = &cli.StringFlag{ Hidden: true, } +var commitFlag = &cli.StringFlag{ + Name: "commit", + Category: "BUILD ENVIRONMENT", + Usage: "Git commit SHA being built", + Sources: cli.EnvVars("BUILDKITE_COMMIT"), + Destination: &cfg.Commit, + Hidden: true, +} + +var messageFlag = &cli.StringFlag{ + Name: "message", + Category: "BUILD ENVIRONMENT", + Usage: "Buildkite build message", + Sources: cli.EnvVars("BUILDKITE_MESSAGE"), + Destination: &cfg.Message, + Hidden: true, +} + +var buildNumberFlag = &cli.StringFlag{ + Name: "build-number", + Category: "BUILD ENVIRONMENT", + Usage: "Buildkite build number", + Sources: cli.EnvVars("BUILDKITE_BUILD_NUMBER"), + Destination: &cfg.BuildNumber, + Hidden: true, +} + +var buildUrlFlag = &cli.StringFlag{ + Name: "build-url", + Category: "BUILD ENVIRONMENT", + Usage: "Buildkite build URL", + Sources: cli.EnvVars("BUILDKITE_BUILD_URL"), + Destination: &cfg.BuildUrl, + Hidden: true, +} + var stepIDFlag = &cli.StringFlag{ Name: "step-id", Category: "BUILD ENVIRONMENT", @@ -601,6 +637,14 @@ var cliCommand = &cli.Command{ uploadTokenFlag, uploadFormatFlag, uploadUrlFlag, + // Build environment values used to populate run_env on uploads. + buildIDFlag, + branchFlag, + commitFlag, + jobIDFlag, + messageFlag, + buildNumberFlag, + buildUrlFlag, }, }, { diff --git a/internal/config/config.go b/internal/config/config.go index eed2bd3b..950c9db5 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -7,8 +7,12 @@ type Config struct { // AccessToken is the access token for the API. AccessToken string `json:"-"` // Branch is the string value of the git branch name, used by Buildkite only. - Branch string `json:"BUILDKITE_BRANCH"` - BuildId string `json:"BUILDKITE_BUILD_ID"` + Branch string `json:"BUILDKITE_BRANCH"` + BuildId string `json:"BUILDKITE_BUILD_ID"` + BuildNumber string `json:"BUILDKITE_BUILD_NUMBER"` + BuildUrl string `json:"BUILDKITE_BUILD_URL"` + // Commit is the git commit SHA being built. + Commit string `json:"BUILDKITE_COMMIT"` // CollectGitMetadata enables git metadata auto-collection on plan without requiring --selection-strategy to be set. CollectGitMetadata bool `json:"-"` // Concurrency is the number of concurrent git operations for diff collection (default 10). @@ -22,6 +26,7 @@ type Config struct { // Identifier is the identifier of the build. Identifier string `json:"BUILDKITE_TEST_ENGINE_IDENTIFIER"` JobId string `json:"BUILDKITE_JOB_ID"` + Message string `json:"BUILDKITE_MESSAGE"` // JobRetryCount is the count of the number of times the job has been retried. JobRetryCount int `json:"BUILDKITE_RETRY_COUNT"` // LocationPrefix is prepended to test file paths when requesting a test plan. diff --git a/internal/upload/upload.go b/internal/upload/upload.go index 2f36f52d..77dc98cf 100644 --- a/internal/upload/upload.go +++ b/internal/upload/upload.go @@ -29,12 +29,22 @@ var userAgent = fmt.Sprintf( version.Version, runtime.GOOS, runtime.GOARCH, ) -// EnvLookup mirrors the signature of os.LookupEnv so callers can pass it -// directly, while tests can substitute a map-backed lookup. -type EnvLookup func(key string) (value string, ok bool) - type RunEnvMap map[string]string +// BuildEnv carries the runtime build/job context that ends up in the upload +// run_env metadata. Fields are typically populated from cli flags (which in +// turn read BUILDKITE_* env vars) so the upload package itself does not +// touch os.Environ. +type BuildEnv struct { + BuildId string + Branch string + Commit string + JobId string + Message string + BuildNumber string + BuildUrl string +} + // Config is upload-specific configuration. UploadUrl and SuiteToken are // typically populated from cli flags in cmd/main. type Config struct { @@ -79,10 +89,9 @@ func validateFormat(format string) error { return nil } -// UploadFile uploads the given test results file to Test Engine, deriving -// run-env metadata from env. If format is empty, it is inferred from the -// filename extension. -func UploadFile(ctx context.Context, cfg Config, env EnvLookup, filename string, format string) error { +// UploadFile uploads the given test results file to Test Engine. If format +// is empty, it is inferred from the filename extension. +func UploadFile(ctx context.Context, cfg Config, build BuildEnv, filename string, format string) error { if cfg.SuiteToken == "" { return fmt.Errorf("BUILDKITE_ANALYTICS_TOKEN missing") } @@ -110,7 +119,7 @@ func UploadFile(ctx context.Context, cfg Config, env EnvLookup, filename string, return err } - runEnv, err := RunEnvFromEnv(env) + runEnv, err := RunEnvFromBuildEnv(build) if err != nil { return fmt.Errorf("unable to derive runEnv: %w", err) } @@ -203,24 +212,26 @@ func Upload(ctx context.Context, cfg Config, runEnv RunEnvMap, format string, fi return respData, err } -func RunEnvFromEnv(env EnvLookup) (RunEnvMap, error) { - get := func(k string) string { v, _ := env(k); return v } - +// RunEnvFromBuildEnv builds the run_env map sent to the upload API. When +// build.BuildId is set we treat the run as Buildkite-originated and emit the +// full set of build context fields; otherwise we emit a generic-CI run with +// a fresh UUIDv7 key. +func RunEnvFromBuildEnv(build BuildEnv) (RunEnvMap, error) { runEnv := RunEnvMap{ "collector": "bktec", "version": version.Version, } - if _, ok := env("BUILDKITE_BUILD_ID"); ok { + if build.BuildId != "" { maps.Copy(runEnv, RunEnvMap{ "CI": "buildkite", - "branch": get("BUILDKITE_BRANCH"), - "commit_sha": get("BUILDKITE_COMMIT"), - "job_id": get("BUILDKITE_JOB_ID"), - "key": get("BUILDKITE_BUILD_ID"), - "message": get("BUILDKITE_MESSAGE"), - "number": get("BUILDKITE_BUILD_NUMBER"), - "url": get("BUILDKITE_BUILD_URL"), + "branch": build.Branch, + "commit_sha": build.Commit, + "job_id": build.JobId, + "key": build.BuildId, + "message": build.Message, + "number": build.BuildNumber, + "url": build.BuildUrl, }) } else { key, err := uuid.NewV7() diff --git a/internal/upload/upload_test.go b/internal/upload/upload_test.go index fa9cc0bd..36d7fa12 100644 --- a/internal/upload/upload_test.go +++ b/internal/upload/upload_test.go @@ -20,15 +20,15 @@ import ( ) func TestBuildRunEnv(t *testing.T) { - runEnv, err := RunEnvFromEnv(mapLookup(map[string]string{ - "BUILDKITE_BUILD_ID": "thebuild", - "BUILDKITE_BRANCH": "trunk", - "BUILDKITE_COMMIT": "cafe", - "BUILDKITE_JOB_ID": "thejob", - "BUILDKITE_MESSAGE": "hello world", - "BUILDKITE_BUILD_NUMBER": "42", - "BUILDKITE_BUILD_URL": "http://localhost/builds/42", - })) + runEnv, err := RunEnvFromBuildEnv(BuildEnv{ + BuildId: "thebuild", + Branch: "trunk", + Commit: "cafe", + JobId: "thejob", + Message: "hello world", + BuildNumber: "42", + BuildUrl: "http://localhost/builds/42", + }) if err != nil { t.Errorf("buildRunEnv(): %v", err) } @@ -52,7 +52,7 @@ func TestBuildRunEnv(t *testing.T) { } func TestBuildRunEnv_generic(t *testing.T) { - runEnv, err := RunEnvFromEnv(mapLookup(map[string]string{})) + runEnv, err := RunEnvFromBuildEnv(BuildEnv{}) if err != nil { t.Errorf("buildRunEnv(): %v", err) } @@ -223,11 +223,8 @@ func TestUploadFile_EndToEnd(t *testing.T) { defer srv.Close() cfg := Config{UploadUrl: srv.URL, SuiteToken: "tok"} - env := mapLookup(map[string]string{ - "BUILDKITE_BUILD_ID": "build-1", - "BUILDKITE_BRANCH": "main", - }) - if err := UploadFile(context.Background(), cfg, env, filename, ""); err != nil { + build := BuildEnv{BuildId: "build-1", Branch: "main"} + if err := UploadFile(context.Background(), cfg, build, filename, ""); err != nil { t.Fatalf("UploadFile: %v", err) } if got, want := gotData["format"], "junit"; got != want { @@ -240,7 +237,7 @@ func TestUploadFile_EndToEnd(t *testing.T) { func TestUploadFile_MissingToken(t *testing.T) { cfg := Config{} - err := UploadFile(context.Background(), cfg, mapLookup(nil), "any.xml", "") + err := UploadFile(context.Background(), cfg, BuildEnv{}, "any.xml", "") if err == nil || !strings.Contains(err.Error(), "BUILDKITE_ANALYTICS_TOKEN") { t.Errorf("err = %v, want missing-token error", err) } @@ -265,7 +262,7 @@ func TestUploadFile_FormatOverride(t *testing.T) { defer srv.Close() cfg := Config{UploadUrl: srv.URL, SuiteToken: "t"} - if err := UploadFile(context.Background(), cfg, mapLookup(nil), f.Name(), "json"); err != nil { + if err := UploadFile(context.Background(), cfg, BuildEnv{}, f.Name(), "json"); err != nil { t.Fatalf("UploadFile: %v", err) } if gotFormat != "json" { @@ -275,7 +272,7 @@ func TestUploadFile_FormatOverride(t *testing.T) { func TestUploadFile_StatErrorIsWrapped(t *testing.T) { cfg := Config{UploadUrl: "http://unused", SuiteToken: "t"} - err := UploadFile(context.Background(), cfg, mapLookup(nil), "/no/such/file.xml", "") + err := UploadFile(context.Background(), cfg, BuildEnv{}, "/no/such/file.xml", "") if err == nil { t.Fatal("expected error for missing file") } @@ -293,20 +290,12 @@ func TestUploadFile_FormatInferenceFailsWithoutExtension(t *testing.T) { defer os.Remove(f.Name()) cfg := Config{UploadUrl: "http://unused", SuiteToken: "t"} - err = UploadFile(context.Background(), cfg, mapLookup(nil), f.Name(), "") + err = UploadFile(context.Background(), cfg, BuildEnv{}, f.Name(), "") if err == nil || !strings.Contains(err.Error(), "could not infer format") { t.Errorf("err = %v, want infer-format error", err) } } -// mapLookup adapts a map to the EnvLookup function signature. -func mapLookup(m map[string]string) EnvLookup { - return func(k string) (string, bool) { - v, ok := m[k] - return v, ok - } -} - // cmpKeyValidUUID is an Option for cmp.Diff that validates the values of `key` // in two maps being compared are both valid UUIDs. Note that Comparer // functions must be symmetric; they're run as fn(a,b) and fn(b,a). diff --git a/main.go b/main.go index c92ff50e..daca0d00 100644 --- a/main.go +++ b/main.go @@ -77,7 +77,17 @@ func uploadAction(ctx context.Context, cmd *cli.Command) error { return fmt.Errorf("expected exactly one argument: path to JUnit XML or JSON file") } - return upload.UploadFile(ctx, uploadConfig, os.LookupEnv, cmd.Args().First(), cmd.String("format")) + build := upload.BuildEnv{ + BuildId: cfg.BuildId, + Branch: cfg.Branch, + Commit: cfg.Commit, + JobId: cfg.JobId, + Message: cfg.Message, + BuildNumber: cfg.BuildNumber, + BuildUrl: cfg.BuildUrl, + } + + return upload.UploadFile(ctx, uploadConfig, build, cmd.Args().First(), cmd.String("format")) } func printVersion(ctx context.Context, cmd *cli.Command, versionFlag bool) error {