From f671fa0306ed1a928b10cc259d9634cd60c322b4 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Fri, 23 Jun 2023 11:32:34 +0100 Subject: [PATCH 1/5] refactor part 1: Remove global state In order to make the code testable one thing we need to do is eliminate the global state. That's what we do here. The handlers are turned into functions and they can have their own local state which they pass around. --- cmd/wait-for-github/ci.go | 75 ++++++++++--------- cmd/wait-for-github/config.go | 2 - cmd/wait-for-github/main.go | 2 +- cmd/wait-for-github/pr.go | 55 ++++++++------ cmd/wait-for-github/root.go | 134 +++++++++++++++++++--------------- 5 files changed, 150 insertions(+), 118 deletions(-) diff --git a/cmd/wait-for-github/ci.go b/cmd/wait-for-github/ci.go index 29db369d..eba07ee4 100644 --- a/cmd/wait-for-github/ci.go +++ b/cmd/wait-for-github/ci.go @@ -21,6 +21,7 @@ import ( "fmt" "regexp" "strings" + "time" "github.com/grafana/wait-for-github/internal/github" "github.com/grafana/wait-for-github/internal/utils" @@ -41,10 +42,9 @@ type ciConfig struct { var ( // https://regex101.com/r/dqMmDP/1 commitRegexp = regexp.MustCompile(`.*github\.com/(?P[^/]+)/(?P[^/]+)/commit/(?P[abcdef\d]+)/?.*`) - ciConf ciConfig ) -func parseCIArguments(c *cli.Context) error { +func parseCIArguments(c *cli.Context) (ciConfig, error) { var owner, repo, ref string switch { @@ -53,7 +53,7 @@ func parseCIArguments(c *cli.Context) error { url := c.Args().Get(0) match := commitRegexp.FindStringSubmatch(url) if match == nil { - return fmt.Errorf("invalid commit URL: %s", url) + return ciConfig{}, fmt.Errorf("invalid commit URL: %s", url) } owner = match[1] @@ -74,23 +74,23 @@ func parseCIArguments(c *cli.Context) error { cli.ShowCommandHelpAndExit(parent, "ci", 1) // shouldn't get here, the previous line should exit - return nil + return ciConfig{}, nil } - ciConf.owner = owner - ciConf.repo = repo - ciConf.ref = ref - ciConf.checks = c.StringSlice("check") - log.Debugf("Will wait for CI on %s/%s@%s", owner, repo, ref) - return nil + return ciConfig{ + owner: owner, + repo: repo, + ref: ref, + checks: c.StringSlice("check"), + }, nil } -func handleCIStatus(status github.CIStatus) cli.ExitCoder { +func handleCIStatus(status github.CIStatus, recheckInterval time.Duration) cli.ExitCoder { switch status { case github.CIStatusUnknown: - log.Infof("CI status is unknown, rechecking in %s", cfg.recheckInterval) + log.Infof("CI status is unknown, rechecking in %s", recheckInterval) case github.CIStatusPending: case github.CIStatusPassed: return cli.Exit("CI successful", 0) @@ -98,11 +98,11 @@ func handleCIStatus(status github.CIStatus) cli.ExitCoder { return cli.Exit("CI failed", 1) } - log.Infof("CI is not finished yet, rechecking in %s", cfg.recheckInterval) + log.Infof("CI is not finished yet, rechecking in %s", recheckInterval) return nil } -func checkCIStatus(c *cli.Context) error { +func checkCIStatus(c *cli.Context, cfg *config, ciConf *ciConfig) error { ctx := context.Background() timeoutCtx, cancel := context.WithTimeout(ctx, cfg.globalTimeout) @@ -133,7 +133,7 @@ func checkCIStatus(c *cli.Context) error { log.Infof("CI checks are not finished yet (still waiting for %s), rechecking in %s", strings.Join(interestingChecks, ", "), cfg.recheckInterval) } - return handleCIStatus(status) + return handleCIStatus(status, cfg.recheckInterval) } checkAllCI := func() error { @@ -142,7 +142,7 @@ func checkCIStatus(c *cli.Context) error { return err } - ret := handleCIStatus(status) + ret := handleCIStatus(status, cfg.recheckInterval) if ret == nil { log.Infof("CI checks are not finished yet, rechecking in %s", cfg.recheckInterval) @@ -159,23 +159,32 @@ func checkCIStatus(c *cli.Context) error { return utils.RunUntilCancelledOrTimeout(timeoutCtx, checkAllCI, cfg.recheckInterval) } -var ciCommand = &cli.Command{ - Name: "ci", - Usage: "Wait for CI to be finished", - ArgsUsage: " [ ]", - Before: parseCIArguments, - Action: checkCIStatus, - Flags: []cli.Flag{ - &cli.StringSliceFlag{ - Name: "check", - Aliases: []string{ - "c", - }, - Usage: "Check the status of a specific CI check. " + - "By default, the status of all checks is checked.", - EnvVars: []string{ - "GITHUB_CI_CHECKS", +func ciCommand(cfg *config) *cli.Command { + var ciConf ciConfig + + return &cli.Command{ + Name: "ci", + Usage: "Wait for CI to be finished", + ArgsUsage: " [ ]", + Before: func(c *cli.Context) error { + var err error + ciConf, err = parseCIArguments(c) + + return err + }, + Action: func(c *cli.Context) error { return checkCIStatus(c, cfg, &ciConf) }, + Flags: []cli.Flag{ + &cli.StringSliceFlag{ + Name: "check", + Aliases: []string{ + "c", + }, + Usage: "Check the status of a specific CI check. " + + "By default, the status of all checks is checked.", + EnvVars: []string{ + "GITHUB_CI_CHECKS", + }, }, }, - }, + } } diff --git a/cmd/wait-for-github/config.go b/cmd/wait-for-github/config.go index 7272bc7b..c6e67643 100644 --- a/cmd/wait-for-github/config.go +++ b/cmd/wait-for-github/config.go @@ -27,5 +27,3 @@ type config struct { recheckInterval time.Duration globalTimeout time.Duration } - -var cfg config diff --git a/cmd/wait-for-github/main.go b/cmd/wait-for-github/main.go index 3cbe6098..13838f29 100644 --- a/cmd/wait-for-github/main.go +++ b/cmd/wait-for-github/main.go @@ -23,7 +23,7 @@ import ( ) func main() { - if err := root.Run(os.Args); err != nil { + if err := root().Run(os.Args); err != nil { log.Fatal(err) } } diff --git a/cmd/wait-for-github/pr.go b/cmd/wait-for-github/pr.go index 7c049c8c..dd6d7438 100644 --- a/cmd/wait-for-github/pr.go +++ b/cmd/wait-for-github/pr.go @@ -39,10 +39,9 @@ type prConfig struct { var ( // https://regex101.com/r/nexaWT/1 pullRequestRegexp = regexp.MustCompile(`.*github\.com/(?P[^/]+)/(?P[^/]+)/pull/(?P\d+)/?.*`) - prConf prConfig ) -func parsePRArguments(c *cli.Context) error { +func parsePRArguments(c *cli.Context) (prConfig, error) { var owner, repo, number string switch { @@ -51,7 +50,7 @@ func parsePRArguments(c *cli.Context) error { url := c.Args().Get(0) match := pullRequestRegexp.FindStringSubmatch(url) if match == nil { - return fmt.Errorf("invalid pull request URL: %s", url) + return prConfig{}, fmt.Errorf("invalid pull request URL: %s", url) } owner = match[1] @@ -72,21 +71,20 @@ func parsePRArguments(c *cli.Context) error { cli.ShowCommandHelpAndExit(parent, "pr", 1) // shouldn't get here, the previous line should exit - return nil + return prConfig{}, nil } - prConf.owner = owner - prConf.repo = repo - n, err := strconv.Atoi(number) if err != nil { - return fmt.Errorf("PR must be a number, got '%s'", c.Args().Get(2)) + return prConfig{}, fmt.Errorf("PR must be a number, got '%s'", c.Args().Get(2)) } - prConf.pr = n - log.Infof("Waiting for PR %s/%s#%d to be merged/closed", owner, repo, n) - return nil + return prConfig{ + owner: owner, + repo: repo, + pr: n, + }, nil } type commitInfo struct { @@ -96,7 +94,7 @@ type commitInfo struct { MergedAt int64 `json:"mergedAt"` } -func checkPRMerged(c *cli.Context) error { +func checkPRMerged(c *cli.Context, cfg *config, prConf *prConfig) error { ctx := context.Background() timeoutCtx, cancel := context.WithTimeout(ctx, cfg.globalTimeout) @@ -149,17 +147,26 @@ func checkPRMerged(c *cli.Context) error { return utils.RunUntilCancelledOrTimeout(timeoutCtx, checkPRMergedOrClosed, cfg.recheckInterval) } -var prCommand = &cli.Command{ - Name: "pr", - Usage: "Wait for a PR to be merged", - ArgsUsage: " [ ]", - Flags: []cli.Flag{ - &cli.StringFlag{ - Name: "commit-info-file", - Usage: "Path to a file which the commit info will be written. " + - "The file will be overwritten if it already exists.", +func prCommand(cfg *config) *cli.Command { + var prConf prConfig + + return &cli.Command{ + Name: "pr", + Usage: "Wait for a PR to be merged", + ArgsUsage: " [ ]", + Flags: []cli.Flag{ + &cli.StringFlag{ + Name: "commit-info-file", + Usage: "Path to a file which the commit info will be written. " + + "The file will be overwritten if it already exists.", + }, }, - }, - Before: parsePRArguments, - Action: checkPRMerged, + Before: func(c *cli.Context) error { + var err error + prConf, err = parsePRArguments(c) + + return err + }, + Action: func(c *cli.Context) error { return checkPRMerged(c, cfg, &prConf) }, + } } diff --git a/cmd/wait-for-github/root.go b/cmd/wait-for-github/root.go index 11856181..055a5e61 100644 --- a/cmd/wait-for-github/root.go +++ b/cmd/wait-for-github/root.go @@ -28,60 +28,76 @@ import ( log "github.com/sirupsen/logrus" ) -var root = &cli.App{ - Name: "wait-for-github", - Usage: "Wait for things to happen on GitHub", - Flags: []cli.Flag{ - &cli.StringFlag{ - Name: "log-level", - Aliases: []string{"l"}, - Usage: fmt.Sprintf("Set the log level. Valid levels are: %s.", strings.Join(validLogLevels(), ", ")), - Value: "info", - }, - &cli.StringFlag{ - Name: "github-app-private-key-path", - Aliases: []string{"p"}, - Usage: "Path to the GitHub App private key", - }, - &cli.StringFlag{ - Name: "github-app-private-key", - Usage: "Contents of the GitHub App private key", - EnvVars: []string{"GITHUB_APP_PRIVATE_KEY"}, - }, - &cli.Int64Flag{ - Name: "github-app-id", - Usage: "GitHub App ID", - EnvVars: []string{"GITHUB_APP_ID"}, - }, - &cli.Int64Flag{ - Name: "github-app-installation-id", - Usage: "GitHub App installation ID", - EnvVars: []string{"GITHUB_APP_INSTALLATION_ID"}, - }, - &cli.StringFlag{ - Name: "github-token", - Usage: "GitHub token. If not provided, the app will try to use the " + - "GitHub App authentication mechanism.", - EnvVars: []string{"GITHUB_TOKEN"}, - }, - &cli.DurationFlag{ - Name: "recheck-interval", - Usage: "Interval after which to recheck GitHub.", - EnvVars: []string{"RECHECK_INTERVAL"}, - Value: time.Duration(30 * time.Second), +type cmdFunc func(cfg *config) *cli.Command + +func root() *cli.App { + var cfg config + + var commands []*cli.Command + for _, cf := range []cmdFunc{ciCommand, prCommand} { + commands = append(commands, cf(&cfg)) + } + + return &cli.App{ + Name: "wait-for-github", + Usage: "Wait for things to happen on GitHub", + Flags: []cli.Flag{ + &cli.StringFlag{ + Name: "log-level", + Aliases: []string{"l"}, + Usage: fmt.Sprintf("Set the log level. Valid levels are: %s.", strings.Join(validLogLevels(), ", ")), + Value: "info", + }, + &cli.StringFlag{ + Name: "github-app-private-key-path", + Aliases: []string{"p"}, + Usage: "Path to the GitHub App private key", + }, + &cli.StringFlag{ + Name: "github-app-private-key", + Usage: "Contents of the GitHub App private key", + EnvVars: []string{"GITHUB_APP_PRIVATE_KEY"}, + }, + &cli.Int64Flag{ + Name: "github-app-id", + Usage: "GitHub App ID", + EnvVars: []string{"GITHUB_APP_ID"}, + }, + &cli.Int64Flag{ + Name: "github-app-installation-id", + Usage: "GitHub App installation ID", + EnvVars: []string{"GITHUB_APP_INSTALLATION_ID"}, + }, + &cli.StringFlag{ + Name: "github-token", + Usage: "GitHub token. If not provided, the app will try to use the " + + "GitHub App authentication mechanism.", + EnvVars: []string{"GITHUB_TOKEN"}, + }, + &cli.DurationFlag{ + Name: "recheck-interval", + Usage: "Interval after which to recheck GitHub.", + EnvVars: []string{"RECHECK_INTERVAL"}, + Value: time.Duration(30 * time.Second), + }, + &cli.DurationFlag{ + Name: "timeout", + Usage: "Timeout after which to stop checking GitHub.", + EnvVars: []string{"TIMEOUT"}, + Value: time.Duration(7 * 24 * time.Hour), + }, }, - &cli.DurationFlag{ - Name: "timeout", - Usage: "Timeout after which to stop checking GitHub.", - EnvVars: []string{"TIMEOUT"}, - Value: time.Duration(7 * 24 * time.Hour), + Commands: commands, + Before: func(c *cli.Context) error { + var err error + cfg, err = handleGlobalConfig(c) + if err != nil { + return err + } + + return nil }, - }, - Commands: []*cli.Command{ - ciCommand, - prCommand, - }, - Before: handleGlobalConfig, + } } func validLogLevels() []string { @@ -92,7 +108,7 @@ func validLogLevels() []string { return levels } -func handleGlobalConfig(c *cli.Context) error { +func handleGlobalConfig(c *cli.Context) (config, error) { formatter := &log.TextFormatter{ FullTimestamp: true, TimestampFormat: "2006-01-02 15:04:05", @@ -101,7 +117,7 @@ func handleGlobalConfig(c *cli.Context) error { level := c.String("log-level") if logLevel, err := log.ParseLevel(level); err != nil { - return fmt.Errorf("invalid log level: %s. valid levels are: %s", level, strings.Join(validLogLevels(), ", ")) + return config{}, fmt.Errorf("invalid log level: %s. valid levels are: %s", level, strings.Join(validLogLevels(), ", ")) } else { log.SetLevel(logLevel) } @@ -109,16 +125,18 @@ func handleGlobalConfig(c *cli.Context) error { log.Debug("Debug logging enabled") log.Trace("Trace logging enabled") + cfg := config{} + cfg.recheckInterval = c.Duration("recheck-interval") cfg.globalTimeout = c.Duration("timeout") token := c.String("github-token") if token != "" { - log.Debug("Using GitHub token for authentication") + log.Debug("Will use GitHub token for authentication") log.Debug("Using token starting with ", token[:10], "...") cfg.AuthInfo.GithubToken = token - return nil + return cfg, nil } privateKey := []byte(c.String("github-app-private-key")) @@ -128,7 +146,7 @@ func handleGlobalConfig(c *cli.Context) error { var err error privateKey, err = os.ReadFile(file) if err != nil { - return fmt.Errorf("failed to read private key file: %w", err) + return config{}, fmt.Errorf("failed to read private key file: %w", err) } } @@ -146,5 +164,5 @@ func handleGlobalConfig(c *cli.Context) error { PrivateKey: privateKey, } - return nil + return cfg, nil } From 4445cd7dd3fc26c8d885c4d45e167a882955a2d7 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Fri, 23 Jun 2023 14:11:15 +0100 Subject: [PATCH 2/5] refactor part 2: Separate functions Add a `Check` interface. This lets us separate out the part which is repeatedly called into their own functions. Then they'll be able to be tested. --- cmd/wait-for-github/ci.go | 89 +++++++++++++++++++++-------------- cmd/wait-for-github/main.go | 2 +- cmd/wait-for-github/pr.go | 91 ++++++++++++++++++++++-------------- cmd/wait-for-github/root.go | 2 +- internal/utils/utils.go | 10 ++-- internal/utils/utils_test.go | 66 +++++++++++++++++--------- 6 files changed, 162 insertions(+), 98 deletions(-) diff --git a/cmd/wait-for-github/ci.go b/cmd/wait-for-github/ci.go index eba07ee4..99bbb810 100644 --- a/cmd/wait-for-github/ci.go +++ b/cmd/wait-for-github/ci.go @@ -1,5 +1,5 @@ // wait-for-github -// Copyright (C) 2022, Grafana Labs +// Copyright (C) 2022-2023, Grafana Labs // This program is free software: you can redistribute it and/or modify it under // the terms of the GNU Affero General Public License as published by the Free @@ -102,61 +102,80 @@ func handleCIStatus(status github.CIStatus, recheckInterval time.Duration) cli.E return nil } -func checkCIStatus(c *cli.Context, cfg *config, ciConf *ciConfig) error { - ctx := context.Background() - - timeoutCtx, cancel := context.WithTimeout(ctx, cfg.globalTimeout) - defer cancel() +type checkAllCI struct { + githubClient github.GithubClient + owner string + repo string + ref string +} - githubClient, err := github.NewGithubClient(ctx, cfg.AuthInfo) +func (ci checkAllCI) Check(ctx context.Context, recheckInterval time.Duration) error { + status, err := ci.githubClient.GetCIStatus(ctx, ci.owner, ci.repo, ci.ref) if err != nil { return err } - log.Infof("Checking CI status on %s/%s@%s", ciConf.owner, ciConf.repo, ciConf.ref) + return handleCIStatus(status, recheckInterval) +} - checkSpecificCI := func() error { - var status github.CIStatus +type checkSpecificCI struct { + // same field as checkAllCI, and also a list of checks to wait for + checkAllCI + checks []string +} - status, interestingChecks, err := githubClient.GetCIStatusForChecks(ctx, ciConf.owner, ciConf.repo, ciConf.ref, ciConf.checks) - if err != nil { - return err - } +func (ci checkSpecificCI) Check(ctx context.Context, recheckInterval time.Duration) error { + var status github.CIStatus - if status == github.CIStatusFailed { - log.Infof("CI check %s failed, not waiting for other checks", strings.Join(interestingChecks, ", ")) - } + status, interestingChecks, err := ci.githubClient.GetCIStatusForChecks(ctx, ci.owner, ci.repo, ci.ref, ci.checks) + if err != nil { + return err + } - // we didn't find any failed checks, and not all checks are finished, so - // we need to recheck - if status == github.CIStatusPending { - log.Infof("CI checks are not finished yet (still waiting for %s), rechecking in %s", strings.Join(interestingChecks, ", "), cfg.recheckInterval) - } + if status == github.CIStatusFailed { + log.Infof("CI check %s failed, not waiting for other checks", strings.Join(interestingChecks, ", ")) + } - return handleCIStatus(status, cfg.recheckInterval) + // we didn't find any failed checks, and not all checks are finished, so + // we need to recheck + if status == github.CIStatusPending { + log.Infof("CI checks are not finished yet (still waiting for %s), rechecking in %s", strings.Join(interestingChecks, ", "), recheckInterval) } - checkAllCI := func() error { - status, err := githubClient.GetCIStatus(ctx, ciConf.owner, ciConf.repo, ciConf.ref) - if err != nil { - return err - } + return handleCIStatus(status, recheckInterval) +} - ret := handleCIStatus(status, cfg.recheckInterval) +func checkCIStatus(c *cli.Context, cfg *config, ciConf *ciConfig) error { + ctx := context.Background() - if ret == nil { - log.Infof("CI checks are not finished yet, rechecking in %s", cfg.recheckInterval) - } + timeoutCtx, cancel := context.WithTimeout(ctx, cfg.globalTimeout) + defer cancel() + + githubClient, err := github.NewGithubClient(ctx, cfg.AuthInfo) + if err != nil { + return err + } + + log.Infof("Checking CI status on %s/%s@%s", ciConf.owner, ciConf.repo, ciConf.ref) + + all := checkAllCI{ + githubClient: githubClient, + owner: ciConf.owner, + repo: ciConf.repo, + ref: ciConf.ref, + } - return ret + specific := checkSpecificCI{ + checkAllCI: all, + checks: ciConf.checks, } if len(ciConf.checks) > 0 { log.Infof("Checking CI status for checks: %s", strings.Join(ciConf.checks, ", ")) - return utils.RunUntilCancelledOrTimeout(timeoutCtx, checkSpecificCI, cfg.recheckInterval) + return utils.RunUntilCancelledOrTimeout(timeoutCtx, specific, cfg.recheckInterval) } - return utils.RunUntilCancelledOrTimeout(timeoutCtx, checkAllCI, cfg.recheckInterval) + return utils.RunUntilCancelledOrTimeout(timeoutCtx, all, cfg.recheckInterval) } func ciCommand(cfg *config) *cli.Command { diff --git a/cmd/wait-for-github/main.go b/cmd/wait-for-github/main.go index 13838f29..07924c8c 100644 --- a/cmd/wait-for-github/main.go +++ b/cmd/wait-for-github/main.go @@ -1,5 +1,5 @@ // wait-for-github -// Copyright (C) 2022, Grafana Labs +// Copyright (C) 2022-2023, Grafana Labs // This program is free software: you can redistribute it and/or modify it under // the terms of the GNU Affero General Public License as published by the Free diff --git a/cmd/wait-for-github/pr.go b/cmd/wait-for-github/pr.go index dd6d7438..b6344d95 100644 --- a/cmd/wait-for-github/pr.go +++ b/cmd/wait-for-github/pr.go @@ -1,5 +1,5 @@ // wait-for-github -// Copyright (C) 2022, Grafana Labs +// Copyright (C) 2022-2023, Grafana Labs // This program is free software: you can redistribute it and/or modify it under // the terms of the GNU Affero General Public License as published by the Free @@ -23,6 +23,7 @@ import ( "os" "regexp" "strconv" + "time" "github.com/grafana/wait-for-github/internal/github" "github.com/grafana/wait-for-github/internal/utils" @@ -94,6 +95,53 @@ type commitInfo struct { MergedAt int64 `json:"mergedAt"` } +type prCheck struct { + githubClient github.GithubClient + + owner string + repo string + pr int + + commitInfoFile string +} + +func (pr prCheck) Check(ctx context.Context, recheckInterval time.Duration) error { + mergedCommit, closed, mergedAt, err := pr.githubClient.IsPRMergedOrClosed(ctx, pr.owner, pr.repo, pr.pr) + if err != nil { + return err + } + + if mergedCommit != "" { + log.Info("PR is merged, exiting") + if pr.commitInfoFile != "" { + commit := commitInfo{ + Owner: pr.owner, + Repo: pr.repo, + Commit: mergedCommit, + MergedAt: mergedAt, + } + + jsonCommit, err := json.MarshalIndent(commit, "", " ") + if err != nil { + return fmt.Errorf("failed to marshal commit info to json: %w", err) + } + + log.Debugf("Writing commit info to file %s", pr.commitInfoFile) + if err := os.WriteFile(pr.commitInfoFile, jsonCommit, 0644); err != nil { + return fmt.Errorf("failed to write commit info to file: %w", err) + } + } + return cli.Exit("PR is merged", 0) + } + + if closed { + return cli.Exit("PR is closed", 1) + } + + log.Infof("PR is not closed yet, rechecking in %s", recheckInterval) + return nil +} + func checkPRMerged(c *cli.Context, cfg *config, prConf *prConfig) error { ctx := context.Background() @@ -107,41 +155,12 @@ func checkPRMerged(c *cli.Context, cfg *config, prConf *prConfig) error { commitInfoFile := c.String("commit-info-file") - checkPRMergedOrClosed := func() error { - mergedCommit, closed, mergedAt, err := githubClient.IsPRMergedOrClosed(ctx, prConf.owner, prConf.repo, prConf.pr) - if err != nil { - return err - } - - if mergedCommit != "" { - log.Info("PR is merged, exiting") - if commitInfoFile != "" { - commit := commitInfo{ - Owner: prConf.owner, - Repo: prConf.repo, - Commit: mergedCommit, - MergedAt: mergedAt, - } - - jsonCommit, err := json.MarshalIndent(commit, "", " ") - if err != nil { - return fmt.Errorf("failed to marshal commit info to json: %w", err) - } - - log.Debugf("Writing commit info to file %s", commitInfoFile) - if err := os.WriteFile(commitInfoFile, jsonCommit, 0644); err != nil { - return fmt.Errorf("failed to write commit info to file: %w", err) - } - } - return cli.Exit("PR is merged", 0) - } - - if closed { - log.Info("PR is closed, exiting") - return cli.Exit("PR is closed without being merged", 1) - } - log.Infof("PR is not closed yet, rechecking in %s", cfg.recheckInterval) - return nil + checkPRMergedOrClosed := prCheck{ + githubClient: githubClient, + owner: prConf.owner, + repo: prConf.repo, + pr: prConf.pr, + commitInfoFile: commitInfoFile, } return utils.RunUntilCancelledOrTimeout(timeoutCtx, checkPRMergedOrClosed, cfg.recheckInterval) diff --git a/cmd/wait-for-github/root.go b/cmd/wait-for-github/root.go index 055a5e61..8822ef5b 100644 --- a/cmd/wait-for-github/root.go +++ b/cmd/wait-for-github/root.go @@ -1,5 +1,5 @@ // wait-for-github -// Copyright (C) 2022, Grafana Labs +// Copyright (C) 2022-2023, Grafana Labs // This program is free software: you can redistribute it and/or modify it under // the terms of the GNU Affero General Public License as published by the Free diff --git a/internal/utils/utils.go b/internal/utils/utils.go index 8dd41257..d947160d 100644 --- a/internal/utils/utils.go +++ b/internal/utils/utils.go @@ -1,5 +1,5 @@ // wait-for-github -// Copyright (C) 2022, Grafana Labs +// Copyright (C) 2022-2023, Grafana Labs // This program is free software: you can redistribute it and/or modify it under // the terms of the GNU Affero General Public License as published by the Free @@ -28,7 +28,11 @@ import ( log "github.com/sirupsen/logrus" ) -func RunUntilCancelledOrTimeout(ctx context.Context, f func() error, interval time.Duration) error { +type Check interface { + Check(ctx context.Context, recheckInterval time.Duration) error +} + +func RunUntilCancelledOrTimeout(ctx context.Context, check Check, interval time.Duration) error { ticker := time.NewTicker(interval) defer ticker.Stop() @@ -36,7 +40,7 @@ func RunUntilCancelledOrTimeout(ctx context.Context, f func() error, interval ti signal.Notify(signalChan, syscall.SIGINT) for { - err := f() + err := check.Check(ctx, interval) if err != nil { return err } diff --git a/internal/utils/utils_test.go b/internal/utils/utils_test.go index c1dbcc33..49037e29 100644 --- a/internal/utils/utils_test.go +++ b/internal/utils/utils_test.go @@ -11,6 +11,14 @@ import ( "github.com/stretchr/testify/assert" ) +type TestCheck struct { + fn func() error +} + +func (t *TestCheck) Check(ctx context.Context, recheckInterval time.Duration) error { + return t.fn() +} + // TestCancel tests that RunUntilCancelledOrTimeout returns an error when the // context is cancelled. func TestCancel(t *testing.T) { @@ -18,10 +26,13 @@ func TestCancel(t *testing.T) { timeoutCtx, cancel := context.WithCancel(ctx) cancel() - // This should return immediately since the context is cancelled - err := RunUntilCancelledOrTimeout(timeoutCtx, func() error { - return nil - }, 1*time.Second) + check := &TestCheck{ + fn: func() error { + return nil + }, + } + + err := RunUntilCancelledOrTimeout(timeoutCtx, check, 1*time.Second) assert.Error(t, err) } @@ -35,13 +46,17 @@ func TestCalledRepeatedly(t *testing.T) { exitError := fmt.Errorf("exit error") n := 0 - err := RunUntilCancelledOrTimeout(timeoutCtx, func() error { - n++ - if n < 10 { - return nil - } - return exitError - }, 1*time.Millisecond) + check := &TestCheck{ + fn: func() error { + n++ + if n < 10 { + return nil + } + return exitError + }, + } + + err := RunUntilCancelledOrTimeout(timeoutCtx, check, 1*time.Millisecond) assert.Equal(t, 10, n) assert.Equal(t, exitError, err) @@ -52,14 +67,17 @@ func TestInterrupt(t *testing.T) { timeoutCtx, cancel := context.WithTimeout(ctx, 1*time.Second) defer cancel() - err := RunUntilCancelledOrTimeout(timeoutCtx, func() error { - // send a SIGINT to the process - p, err := os.FindProcess(os.Getpid()) - if err != nil { - return err - } - return p.Signal(syscall.SIGINT) - }, 1*time.Millisecond) + check := &TestCheck{ + fn: func() error { + p, err := os.FindProcess(os.Getpid()) + if err != nil { + return err + } + return p.Signal(syscall.SIGINT) + }, + } + + err := RunUntilCancelledOrTimeout(timeoutCtx, check, 1*time.Millisecond) assert.EqualError(t, err, "Received SIGINT") } @@ -69,9 +87,13 @@ func TestGlobalTimeout(t *testing.T) { timeoutCtx, cancel := context.WithTimeout(ctx, 0) defer cancel() - err := RunUntilCancelledOrTimeout(timeoutCtx, func() error { - return nil - }, 1*time.Millisecond) + check := &TestCheck{ + fn: func() error { + return nil + }, + } + + err := RunUntilCancelledOrTimeout(timeoutCtx, check, 1*time.Millisecond) assert.EqualError(t, err, "Timeout reached") } From aed28fd93afec1ea26a66d0de589ce397afad408 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Mon, 26 Jun 2023 19:16:06 +0100 Subject: [PATCH 3/5] Supply timeout context to each command centrally Boilerplate removal --- cmd/wait-for-github/ci.go | 13 +++---------- cmd/wait-for-github/pr.go | 37 +++++++++++++------------------------ cmd/wait-for-github/root.go | 19 +++++++++++++------ 3 files changed, 29 insertions(+), 40 deletions(-) diff --git a/cmd/wait-for-github/ci.go b/cmd/wait-for-github/ci.go index 99bbb810..6df95c58 100644 --- a/cmd/wait-for-github/ci.go +++ b/cmd/wait-for-github/ci.go @@ -77,8 +77,6 @@ func parseCIArguments(c *cli.Context) (ciConfig, error) { return ciConfig{}, nil } - log.Debugf("Will wait for CI on %s/%s@%s", owner, repo, ref) - return ciConfig{ owner: owner, repo: repo, @@ -145,13 +143,8 @@ func (ci checkSpecificCI) Check(ctx context.Context, recheckInterval time.Durati return handleCIStatus(status, recheckInterval) } -func checkCIStatus(c *cli.Context, cfg *config, ciConf *ciConfig) error { - ctx := context.Background() - - timeoutCtx, cancel := context.WithTimeout(ctx, cfg.globalTimeout) - defer cancel() - - githubClient, err := github.NewGithubClient(ctx, cfg.AuthInfo) +func checkCIStatus(timeoutCtx context.Context, cfg *config, ciConf *ciConfig) error { + githubClient, err := github.NewGithubClient(timeoutCtx, cfg.AuthInfo) if err != nil { return err } @@ -191,7 +184,7 @@ func ciCommand(cfg *config) *cli.Command { return err }, - Action: func(c *cli.Context) error { return checkCIStatus(c, cfg, &ciConf) }, + Action: func(c *cli.Context) error { return checkCIStatus(c.Context, cfg, &ciConf) }, Flags: []cli.Flag{ &cli.StringSliceFlag{ Name: "check", diff --git a/cmd/wait-for-github/pr.go b/cmd/wait-for-github/pr.go index b6344d95..73aceaef 100644 --- a/cmd/wait-for-github/pr.go +++ b/cmd/wait-for-github/pr.go @@ -35,6 +35,8 @@ type prConfig struct { owner string repo string pr int + + commitInfoFile string } var ( @@ -82,9 +84,10 @@ func parsePRArguments(c *cli.Context) (prConfig, error) { log.Infof("Waiting for PR %s/%s#%d to be merged/closed", owner, repo, n) return prConfig{ - owner: owner, - repo: repo, - pr: n, + owner: owner, + repo: repo, + pr: n, + commitInfoFile: c.String("commit-info-file"), }, nil } @@ -96,13 +99,9 @@ type commitInfo struct { } type prCheck struct { - githubClient github.GithubClient - - owner string - repo string - pr int + prConfig - commitInfoFile string + githubClient github.GithubClient } func (pr prCheck) Check(ctx context.Context, recheckInterval time.Duration) error { @@ -142,25 +141,15 @@ func (pr prCheck) Check(ctx context.Context, recheckInterval time.Duration) erro return nil } -func checkPRMerged(c *cli.Context, cfg *config, prConf *prConfig) error { - ctx := context.Background() - - timeoutCtx, cancel := context.WithTimeout(ctx, cfg.globalTimeout) - defer cancel() - - githubClient, err := github.NewGithubClient(ctx, cfg.AuthInfo) +func checkPRMerged(timeoutCtx context.Context, cfg *config, prConf *prConfig) error { + githubClient, err := github.NewGithubClient(timeoutCtx, cfg.AuthInfo) if err != nil { return err } - commitInfoFile := c.String("commit-info-file") - checkPRMergedOrClosed := prCheck{ - githubClient: githubClient, - owner: prConf.owner, - repo: prConf.repo, - pr: prConf.pr, - commitInfoFile: commitInfoFile, + githubClient: githubClient, + prConfig: *prConf, } return utils.RunUntilCancelledOrTimeout(timeoutCtx, checkPRMergedOrClosed, cfg.recheckInterval) @@ -186,6 +175,6 @@ func prCommand(cfg *config) *cli.Command { return err }, - Action: func(c *cli.Context) error { return checkPRMerged(c, cfg, &prConf) }, + Action: func(c *cli.Context) error { return checkPRMerged(c.Context, cfg, &prConf) }, } } diff --git a/cmd/wait-for-github/root.go b/cmd/wait-for-github/root.go index 8822ef5b..4ab6249e 100644 --- a/cmd/wait-for-github/root.go +++ b/cmd/wait-for-github/root.go @@ -17,6 +17,7 @@ package main import ( + "context" "fmt" "os" "strings" @@ -33,9 +34,19 @@ type cmdFunc func(cfg *config) *cli.Command func root() *cli.App { var cfg config + // give a timeout context to all commands var commands []*cli.Command for _, cf := range []cmdFunc{ciCommand, prCommand} { - commands = append(commands, cf(&cfg)) + cmd := cf(&cfg) + action := cmd.Action + cmd.Action = func(c *cli.Context) error { + timeoutCtx, cancel := context.WithTimeout(c.Context, cfg.globalTimeout) + defer cancel() + c.Context = timeoutCtx + + return action(c) + } + commands = append(commands, cmd) } return &cli.App{ @@ -91,11 +102,7 @@ func root() *cli.App { Before: func(c *cli.Context) error { var err error cfg, err = handleGlobalConfig(c) - if err != nil { - return err - } - - return nil + return err }, } } From 1619a1ca59ee0e81b74f855da2efe9c0b161d2a2 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Tue, 27 Jun 2023 12:18:39 +0100 Subject: [PATCH 4/5] github: Split the interfaces In an upcoming commit we will be testing the `ci` and `pr` subcommands. Splitting the interfaces into smaller components will make this a bit easier, since those testsuites will only have to fake what they need. --- cmd/wait-for-github/ci.go | 2 +- cmd/wait-for-github/pr.go | 2 +- internal/github/github.go | 40 ++++++++++++++++++++++++++++----------- 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/cmd/wait-for-github/ci.go b/cmd/wait-for-github/ci.go index 6df95c58..7a3d30a6 100644 --- a/cmd/wait-for-github/ci.go +++ b/cmd/wait-for-github/ci.go @@ -101,7 +101,7 @@ func handleCIStatus(status github.CIStatus, recheckInterval time.Duration) cli.E } type checkAllCI struct { - githubClient github.GithubClient + githubClient github.CheckCIStatus owner string repo string ref string diff --git a/cmd/wait-for-github/pr.go b/cmd/wait-for-github/pr.go index 73aceaef..dffac082 100644 --- a/cmd/wait-for-github/pr.go +++ b/cmd/wait-for-github/pr.go @@ -101,7 +101,7 @@ type commitInfo struct { type prCheck struct { prConfig - githubClient github.GithubClient + githubClient github.CheckPRMerged } func (pr prCheck) Check(ctx context.Context, recheckInterval time.Duration) error { diff --git a/internal/github/github.go b/internal/github/github.go index a58ae594..e837b88c 100644 --- a/internal/github/github.go +++ b/internal/github/github.go @@ -30,8 +30,11 @@ import ( log "github.com/sirupsen/logrus" ) -type GithubClient interface { +type CheckPRMerged interface { IsPRMergedOrClosed(ctx context.Context, owner, repo string, pr int) (string, bool, int64, error) +} + +type CheckCIStatus interface { GetCIStatus(ctx context.Context, owner, repo string, commitHash string) (CIStatus, error) GetCIStatusForChecks(ctx context.Context, owner, repo string, commitHash string, checkNames []string) (CIStatus, []string, error) } @@ -48,7 +51,7 @@ type GHClient struct { client *github.Client } -func NewGithubClient(ctx context.Context, authInfo AuthInfo) (GithubClient, error) { +func NewGithubClient(ctx context.Context, authInfo AuthInfo) (GHClient, error) { // If a GitHub token is provided, use it to authenticate in preference to // App authentication if authInfo.GithubToken != "" { @@ -72,7 +75,7 @@ func cachingRetryableTransport() http.RoundTripper { } // AuthenticateWithToken authenticates with a GitHub token -func AuthenticateWithToken(ctx context.Context, token string) GithubClient { +func AuthenticateWithToken(ctx context.Context, token string) GHClient { src := oauth2.StaticTokenSource( &oauth2.Token{AccessToken: token}, ) @@ -80,22 +83,22 @@ func AuthenticateWithToken(ctx context.Context, token string) GithubClient { httpClient := oauth2.NewClient(ctx, src) githubClient := github.NewClient(httpClient) - return &GHClient{client: githubClient} + return GHClient{client: githubClient} } // AuthenticateWithApp authenticates with a GitHub App -func AuthenticateWithApp(ctx context.Context, privateKey []byte, appID, installationID int64) (GithubClient, error) { +func AuthenticateWithApp(ctx context.Context, privateKey []byte, appID, installationID int64) (GHClient, error) { itr, err := ghinstallation.New(cachingRetryableTransport(), appID, installationID, privateKey) if err != nil { - return nil, fmt.Errorf("failed to create transport: %w", err) + return GHClient{}, fmt.Errorf("failed to create transport: %w", err) } githubClient := github.NewClient(&http.Client{Transport: itr}) - return &GHClient{client: githubClient}, nil + return GHClient{client: githubClient}, nil } -func (c *GHClient) IsPRMergedOrClosed(ctx context.Context, owner, repo string, prNumber int) (string, bool, int64, error) { +func (c GHClient) IsPRMergedOrClosed(ctx context.Context, owner, repo string, prNumber int) (string, bool, int64, error) { pr, _, err := c.client.PullRequests.Get(ctx, owner, repo, prNumber) if err != nil { return "", false, -1, fmt.Errorf("failed to query GitHub: %w", err) @@ -123,7 +126,22 @@ const ( CIStatusUnknown ) -func (c *GHClient) GetCIStatus(ctx context.Context, owner, repoName string, ref string) (CIStatus, error) { +func (c CIStatus) String() string { + switch c { + case CIStatusPassed: + return "passed" + case CIStatusFailed: + return "failed" + case CIStatusPending: + return "pending" + case CIStatusUnknown: + return "unknown" + default: + return "unknown" + } +} + +func (c GHClient) GetCIStatus(ctx context.Context, owner, repoName string, ref string) (CIStatus, error) { opt := &github.ListOptions{ PerPage: 100, } @@ -145,7 +163,7 @@ func (c *GHClient) GetCIStatus(ctx context.Context, owner, repoName string, ref return CIStatusUnknown, nil } -func (c *GHClient) getOneStatus(ctx context.Context, owner, repoName, ref, check string) (CIStatus, error) { +func (c GHClient) getOneStatus(ctx context.Context, owner, repoName, ref, check string) (CIStatus, error) { listOptions := github.ListOptions{ PerPage: 100, } @@ -213,7 +231,7 @@ func (c *GHClient) getOneStatus(ctx context.Context, owner, repoName, ref, check // GetCIStatusForCheck returns the CI status for a specific commit. It looks at // both 'checks' and 'statuses'. -func (c *GHClient) GetCIStatusForChecks(ctx context.Context, owner, repoName string, ref string, checkNames []string) (CIStatus, []string, error) { +func (c GHClient) GetCIStatusForChecks(ctx context.Context, owner, repoName string, ref string, checkNames []string) (CIStatus, []string, error) { allFinished := true awaitedChecks := make(map[string]bool, len(checkNames)) var status CIStatus From 760880a8415a2815e7bc87af395ce25873d66428 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Tue, 27 Jun 2023 12:19:41 +0100 Subject: [PATCH 5/5] github: Add more testcases Now we've done all that refactoring, we can start to add more testcases sensibly. Actually, this particular commit would have been more or less possible already. Test the low-level github internal module. We've aimed for as close to 100% coverage as we can get, so there are a lot of cases. --- internal/github/github_test.go | 435 ++++++++++++++++++++++++++++++++- 1 file changed, 427 insertions(+), 8 deletions(-) diff --git a/internal/github/github_test.go b/internal/github/github_test.go index 8559740b..023375b8 100644 --- a/internal/github/github_test.go +++ b/internal/github/github_test.go @@ -19,6 +19,7 @@ package github import ( "context" "net/http" + "net/url" "testing" "time" @@ -35,6 +36,8 @@ import ( // whose transport is correctly configured to use the provided token and uses a // retrying transport which itself uses a caching transport. func TestNewGithubClientWithToken(t *testing.T) { + t.Parallel() + ctx := context.Background() // Set up test data @@ -42,12 +45,9 @@ func TestNewGithubClientWithToken(t *testing.T) { GithubToken: "my-token", } - client, err := NewGithubClient(ctx, authInfo) + githubClient, err := NewGithubClient(ctx, authInfo) require.NoError(t, err) - githubClient, ok := client.(*GHClient) - require.Truef(t, ok, "Returned client is not a GHClient (is %T)", client) - if githubClient.client == nil { t.Fatal("Returned client has nil client field") } @@ -70,6 +70,8 @@ func TestNewGithubClientWithToken(t *testing.T) { // authentication and uses a retrying transport which itself uses a caching // transport. func TestNewGithubClientWithAppAuthentication(t *testing.T) { + t.Parallel() + ctx := context.Background() // Set up test data @@ -80,12 +82,9 @@ func TestNewGithubClientWithAppAuthentication(t *testing.T) { PrivateKey: []byte("-----BEGIN RSA PRIVATE KEY-----\nMC0CAQACBQD7J5Q9AgMBAAECBB6C8NkCAwD+JwIDAPz7AgMA1xcCAkoZAgMAwE8=\n-----END RSA PRIVATE KEY-----"), } - client, err := NewGithubClient(ctx, authInfo) + githubClient, err := NewGithubClient(ctx, authInfo) require.NoError(t, err) - githubClient, ok := client.(*GHClient) - require.Truef(t, ok, "Returned client is not a GHClient (is %T)", client) - if githubClient.client == nil { t.Fatal("Returned client has nil client field") } @@ -124,6 +123,8 @@ func newClientFromMock(t *testing.T, mockClient *http.Client) *GHClient { // TestResponsesAreRetried tests that responses are retried by the retrying // transport which underlies the github clients we create. func TestResponsesAreRetried(t *testing.T) { + t.Parallel() + retryCount := 0 mockClient := mock.NewMockedHTTPClient( mock.WithRequestMatchHandler( @@ -149,6 +150,8 @@ func TestResponsesAreRetried(t *testing.T) { // TestResponsesAreCached tests that responses are cached by the caching // transport which underlies the github clients we create. func TestResponsesAreCached(t *testing.T) { + t.Parallel() + hits := 0 lastStatusSent := 0 @@ -225,3 +228,419 @@ func TestResponsesAreCached(t *testing.T) { require.Equal(t, closed, closed3) require.Equal(t, mergedTs, mergedTs3) } + +func errorReturningHandler(t *testing.T, ep mock.EndpointPattern) mock.MockBackendOption { + t.Helper() + + return mock.WithRequestMatchHandler( + mock.GetReposCommitsCheckRunsByOwnerByRepoByRef, + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mock.WriteError( + w, + http.StatusInternalServerError, + "Internal Server Error", + ) + }), + ) +} + +func newErrorReturningClient(t *testing.T) *GHClient { + t.Helper() + + return newClientFromMock(t, mock.NewMockedHTTPClient( + errorReturningHandler(t, mock.GetReposCommitsCheckRunsByOwnerByRepoByRef), + )) +} + +func TestIsPRMergedOrClosed_Success(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + mockedHTTPClient := mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetReposPullsByOwnerByRepoByPullNumber, + github.PullRequest{ + Number: github.Int(1), + State: github.String("closed"), + Merged: github.Bool(true), + MergedAt: &github.Timestamp{Time: time.Now()}, + MergeCommitSHA: github.String("abcdef12345"), + }, + ), + ) + + ghClient := newClientFromMock(t, mockedHTTPClient) + sha, closed, mergedAt, err := ghClient.IsPRMergedOrClosed(ctx, "owner", "repo", 1) + + require.NoError(t, err) + require.Equal(t, "abcdef12345", sha) + require.True(t, closed) + // As time is not frozen, we only check that mergedAt is not zero + require.NotZero(t, mergedAt) +} + +func TestIsPRMergedOrClosed_Error(t *testing.T) { + t.Parallel() + + ctx := context.Background() + ghClient := newErrorReturningClient(t) + _, _, _, err := ghClient.IsPRMergedOrClosed(ctx, "owner", "repo", 1) + require.Error(t, err) +} + +func TestGetCIStatus(t *testing.T) { + tests := []struct { + name string + status *github.CombinedStatus + expectedStatus CIStatus + }{ + { + name: "success", + status: &github.CombinedStatus{ + State: github.String("success"), + }, + expectedStatus: CIStatusPassed, + }, + { + name: "pending", + status: &github.CombinedStatus{ + State: github.String("pending"), + }, + expectedStatus: CIStatusPending, + }, + { + name: "failure", + status: &github.CombinedStatus{ + State: github.String("failure"), + }, + expectedStatus: CIStatusFailed, + }, + { + name: "unknown", + status: &github.CombinedStatus{ + State: github.String("unknown"), + }, + expectedStatus: CIStatusUnknown, + }, + } + + for _, tt := range tests { + tt := tt + + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + mockedHTTPClient := mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetReposCommitsStatusByOwnerByRepoByRef, + tt.status, + ), + ) + + ghClient := newClientFromMock(t, mockedHTTPClient) + result, err := ghClient.GetCIStatus(ctx, "owner", "repo", "abcdef12345") + + require.NoError(t, err) + require.Equalf(t, tt.expectedStatus, result, "expected %s, got %s", tt.expectedStatus, result) + }) + } +} + +func TestGetCIStatus_Error(t *testing.T) { + t.Parallel() + + ctx := context.Background() + ghClient := newErrorReturningClient(t) + _, err := ghClient.GetCIStatus(ctx, "owner", "repo", "abcdef12345") + require.Error(t, err) +} + +func TestGetCIStatusForChecks(t *testing.T) { + tests := []struct { + name string + checksToLookFor []string + mockCheckRuns []*github.CheckRun + mockRepoStatus []*github.RepoStatus + expectedStatus CIStatus + expectedAwait []string + }{ + { + name: "Single check - completed with success", + checksToLookFor: []string{"check1"}, + mockCheckRuns: []*github.CheckRun{ + { + Name: github.String("check1"), + Status: github.String("completed"), + Conclusion: github.String("success"), + }, + }, + expectedStatus: CIStatusPassed, + expectedAwait: nil, + }, + { + name: "Single check - completed with failure", + checksToLookFor: []string{"check1"}, + mockCheckRuns: []*github.CheckRun{ + { + Name: github.String("check1"), + Status: github.String("completed"), + Conclusion: github.String("failure"), + }, + }, + expectedStatus: CIStatusFailed, + expectedAwait: []string{"check1"}, + }, + { + name: "Single check - queued", + checksToLookFor: []string{"check1"}, + mockCheckRuns: []*github.CheckRun{ + { + Name: github.String("check1"), + Status: github.String("queued"), + }, + }, + expectedStatus: CIStatusPending, + expectedAwait: []string{"check1"}, + }, + { + name: "Single check - in progress", + checksToLookFor: []string{"check1"}, + mockCheckRuns: []*github.CheckRun{ + { + Name: github.String("check1"), + Status: github.String("in_progress"), + }, + }, + expectedStatus: CIStatusPending, + expectedAwait: []string{"check1"}, + }, + { + name: "Single check - skipped", + checksToLookFor: []string{"check1"}, + mockCheckRuns: []*github.CheckRun{ + { + Name: github.String("check1"), + Conclusion: github.String("skipped"), + Status: github.String("completed"), + }, + }, + expectedStatus: CIStatusPassed, + expectedAwait: nil, + }, + { + name: "Single check - doesn't exist, status success", + checksToLookFor: []string{"check1"}, + mockRepoStatus: []*github.RepoStatus{ + { + Context: github.String("check1"), + State: github.String("success"), + }, + }, + expectedStatus: CIStatusPassed, + expectedAwait: nil, + }, + { + name: "Single check - doesn't exist, status success, multiple statuses returned", + checksToLookFor: []string{"check1"}, + mockRepoStatus: []*github.RepoStatus{ + { + Context: github.String("what"), + State: github.String("failed"), + }, + { + Context: github.String("check1"), + State: github.String("success"), + }, + }, + expectedStatus: CIStatusPassed, + expectedAwait: nil, + }, + { + name: "Single check - doesn't exist, status failure", + checksToLookFor: []string{"check1"}, + mockRepoStatus: []*github.RepoStatus{ + { + Context: github.String("check1"), + State: github.String("failure"), + }, + }, + expectedStatus: CIStatusFailed, + expectedAwait: []string{"check1"}, + }, + { + name: "Single check - doesn't exist, status pending", + checksToLookFor: []string{"check1"}, + mockRepoStatus: []*github.RepoStatus{ + { + Context: github.String("check1"), + State: github.String("pending"), + }, + }, + expectedStatus: CIStatusPending, + expectedAwait: []string{"check1"}, + }, + { + name: "Single check - doesn't exist, status error", + checksToLookFor: []string{"check1"}, + mockRepoStatus: []*github.RepoStatus{ + { + Context: github.String("check1"), + State: github.String("error"), + }, + }, + expectedStatus: CIStatusFailed, + expectedAwait: []string{"check1"}, + }, + { + name: "Multiple checks - all successful", + checksToLookFor: []string{"check1", "check2"}, + mockCheckRuns: []*github.CheckRun{ + { + Name: github.String("check1"), + Status: github.String("completed"), + Conclusion: github.String("success"), + }, + { + Name: github.String("check2"), + Status: github.String("completed"), + Conclusion: github.String("success"), + }, + }, + expectedStatus: CIStatusPassed, + expectedAwait: nil, + }, + { + name: "Multiple checks - one failed", + checksToLookFor: []string{"check1", "check2"}, + mockCheckRuns: []*github.CheckRun{ + { + Name: github.String("check1"), + Status: github.String("completed"), + Conclusion: github.String("success"), + }, + { + Name: github.String("check2"), + Status: github.String("completed"), + Conclusion: github.String("failure"), + }, + }, + expectedStatus: CIStatusFailed, + expectedAwait: []string{"check2"}, + }, + { + name: "Multiple checks - one pending", + checksToLookFor: []string{"check1", "check2"}, + mockCheckRuns: []*github.CheckRun{ + { + Name: github.String("check1"), + Status: github.String("completed"), + Conclusion: github.String("success"), + }, + { + Name: github.String("check2"), + Status: github.String("queued"), + }, + }, + expectedStatus: CIStatusPending, + expectedAwait: []string{"check2"}, + }, + { + name: "Check not returned at all - unknown - converted to pending", + checksToLookFor: []string{"check1"}, + expectedStatus: CIStatusPending, + expectedAwait: []string{"check1"}, + }, + } + + for _, tt := range tests { + tt := tt + + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + mockedHTTPClient := mock.NewMockedHTTPClient( + mock.WithRequestMatchHandler( + mock.GetReposCommitsCheckRunsByOwnerByRepoByRef, + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // parse the request URI to get the check name. only + // return matching ones. + uri, err := url.ParseRequestURI(r.RequestURI) + require.NoError(t, err) + checkName := uri.Query().Get("check_name") + + var runs []*github.CheckRun + if checkName == "" { + runs = tt.mockCheckRuns + } else { + for _, run := range tt.mockCheckRuns { + if *run.Name == checkName { + runs = append(runs, run) + } + } + } + + _, err = w.Write( + mock.MustMarshal( + github.ListCheckRunsResults{ + Total: github.Int(len(runs)), + CheckRuns: runs, + }, + ), + ) + + require.NoError(t, err) + }), + ), + mock.WithRequestMatch( + mock.GetReposCommitsStatusesByOwnerByRepoByRef, + tt.mockRepoStatus, + ), + ) + + ghClient := newClientFromMock(t, mockedHTTPClient) + status, awaiting, err := ghClient.GetCIStatusForChecks(ctx, "owner", "repo", "abcdef12345", tt.checksToLookFor) + + require.NoError(t, err) + require.Equalf(t, tt.expectedStatus, status, "expected status %s, got %s", tt.expectedStatus, status) + require.ElementsMatch(t, tt.expectedAwait, awaiting) + }) + } +} + +func TestGetCIStatusForChecks_Error(t *testing.T) { + t.Parallel() + + ctx := context.Background() + ghClient := newErrorReturningClient(t) + _, _, err := ghClient.GetCIStatusForChecks(ctx, "owner", "repo", "abcdef12345", []string{"check1"}) + require.Error(t, err) +} + +// first call returns okay, second call returns error +func TestGetCIStatusForChecks_ErrorListStatuses(t *testing.T) { + t.Parallel() + + ctx := context.Background() + + mockedHTTPClient := mock.NewMockedHTTPClient( + mock.WithRequestMatch( + mock.GetReposCommitsCheckRunsByOwnerByRepoByRef, + github.ListCheckRunsResults{ + Total: github.Int(0), + CheckRuns: []*github.CheckRun{}, + }, + ), + errorReturningHandler(t, mock.GetReposCommitsStatusesByOwnerByRepoByRef), + ) + + ghClient := newClientFromMock(t, mockedHTTPClient) + + _, _, err := ghClient.GetCIStatusForChecks(ctx, "owner", "repo", "abcdef12345", []string{"check1"}) + require.Error(t, err) + require.ErrorContains(t, err, "failed to query GitHub") +}