From cdc390659c50a7671991bdffe8c8caee106834db Mon Sep 17 00:00:00 2001 From: Geoffrey Wiseman Date: Mon, 14 Sep 2026 21:24:39 -0400 Subject: [PATCH 1/3] Adding Progress Reporting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At times it felt like the extension was stalled simply because it was fetching a lot of data. Adding some progress reporting so it’s easier to tell when it’s working. There are still some places where it might go silent for a while, but it’s less frequent. [closes #59] Signed-off-by: Geoffrey Wiseman --- README.md | 4 + client/client.go | 40 ++++---- client/client_test.go | 12 +-- client/transport.go | 7 +- main.go | 17 +++- progress.go | 96 ++++++++++++++++++++ progress_test.go | 121 ++++++++++++++++++++++++ usage/collector.go | 133 +++++++++++++++++++++++---- usage/collector_test.go | 197 +++++++++++++++++++++++++++++++++++++--- 9 files changed, 562 insertions(+), 65 deletions(-) create mode 100644 progress.go create mode 100644 progress_test.go diff --git a/README.md b/README.md index 0b0e508..3ae3921 100644 --- a/README.md +++ b/README.md @@ -29,6 +29,10 @@ How it works, and what that means for the numbers: its requests to stay within GitHub's rate limits, waits and retries when GitHub asks it to, and refuses up front if the remaining API budget looks too small for the repositories requested. That check is an estimate, since the number of jobs per commit is not known until they are fetched. +- Large collections say what they are doing on stderr: listing many repositories, with a running count; then the + number of commits, API calls and rough time the fetch will take, with a running count of commits fetched. On a + terminal the count is redrawn in place; elsewhere a line is printed every tenth. The report itself goes to stdout, + so `--output=tsv` stays clean. I wrote a version of this extension before the Golang support was available for `gh`, which is still available [here](https://github.com/geoffreywiseman/gh-actuse). diff --git a/client/client.go b/client/client.go index 3a4978f..1de9125 100644 --- a/client/client.go +++ b/client/client.go @@ -62,23 +62,20 @@ func (e UnexpectedHostError) Error() string { return "Unexpected host: " + string(e) } -// GetWorkflows returns a slice of Workflow instances, one for each workflow in the repository +// GetWorkflows returns a slice of Workflow instances, one for each workflow in the repository. Pages are the +// largest GitHub allows, and a short page ends the listing without a further call. func (c *Client) GetWorkflows(repository Repository) ([]Workflow, error) { - var page uint8 = 1 var workflows = make([]Workflow, 0) - - for { + for page := 1; ; page++ { wfp, err := c.getWorkflowPage(repository, page) if err != nil { return nil, err } - if len(wfp) == 0 { - break - } workflows = append(workflows, wfp...) - page++ + if len(wfp) < perPage { + return workflows, nil + } } - return workflows, nil } type workflowPage struct { @@ -86,9 +83,9 @@ type workflowPage struct { TotalCount uint64 `json:"total_count"` } -func (c *Client) getWorkflowPage(repository Repository, page uint8) ([]Workflow, error) { +func (c *Client) getWorkflowPage(repository Repository, page int) ([]Workflow, error) { response := workflowPage{} - url := fmt.Sprintf("repos/%s/actions/workflows?page=%d", repository.FullName, page) + url := fmt.Sprintf("repos/%s/actions/workflows?per_page=%d&page=%d", repository.FullName, perPage, page) err := c.Rest.Get(url, &response) if err != nil { return nil, fmt.Errorf("could not get workflow page: %w", err) @@ -157,12 +154,11 @@ func (c *Client) GetUser(name string) (*User, error) { return &response, nil } -// GetAllRepositories returns a list of repositories for the specified user +// GetAllRepositories returns a list of repositories for the specified user. Pages are the largest GitHub +// allows, and a short page ends the listing without a further call. func (c *Client) GetAllRepositories(user *User) ([]*Repository, error) { - var page uint8 = 1 var repos = make([]*Repository, 0) - - for { + for page := 1; ; page++ { path, err := c.getAllRepositoriesPath(user, page) if err != nil { return nil, err @@ -171,21 +167,19 @@ func (c *Client) GetAllRepositories(user *User) ([]*Repository, error) { if err != nil { return nil, err } - if len(rp) == 0 { - break - } repos = append(repos, rp...) - page++ + if len(rp) < perPage { + return repos, nil + } } - return repos, nil } -func (c *Client) getAllRepositoriesPath(user *User, page uint8) (string, error) { +func (c *Client) getAllRepositoriesPath(user *User, page int) (string, error) { switch user.Type { case "Organization": - return fmt.Sprintf("orgs/%s/repos?page=%d", user.Login, page), nil + return fmt.Sprintf("orgs/%s/repos?per_page=%d&page=%d", user.Login, perPage, page), nil case "User": - return fmt.Sprintf("users/%s/repos?page=%d", user.Login, page), nil + return fmt.Sprintf("users/%s/repos?per_page=%d&page=%d", user.Login, perPage, page), nil default: return "", UnexpectedUserTypeError(user.Type) } diff --git a/client/client_test.go b/client/client_test.go index 68f789d..a3299cc 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -82,19 +82,13 @@ func TestClient_GetWorkflows(t *testing.T) { // Given rest, client := getTestClient() repo := Repository{ID: 1, Name: "gh-actions-usage", FullName: testRepoFullName} - rest.On("Get", "repos/"+testRepoFullName+"/actions/workflows?page=1", mock.Anything). + rest.On("Get", "repos/"+testRepoFullName+"/actions/workflows?per_page=100&page=1", mock.Anything). Return(nil). Run(func(args mock.Arguments) { wp := args.Get(1).(*workflowPage) wp.Workflows = append(wp.Workflows, Workflow{ID: 1, Name: "Build", Path: ".github/workflows/build.yml", State: "active"}) wp.TotalCount = 1 }) - rest.On("Get", "repos/"+testRepoFullName+"/actions/workflows?page=2", mock.Anything). - Return(nil). - Run(func(args mock.Arguments) { - wp := args.Get(1).(*workflowPage) - wp.TotalCount = 0 - }) // When repos, err := client.GetWorkflows(repo) @@ -153,14 +147,12 @@ func TestClient_GetUser_NotFound(t *testing.T) { func TestClient_GetAllRepositories(t *testing.T) { // Given rest, client := getTestClient() - rest.On("Get", "users/geoffreywiseman/repos?page=1", mock.Anything). + rest.On("Get", "users/geoffreywiseman/repos?per_page=100&page=1", mock.Anything). Return(nil). Run(func(args mock.Arguments) { ars := args.Get(1).(*[]*Repository) *ars = append(*ars, &Repository{ID: 427462569, Name: "gh-actuse", FullName: "geoffreywiseman/gh-actuse"}) }) - rest.On("Get", "users/geoffreywiseman/repos?page=2", mock.Anything). - Return(nil) //. owner := &User{ID: 49935, Login: "geoffreywiseman", Type: "User"} // When diff --git a/client/transport.go b/client/transport.go index b42fd1b..86b467d 100644 --- a/client/transport.go +++ b/client/transport.go @@ -14,8 +14,9 @@ import ( ) const ( - // requestsPerSecond keeps the extension under GitHub's secondary limit of roughly 900 REST points per minute. - requestsPerSecond = 10 + // RequestsPerSecond keeps the extension under GitHub's secondary limit of roughly 900 REST points per minute. + // It is exported so that callers can estimate how long a known number of requests will take. + RequestsPerSecond = 10 // requestBurst is the number of requests that may be issued back-to-back before the limiter starts pacing. requestBurst = 10 // maxAttempts is the total number of times a request will be tried when GitHub reports it as rate limited. @@ -47,7 +48,7 @@ type throttle struct { func newThrottle(next http.RoundTripper) *throttle { return &throttle{ next: next, - limiter: rate.NewLimiter(requestsPerSecond, requestBurst), + limiter: rate.NewLimiter(RequestsPerSecond, requestBurst), now: time.Now, sleep: sleepContext, notify: notifyStderr, diff --git a/main.go b/main.go index f1c8b6e..f91bf7c 100644 --- a/main.go +++ b/main.go @@ -13,6 +13,7 @@ import ( "time" gogherrors "github.com/cli/go-gh/v2/pkg/api" + "github.com/cli/go-gh/v2/pkg/term" "github.com/geoffreywiseman/gh-actions-usage/client" "github.com/geoffreywiseman/gh-actions-usage/format" "github.com/geoffreywiseman/gh-actions-usage/usage" @@ -20,6 +21,9 @@ import ( var gh client.Client +// status is where progress of a collection is reported, apart from the report itself on stdout. +var status = &statusWriter{Writer: os.Stderr} + // monthLayout is the format of the --month flag, a year and month such as 2026-09. const monthLayout = "2006-01" @@ -131,6 +135,7 @@ func tryDisplayCurrentRepo(cfg config) { printError(cfg, "Error collecting usage", err) return } + status.separate() if cfg.skip { skipUnused(repoFlowUsage) } @@ -153,6 +158,7 @@ func tryDisplayAllSpecified(cfg config, targets []string) { printError(cfg, "Error collecting usage", err) return } + status.separate() if cfg.skip { skipUnused(repoFlowUsage) } @@ -187,15 +193,19 @@ func parseMonth(value string, now time.Time) (time.Time, error) { return month, nil } -// collectUsage computes the usage of each repository's workflows for the configured billing period. +// collectUsage computes the usage of each repository's workflows for the configured billing period. Progress +// goes to stderr, so that the report on stdout stays clean for machine-readable formats. func collectUsage(cfg config, repos []*client.Repository) (client.RepoUsage, error) { + terminal := term.IsTerminal(os.Stderr) collector := usage.Collector{ API: &gh, From: cfg.from, To: cfg.to, Notify: func(message string) { - _, _ = fmt.Fprintln(os.Stderr, message) + _, _ = fmt.Fprintln(status, message) }, + ListingProgress: newProgressReporter(status, terminal, "repositories listed").update, + FetchProgress: newProgressReporter(status, terminal, "commits fetched").update, } return collector.Collect(context.Background(), repos) //nolint:wrapcheck // caller prints the error as-is } @@ -297,6 +307,9 @@ func mapOwner(repos repoMap, userName string) error { list = make([]*client.Repository, 0) } + // An owner with thousands of repositories takes a while to page through; say so on stderr, where the + // collector's progress goes too. + _, _ = fmt.Fprintf(status, "Listing repositories of %s...\n", user.Login) ors, err := gh.GetAllRepositories(user) if err != nil { return fmt.Errorf("could not get repositories: %w", err) diff --git a/progress.go b/progress.go new file mode 100644 index 0000000..2188bf5 --- /dev/null +++ b/progress.go @@ -0,0 +1,96 @@ +package main + +import ( + "fmt" + "io" + "time" +) + +const ( + // progressInterval is the shortest time between rewrites of the progress line on a terminal, so that the + // counter never becomes the bottleneck of a collection. + progressInterval = 250 * time.Millisecond + // progressSteps is the number of lines a collection prints when stderr is not a terminal: one per tenth. + progressSteps = 10 + percent = 100 +) + +// statusWriter is the writer for everything said about a collection's progress on stderr. It remembers whether +// anything was said, so that the report can be set apart from it by a blank line only when there is something +// to set it apart from. +type statusWriter struct { + io.Writer + + written bool +} + +func (s *statusWriter) Write(p []byte) (int, error) { + if len(p) > 0 { + s.written = true + } + return s.Writer.Write(p) //nolint:wrapcheck // a plain pass-through +} + +// separate ends the progress output with a blank line if there was any, and forgets it, so that a later +// collection starts afresh. +func (s *statusWriter) separate() { + if s.written { + _, _ = fmt.Fprintln(s) + s.written = false + } +} + +// progressReporter renders the collector's commit counter on stderr. On a terminal the line is rewritten in +// place a few times a second; anywhere else, such as a log or a pipe, a line is printed each time another tenth +// of the total completes, so the output stays readable without a carriage return. +type progressReporter struct { + w io.Writer + terminal bool + what string // what is counted, past tense, such as "commits fetched" + now func() time.Time + last time.Time // when the line was last rewritten on a terminal + shown uint // tenths of the total already printed off a terminal +} + +func newProgressReporter(w io.Writer, terminal bool, what string) *progressReporter { + return &progressReporter{w: w, terminal: terminal, what: what, now: time.Now} +} + +// update reports done of total; the collector calls it once per item. +func (p *progressReporter) update(done, total uint) { + if total == 0 { + return + } + if p.terminal { + p.rewrite(done, total) + } else { + p.step(done, total) + } +} + +func (p *progressReporter) rewrite(done, total uint) { + now := p.now() + if done < total && now.Sub(p.last) < progressInterval { + return + } + p.last = now + _, _ = fmt.Fprintf(p.w, "\r%s", p.line(done, total)) + if done == total { + _, _ = fmt.Fprintln(p.w) + } +} + +func (p *progressReporter) step(done, total uint) { + tenths := done * progressSteps / total + if tenths <= p.shown { + return + } + p.shown = tenths + _, _ = fmt.Fprintln(p.w, p.line(done, total)) +} + +// line describes the progress so far; its length never shrinks as done grows, so rewriting it in place leaves +// nothing behind. +func (p *progressReporter) line(done, total uint) string { + return fmt.Sprintf("%d of %d %s (%d%%)...", done, total, p.what, done*percent/total) +} diff --git a/progress_test.go b/progress_test.go new file mode 100644 index 0000000..f39a8b7 --- /dev/null +++ b/progress_test.go @@ -0,0 +1,121 @@ +package main + +import ( + "bytes" + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +// clock is a fake time source that only moves when told to. +type clock struct{ now time.Time } + +func (c *clock) tick(d time.Duration) { c.now = c.now.Add(d) } + +func newClockedReporter(out *bytes.Buffer, terminal bool) (*progressReporter, *clock) { + c := &clock{now: time.Date(2026, time.September, 13, 9, 0, 0, 0, time.UTC)} + reporter := newProgressReporter(out, terminal, "commits fetched") + reporter.now = func() time.Time { return c.now } + return reporter, c +} + +func TestProgressReporter_Terminal_RewritesInPlaceAFewTimesASecond(t *testing.T) { + // Given + var out bytes.Buffer + reporter, clock := newClockedReporter(&out, true) + + // When: four commits complete in quick succession, then one after a pause + reporter.update(1, 200) + clock.tick(progressInterval / 2) + reporter.update(2, 200) + reporter.update(3, 200) + clock.tick(progressInterval / 2) + reporter.update(4, 200) + clock.tick(progressInterval) + reporter.update(5, 200) + + // Then: the first and the ones after each interval are drawn, the ones in between are dropped + assert.Equal(t, "\r1 of 200 commits fetched (0%)...\r4 of 200 commits fetched (2%)...\r5 of 200 commits fetched (2%)...", out.String()) +} + +func TestProgressReporter_Terminal_AlwaysEndsTheLine(t *testing.T) { + // Given + var out bytes.Buffer + reporter, _ := newClockedReporter(&out, true) + + // When: the last commit completes within the interval of the previous rewrite + reporter.update(1, 2) + reporter.update(2, 2) + + // Then: the final count is drawn regardless, and the line is ended + assert.Equal(t, "\r1 of 2 commits fetched (50%)...\r2 of 2 commits fetched (100%)...\n", out.String()) +} + +func TestProgressReporter_Log_PrintsALineEveryTenth(t *testing.T) { + // Given + var out bytes.Buffer + reporter, _ := newClockedReporter(&out, false) + + // When + for done := uint(1); done <= 25; done++ { + reporter.update(done, 25) + } + + // Then: nothing at zero, a line at each tenth, and the last line is the total + assert.Equal(t, "3 of 25 commits fetched (12%)...\n"+ + "5 of 25 commits fetched (20%)...\n"+ + "8 of 25 commits fetched (32%)...\n"+ + "10 of 25 commits fetched (40%)...\n"+ + "13 of 25 commits fetched (52%)...\n"+ + "15 of 25 commits fetched (60%)...\n"+ + "18 of 25 commits fetched (72%)...\n"+ + "20 of 25 commits fetched (80%)...\n"+ + "23 of 25 commits fetched (92%)...\n"+ + "25 of 25 commits fetched (100%)...\n", out.String()) +} + +func TestProgressReporter_Log_SkipsTenthsAtOnceWhenUpdatesAreSparse(t *testing.T) { + // Given + var out bytes.Buffer + reporter, _ := newClockedReporter(&out, false) + + // When + reporter.update(7, 10) + reporter.update(8, 10) + + // Then: one line for the jump, then one for the next tenth + assert.Equal(t, "7 of 10 commits fetched (70%)...\n8 of 10 commits fetched (80%)...\n", out.String()) +} + +func TestProgressReporter_IgnoresAnEmptyTotal(t *testing.T) { + // Given + var out bytes.Buffer + reporter, _ := newClockedReporter(&out, false) + + // When + reporter.update(0, 0) + + // Then + assert.Empty(t, out.String()) +} + +func TestStatusWriter_SeparatesOnlyAfterOutput(t *testing.T) { + // Given + var out bytes.Buffer + status := &statusWriter{Writer: &out} + + // When: nothing has been written + status.separate() + + // Then + assert.Empty(t, out.String()) + + // When: something has been written, and then separated twice + _, _ = status.Write([]byte("Listing...\n")) + status.separate() + status.separate() + + // Then: one blank line follows it + assert.Equal(t, "Listing...\n\n", out.String()) +} diff --git a/usage/collector.go b/usage/collector.go index cf2de7b..2ed5dff 100644 --- a/usage/collector.go +++ b/usage/collector.go @@ -21,7 +21,14 @@ const ( // extra pages of commits that carry more than a hundred jobs. Job counts are unknown before fetching, so // this is an estimate rather than a bound; if it proves short, the transport waits for the limit to reset. callsPerCommit = 1.3 - // progressThreshold is the number of commits above which a repository's collection is announced. + // callsPerRepository is the estimated number of API calls needed to list a repository's workflows and runs: + // one page of each. Busy repositories need more pages of runs, and the fetch is preflighted separately. + callsPerRepository = 2 + // listingThreshold is the number of repositories above which the listing phase is announced, since each + // repository costs a few paced calls before anything else can be said about the collection. + listingThreshold = 10 + // progressThreshold is the number of commits, across all repositories, above which the fetch of job durations + // is announced and its progress reported. progressThreshold = 20 // statusCompleted and conclusionSkipped are the check-run states that decide whether a job's time counts. statusCompleted = "completed" @@ -57,8 +64,16 @@ type Collector struct { To time.Time // Concurrency is the number of check-run requests kept in flight at once; zero means defaultConcurrency. Concurrency int - // Notify, if set, receives progress messages for long collections. + // Notify, if set, receives a message as each phase of a long collection begins: listing many repositories, + // and fetching job durations for many commits, with the size of the job ahead. Notify func(message string) + // ListingProgress, if set, receives the number of repositories listed so far against the total, as each + // repository of a long collection is listed. + ListingProgress func(done, total uint) + // FetchProgress, if set, receives the number of commits whose job durations have been fetched so far, against + // the total across all repositories, as each commit of a long collection completes. It is called from the fetch + // goroutines, but never concurrently, and always ends with done equal to total unless the fetch fails. + FetchProgress func(done, total uint) } // Period returns the bounds of the metered billing period containing the instant month, which is the calendar @@ -85,6 +100,15 @@ type repoPlan struct { // Collect returns the usage of every workflow in each repository for the collector's period. func (c *Collector) Collect(ctx context.Context, repos []*client.Repository) (client.RepoUsage, error) { + var listing *progress + if len(repos) > listingThreshold { + err := c.preflight(uint(len(repos)) * callsPerRepository) + if err != nil { + return nil, err + } + c.notify(fmt.Sprintf("Listing workflows and runs for %d repositories...", len(repos))) + listing = &progress{total: uint(len(repos)), report: c.ListingProgress} + } plans := make([]*repoPlan, 0, len(repos)) var commits uint for _, repo := range repos { @@ -94,16 +118,26 @@ func (c *Collector) Collect(ctx context.Context, repos []*client.Repository) (cl } plans = append(plans, plan) commits += uint(len(plan.commits)) + listing.advance() } - err := c.preflight(commits) - if err != nil { - return nil, err + needed := callsNeeded(commits) + if commits > 0 { + err := c.preflight(needed) + if err != nil { + return nil, err + } + } + + var fetching *progress + if commits > progressThreshold { + c.notify(forewarning(plans, commits, needed)) + fetching = &progress{total: commits, report: c.FetchProgress} } result := make(client.RepoUsage, len(plans)) for _, plan := range plans { - err = c.fetchDurations(ctx, plan) + err := c.fetchDurations(ctx, plan, fetching) if err != nil { return nil, err } @@ -112,6 +146,12 @@ func (c *Collector) Collect(ctx context.Context, repos []*client.Repository) (cl return result, nil } +func (c *Collector) notify(message string) { + if c.Notify != nil { + c.Notify(message) + } +} + // plan lists a repository's workflows and the runs in the period, seeding every workflow at zero usage and // mapping each run's check suite to its workflow. func (c *Collector) plan(repo *client.Repository) (*repoPlan, error) { @@ -156,27 +196,87 @@ func deletedWorkflow(run client.WorkflowRun) client.Workflow { return client.Workflow{ID: run.WorkflowID, Name: path.Base(run.Path), Path: run.Path, State: deletedWorkflowState} } -// preflight checks that the remaining API budget covers the check-run calls the collection will need. -func (c *Collector) preflight(commits uint) error { - if commits == 0 { - return nil - } +// callsNeeded estimates the API calls that fetching the job durations of the given number of commits will take. +func callsNeeded(commits uint) uint { + return uint(math.Ceil(float64(commits) * callsPerCommit)) +} + +// preflight checks that the remaining API budget covers the calls the next phase of the collection will need. +func (c *Collector) preflight(needed uint) error { limit, err := c.API.GetRateLimit() if err != nil { return fmt.Errorf("checking rate limit: %w", err) } - needed := uint(math.Ceil(float64(commits) * callsPerCommit)) if needed > limit.Remaining { return RateLimitError{Needed: needed, Remaining: limit.Remaining, Reset: limit.ResetAt()} } return nil } -// fetchDurations fetches the check runs of every commit in the plan and adds each job's duration to its workflow. -func (c *Collector) fetchDurations(ctx context.Context, plan *repoPlan) error { - if len(plan.commits) > progressThreshold && c.Notify != nil { - c.Notify(fmt.Sprintf("%s: fetching job durations for %d commits...", plan.repo.FullName, len(plan.commits))) +// forewarning describes the fetch about to begin: how many commits it covers, in which or how many repositories, +// and roughly how many API calls and how long that will take at the transport's pacing rate. The duration is a +// lower bound, since a secondary rate limit can pause the fetch, but the transport announces those pauses itself. +func forewarning(plans []*repoPlan, commits, needed uint) string { + var busy []*repoPlan + for _, plan := range plans { + if len(plan.commits) > 0 { + busy = append(busy, plan) + } + } + where := fmt.Sprintf("%d repositories", len(busy)) + if len(busy) == 1 { + where = busy[0].repo.FullName + } + estimate := time.Duration(float64(needed) / client.RequestsPerSecond * float64(time.Second)) + return fmt.Sprintf("Fetching job durations for %d commits in %s (about %d API calls, %s)...", + commits, where, needed, roughly(estimate)) +} + +// roughly renders a duration estimate in the coarsest unit that still says something: seconds under a minute, +// minutes under an hour, and hours and minutes beyond that. +func roughly(d time.Duration) string { + switch { + case d < time.Minute: + return plural(int(d.Round(time.Second)/time.Second), "second") + case d < time.Hour: + return plural(int(d.Round(time.Minute)/time.Minute), "minute") + default: + d = d.Round(time.Minute) + hours := plural(int(d/time.Hour), "hour") + if minutes := int(d % time.Hour / time.Minute); minutes > 0 { + return hours + " " + plural(minutes, "minute") + } + return hours + } +} + +func plural(n int, unit string) string { + if n == 1 { + return "1 " + unit } + return fmt.Sprintf("%d %ss", n, unit) +} + +// progress counts the commits fetched so far across every repository of a collection and reports each one. +// A nil progress counts nothing, for collections too small to be worth reporting. +type progress struct { + done, total uint + report func(done, total uint) +} + +// advance records one more fetched commit. Callers serialize it, so the report never runs concurrently. +func (p *progress) advance() { + if p == nil { + return + } + p.done++ + if p.report != nil { + p.report(p.done, p.total) + } +} + +// fetchDurations fetches the check runs of every commit in the plan and adds each job's duration to its workflow. +func (c *Collector) fetchDurations(ctx context.Context, plan *repoPlan, tracker *progress) error { var mu sync.Mutex group, ctx := errgroup.WithContext(ctx) group.SetLimit(c.concurrency()) @@ -199,6 +299,7 @@ func (c *Collector) fetchDurations(ctx context.Context, plan *repoPlan) error { plan.usage[workflow] += duration(checkRun) } } + tracker.advance() return nil }) } diff --git a/usage/collector_test.go b/usage/collector_test.go index 266a0e9..ebde57a 100644 --- a/usage/collector_test.go +++ b/usage/collector_test.go @@ -3,6 +3,7 @@ package usage import ( "context" "errors" + "fmt" "testing" "time" @@ -251,27 +252,201 @@ func TestCollector_PreflightsAllRepositoriesTogether(t *testing.T) { api.AssertNumberOfCalls(t, "GetRateLimit", 1) } -func TestCollector_AnnouncesLargeCollections(t *testing.T) { - // Given - api := new(apiMock) - runs := make([]client.WorkflowRun, 0, progressThreshold+1) - for i := range progressThreshold + 1 { - sha := string(rune('a' + i)) +// busyRepo expects the listing of a repository with the given number of commits, each with an empty fetch. +func busyRepo(api *apiMock, repository *client.Repository, commits int) { + runs := make([]client.WorkflowRun, 0, commits) + for i := range commits { + sha := fmt.Sprintf("%s-%d", repository.Name, i) runs = append(runs, client.WorkflowRun{ID: uint(i), WorkflowID: ci.ID, CheckSuiteID: uint(i), HeadSHA: sha}) - api.On("GetCheckRuns", *repo, sha).Return([]client.CheckRun{}, nil) + api.On("GetCheckRuns", *repository, sha).Return([]client.CheckRun{}, nil) } - expectListing(api, []client.Workflow{ci}, runs) + api.On("GetWorkflows", *repository).Return([]client.Workflow{ci}, nil) + api.On("GetWorkflowRuns", *repository, from, to).Return(runs, nil) +} + +// recorder captures what a collector reports through its hooks. +type recorder struct { + messages []string + listed []uint + done []uint + totals map[uint]bool +} + +func record(collector *Collector) *recorder { + r := &recorder{totals: make(map[uint]bool)} + collector.Notify = func(message string) { r.messages = append(r.messages, message) } + collector.ListingProgress = func(done, _ uint) { r.listed = append(r.listed, done) } + collector.FetchProgress = func(done, total uint) { + r.done = append(r.done, done) + r.totals[total] = true + } + return r +} + +// manyRepos expects the listing of enough idle repositories to pass the listing threshold. +func manyRepos(api *apiMock) []*client.Repository { + repos := make([]*client.Repository, 0, listingThreshold+1) + for i := range listingThreshold + 1 { + repository := &client.Repository{FullName: fmt.Sprintf("codiform/repo-%d", i), Name: fmt.Sprintf("repo-%d", i)} + busyRepo(api, repository, 0) + repos = append(repos, repository) + } + return repos +} + +func TestCollector_ForewarnsAndReportsProgressOfLargeCollections(t *testing.T) { + // Given + api := new(apiMock) + busyRepo(api, repo, progressThreshold+1) + plentyOfBudget(api) + collector := newCollector(api) + reported := record(collector) + + // When + _, err := collector.Collect(context.Background(), []*client.Repository{repo}) + + // Then + require.NoError(t, err) + assert.Equal(t, []string{"Fetching job durations for 21 commits in codiform/gh-actions-usage (about 28 API calls, 3 seconds)..."}, reported.messages) + assert.Empty(t, reported.listed) + assert.Len(t, reported.done, progressThreshold+1) + assert.IsIncreasing(t, reported.done) + assert.Equal(t, uint(progressThreshold+1), reported.done[len(reported.done)-1]) + assert.Equal(t, map[uint]bool{progressThreshold + 1: true}, reported.totals) +} + +func TestCollector_CountsProgressAcrossRepositories(t *testing.T) { + // Given: three repositories, one of them idle, whose commits together pass the threshold + api := new(apiMock) + other := &client.Repository{FullName: "codiform/other", Name: "other"} + idle := &client.Repository{FullName: "codiform/idle", Name: "idle"} + busyRepo(api, repo, 15) + busyRepo(api, other, 15) + busyRepo(api, idle, 0) + plentyOfBudget(api) + collector := newCollector(api) + reported := record(collector) + + // When + _, err := collector.Collect(context.Background(), []*client.Repository{repo, other, idle}) + + // Then: one forewarning for the busy repositories, and one running count that spans both + require.NoError(t, err) + assert.Equal(t, []string{"Fetching job durations for 30 commits in 2 repositories (about 39 API calls, 4 seconds)..."}, reported.messages) + assert.Len(t, reported.done, 30) + assert.IsIncreasing(t, reported.done) + assert.Equal(t, map[uint]bool{30: true}, reported.totals) +} + +func TestCollector_AnnouncesAndCountsListingOfManyRepositories(t *testing.T) { + // Given + api := new(apiMock) + repos := manyRepos(api) + plentyOfBudget(api) + collector := newCollector(api) + reported := record(collector) + + // When + _, err := collector.Collect(context.Background(), repos) + + // Then: the listing is preflighted and announced, and counted repository by repository + require.NoError(t, err) + assert.Equal(t, []string{"Listing workflows and runs for 11 repositories..."}, reported.messages) + assert.Equal(t, []uint{1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11}, reported.listed) + assert.Empty(t, reported.done) + api.AssertNumberOfCalls(t, "GetRateLimit", 1) +} + +func TestCollector_RefusesToListManyRepositoriesWhenBudgetIsShort(t *testing.T) { + // Given + api := new(apiMock) + repos := manyRepos(api) + api.On("GetRateLimit").Return(&client.RateLimit{Limit: 5000, Remaining: 21}, nil) + collector := newCollector(api) + reported := record(collector) + + // When + usage, err := collector.Collect(context.Background(), repos) + + // Then: two calls per repository were needed, and none were made + var limitErr RateLimitError + require.ErrorAs(t, err, &limitErr) + assert.Equal(t, uint(22), limitErr.Needed) + assert.Nil(t, usage) + assert.Empty(t, reported.messages) + api.AssertNotCalled(t, "GetWorkflows", mock.Anything) +} + +func TestCollector_IsQuietForSmallCollections(t *testing.T) { + // Given + api := new(apiMock) + busyRepo(api, repo, progressThreshold) plentyOfBudget(api) - var messages []string collector := newCollector(api) - collector.Notify = func(message string) { messages = append(messages, message) } + reported := record(collector) + + // When + _, err := collector.Collect(context.Background(), []*client.Repository{repo}) + + // Then + require.NoError(t, err) + assert.Empty(t, reported.messages) + assert.Empty(t, reported.done) +} + +func TestCollector_DoesNotForewarnWhenBudgetIsShort(t *testing.T) { + // Given + api := new(apiMock) + busyRepo(api, repo, progressThreshold+1) + api.On("GetRateLimit").Return(&client.RateLimit{Limit: 5000, Remaining: 1}, nil) + collector := newCollector(api) + reported := record(collector) // When _, err := collector.Collect(context.Background(), []*client.Repository{repo}) + // Then + require.ErrorAs(t, err, new(RateLimitError)) + assert.Empty(t, reported.messages) +} + +func TestCollector_WorksWithoutHooks(t *testing.T) { + // Given + api := new(apiMock) + busyRepo(api, repo, progressThreshold+1) + plentyOfBudget(api) + + // When + usage, err := newCollector(api).Collect(context.Background(), []*client.Repository{repo}) + // Then require.NoError(t, err) - assert.Equal(t, []string{"codiform/gh-actions-usage: fetching job durations for 21 commits..."}, messages) + assert.Equal(t, client.WorkflowUsage{ci: 0}, usage[repo]) +} + +func TestRoughly(t *testing.T) { + tests := []struct { + d time.Duration + want string + }{ + {d: 0, want: "0 seconds"}, + {d: time.Second, want: "1 second"}, + {d: 2800 * time.Millisecond, want: "3 seconds"}, + {d: 59 * time.Second, want: "59 seconds"}, + {d: 60 * time.Second, want: "1 minute"}, + {d: 89 * time.Second, want: "1 minute"}, + {d: 90 * time.Second, want: "2 minutes"}, + {d: 59*time.Minute + 29*time.Second, want: "59 minutes"}, + {d: time.Hour, want: "1 hour"}, + {d: time.Hour + 5*time.Minute, want: "1 hour 5 minutes"}, + {d: 2*time.Hour + 61*time.Minute, want: "3 hours 1 minute"}, + {d: 2*time.Hour + 59*time.Minute + 45*time.Second, want: "3 hours"}, + } + for _, tc := range tests { + t.Run(tc.want, func(t *testing.T) { + assert.Equal(t, tc.want, roughly(tc.d)) + }) + } } func TestCollector_StopsFetchingAfterAFailure(t *testing.T) { From 0830f89400fcbed67049dff23418b60e2148edea Mon Sep 17 00:00:00 2001 From: Geoffrey Wiseman Date: Mon, 14 Sep 2026 21:43:39 -0400 Subject: [PATCH 2/3] PR Feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addressing some of Copilot’s valid review points on the progress reporting. Signed-off-by: Geoffrey Wiseman --- client/client.go | 8 +++++--- client/transport.go | 12 +++++------- client/transport_test.go | 3 ++- main.go | 6 +++--- progress.go | 42 ++++++++++++++++++++++++++++------------ progress_test.go | 28 +++++++++++++++++++++++++++ usage/collector.go | 24 +++++++++++------------ usage/collector_test.go | 2 ++ 8 files changed, 87 insertions(+), 38 deletions(-) diff --git a/client/client.go b/client/client.go index 1de9125..a120513 100644 --- a/client/client.go +++ b/client/client.go @@ -4,6 +4,7 @@ package client import ( "errors" "fmt" + "io" "net/http" "github.com/cli/go-gh/v2/pkg/api" @@ -11,9 +12,10 @@ import ( ) // New creates a new Client instance, initialized with a GH RESTClient that paces requests to stay within -// GitHub's rate limits and retries requests that are rejected as rate limited. -func New() Client { - rest, err := api.NewRESTClient(api.ClientOptions{Transport: newThrottle(http.DefaultTransport)}) +// GitHub's rate limits and retries requests that are rejected as rate limited. A notice is written to notices +// before each wait for a rate limit, so that the user can see why the command has paused. +func New(notices io.Writer) Client { + rest, err := api.NewRESTClient(api.ClientOptions{Transport: newThrottle(http.DefaultTransport, notices)}) if err != nil { panic(err) } diff --git a/client/transport.go b/client/transport.go index 86b467d..6821018 100644 --- a/client/transport.go +++ b/client/transport.go @@ -6,7 +6,6 @@ import ( "fmt" "io" "net/http" - "os" "strconv" "time" @@ -45,20 +44,19 @@ type throttle struct { notify func(wait time.Duration) } -func newThrottle(next http.RoundTripper) *throttle { +// newThrottle paces requests through next, and writes a notice to notices before each backoff. +func newThrottle(next http.RoundTripper, notices io.Writer) *throttle { return &throttle{ next: next, limiter: rate.NewLimiter(RequestsPerSecond, requestBurst), now: time.Now, sleep: sleepContext, - notify: notifyStderr, + notify: func(wait time.Duration) { + _, _ = fmt.Fprintf(notices, "GitHub rate limit reached; waiting %s before retrying...\n", wait.Round(time.Second)) + }, } } -func notifyStderr(wait time.Duration) { - _, _ = fmt.Fprintf(os.Stderr, "GitHub rate limit reached; waiting %s before retrying...\n", wait.Round(time.Second)) -} - // RoundTrip implements http.RoundTripper func (t *throttle) RoundTrip(req *http.Request) (*http.Response, error) { ctx := req.Context() diff --git a/client/transport_test.go b/client/transport_test.go index 8bc36be..422ee50 100644 --- a/client/transport_test.go +++ b/client/transport_test.go @@ -3,6 +3,7 @@ package client import ( "context" "errors" + "io" "net/http" "net/http/httptest" "strconv" @@ -24,7 +25,7 @@ type throttleLog struct { // testThrottle returns a throttle that records requested sleeps and notifications instead of waiting or printing. func testThrottle(now time.Time) (*throttle, *throttleLog) { log := &throttleLog{} - t := newThrottle(http.DefaultTransport) + t := newThrottle(http.DefaultTransport, io.Discard) t.now = func() time.Time { return now } t.sleep = func(_ context.Context, d time.Duration) error { log.sleeps = append(log.sleeps, d) diff --git a/main.go b/main.go index f91bf7c..6e0d7a6 100644 --- a/main.go +++ b/main.go @@ -68,7 +68,7 @@ func (e InvalidMonthError) Error() string { func main() { fmt.Printf("GitHub Actions Usage (%s)\n\n", getVersion()) - gh = client.New() + gh = client.New(status) now := time.Now() cfg := &config{w: os.Stdout} @@ -131,11 +131,11 @@ func tryDisplayCurrentRepo(cfg config) { return } repoFlowUsage, err := collectUsage(cfg, []*client.Repository{repo}) + status.separate() if err != nil { printError(cfg, "Error collecting usage", err) return } - status.separate() if cfg.skip { skipUnused(repoFlowUsage) } @@ -154,11 +154,11 @@ func tryDisplayAllSpecified(cfg config, targets []string) { targetsList = append(targetsList, list...) } repoFlowUsage, err := collectUsage(cfg, targetsList) + status.separate() if err != nil { printError(cfg, "Error collecting usage", err) return } - status.separate() if cfg.skip { skipUnused(repoFlowUsage) } diff --git a/progress.go b/progress.go index 2188bf5..3480b88 100644 --- a/progress.go +++ b/progress.go @@ -3,6 +3,7 @@ package main import ( "fmt" "io" + "sync" "time" ) @@ -15,29 +16,45 @@ const ( percent = 100 ) -// statusWriter is the writer for everything said about a collection's progress on stderr. It remembers whether -// anything was said, so that the report can be set apart from it by a blank line only when there is something -// to set it apart from. +// statusWriter is the writer for everything said about a collection's progress on stderr: the collector's +// messages and counters, and the transport's rate-limit notices, which can arrive from any goroutine. It keeps +// a counter being redrawn in place from being overwritten by a message, by finishing the line first, and it +// remembers whether anything was said, so that what follows can be set apart by a blank line only when there +// is something to set it apart from. type statusWriter struct { io.Writer - written bool + mu sync.Mutex + written bool // anything at all since the last separation + open bool // the last write left its line unfinished, as an in-place counter does } func (s *statusWriter) Write(p []byte) (int, error) { - if len(p) > 0 { - s.written = true + if len(p) == 0 { + return 0, nil } + s.mu.Lock() + defer s.mu.Unlock() + if s.open && p[0] != '\r' { + _, _ = s.Writer.Write([]byte{'\n'}) + } + s.written = true + s.open = p[len(p)-1] != '\n' return s.Writer.Write(p) //nolint:wrapcheck // a plain pass-through } -// separate ends the progress output with a blank line if there was any, and forgets it, so that a later -// collection starts afresh. +// separate finishes any unfinished line and ends the status output with a blank line if there was any, then +// forgets it all, so that a later collection starts afresh. func (s *statusWriter) separate() { + s.mu.Lock() + defer s.mu.Unlock() + if s.open { + _, _ = s.Writer.Write([]byte{'\n'}) + } if s.written { - _, _ = fmt.Fprintln(s) - s.written = false + _, _ = s.Writer.Write([]byte{'\n'}) } + s.open, s.written = false, false } // progressReporter renders the collector's commit counter on stderr. On a terminal the line is rewritten in @@ -74,10 +91,11 @@ func (p *progressReporter) rewrite(done, total uint) { return } p.last = now - _, _ = fmt.Fprintf(p.w, "\r%s", p.line(done, total)) + end := "" if done == total { - _, _ = fmt.Fprintln(p.w) + end = "\n" } + _, _ = fmt.Fprintf(p.w, "\r%s%s", p.line(done, total), end) } func (p *progressReporter) step(done, total uint) { diff --git a/progress_test.go b/progress_test.go index f39a8b7..b4fceb8 100644 --- a/progress_test.go +++ b/progress_test.go @@ -100,6 +100,34 @@ func TestProgressReporter_IgnoresAnEmptyTotal(t *testing.T) { assert.Empty(t, out.String()) } +func TestStatusWriter_FinishesAnOpenCounterBeforeAMessage(t *testing.T) { + // Given: a counter drawn in place, twice + var out bytes.Buffer + status := &statusWriter{Writer: &out} + _, _ = status.Write([]byte("\r1 of 3")) + _, _ = status.Write([]byte("\r2 of 3")) + + // When: a rate-limit notice arrives, and the counter goes on + _, _ = status.Write([]byte("waiting 5s...\n")) + _, _ = status.Write([]byte("\r3 of 3\n")) + + // Then: redraws stay on their line, the notice gets a line of its own, and the counter resumes below it + assert.Equal(t, "\r1 of 3\r2 of 3\nwaiting 5s...\n\r3 of 3\n", out.String()) +} + +func TestStatusWriter_SeparatesAfterAnOpenCounter(t *testing.T) { + // Given: a collection that failed mid-counter + var out bytes.Buffer + status := &statusWriter{Writer: &out} + _, _ = status.Write([]byte("\r1 of 3")) + + // When + status.separate() + + // Then: the line is finished, then the blank line + assert.Equal(t, "\r1 of 3\n\n", out.String()) +} + func TestStatusWriter_SeparatesOnlyAfterOutput(t *testing.T) { // Given var out bytes.Buffer diff --git a/usage/collector.go b/usage/collector.go index 2ed5dff..1d756dd 100644 --- a/usage/collector.go +++ b/usage/collector.go @@ -235,19 +235,19 @@ func forewarning(plans []*repoPlan, commits, needed uint) string { // roughly renders a duration estimate in the coarsest unit that still says something: seconds under a minute, // minutes under an hour, and hours and minutes beyond that. func roughly(d time.Duration) string { - switch { - case d < time.Minute: - return plural(int(d.Round(time.Second)/time.Second), "second") - case d < time.Hour: - return plural(int(d.Round(time.Minute)/time.Minute), "minute") - default: - d = d.Round(time.Minute) - hours := plural(int(d/time.Hour), "hour") - if minutes := int(d % time.Hour / time.Minute); minutes > 0 { - return hours + " " + plural(minutes, "minute") - } - return hours + d = d.Round(time.Second) + if d < time.Minute { + return plural(int(d/time.Second), "second") + } + d = d.Round(time.Minute) + if d < time.Hour { + return plural(int(d/time.Minute), "minute") + } + hours := plural(int(d/time.Hour), "hour") + if minutes := int(d % time.Hour / time.Minute); minutes > 0 { + return hours + " " + plural(minutes, "minute") } + return hours } func plural(n int, unit string) string { diff --git a/usage/collector_test.go b/usage/collector_test.go index ebde57a..75d153f 100644 --- a/usage/collector_test.go +++ b/usage/collector_test.go @@ -433,10 +433,12 @@ func TestRoughly(t *testing.T) { {d: time.Second, want: "1 second"}, {d: 2800 * time.Millisecond, want: "3 seconds"}, {d: 59 * time.Second, want: "59 seconds"}, + {d: 59500 * time.Millisecond, want: "1 minute"}, {d: 60 * time.Second, want: "1 minute"}, {d: 89 * time.Second, want: "1 minute"}, {d: 90 * time.Second, want: "2 minutes"}, {d: 59*time.Minute + 29*time.Second, want: "59 minutes"}, + {d: 59*time.Minute + 30*time.Second, want: "1 hour"}, {d: time.Hour, want: "1 hour"}, {d: time.Hour + 5*time.Minute, want: "1 hour 5 minutes"}, {d: 2*time.Hour + 61*time.Minute, want: "3 hours 1 minute"}, From 71c9531bd4abf7f91329f8541c67ef0f7f303148 Mon Sep 17 00:00:00 2001 From: Geoffrey Wiseman Date: Mon, 14 Sep 2026 21:53:55 -0400 Subject: [PATCH 3/3] Additional Test Added a missing pagination test. Signed-off-by: Geoffrey Wiseman --- client/client_test.go | 57 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/client/client_test.go b/client/client_test.go index a3299cc..07e5a4f 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -1,6 +1,7 @@ package client import ( + "fmt" "net/url" "testing" @@ -143,6 +144,62 @@ func TestClient_GetUser_NotFound(t *testing.T) { assert.Nil(t, repo) } +func TestClient_GetWorkflows_FetchesTheNextPageAfterAFullOne(t *testing.T) { + // Given: a full first page and a short second one + rest, client := getTestClient() + repo := Repository{ID: 1, Name: "gh-actions-usage", FullName: testRepoFullName} + rest.On("Get", "repos/"+testRepoFullName+"/actions/workflows?per_page=100&page=1", mock.Anything). + Return(nil). + Run(func(args mock.Arguments) { + wp := args.Get(1).(*workflowPage) + for i := range perPage { + wp.Workflows = append(wp.Workflows, Workflow{ID: uint(i), Name: fmt.Sprintf("Workflow %d", i)}) + } + }) + rest.On("Get", "repos/"+testRepoFullName+"/actions/workflows?per_page=100&page=2", mock.Anything). + Return(nil). + Run(func(args mock.Arguments) { + wp := args.Get(1).(*workflowPage) + wp.Workflows = append(wp.Workflows, Workflow{ID: perPage, Name: "Workflow 100"}) + }) + + // When + workflows, err := client.GetWorkflows(repo) + + // Then: both pages are returned, and no third page is requested + require.NoError(t, err) + assert.Len(t, workflows, perPage+1) + rest.AssertNumberOfCalls(t, "Get", 2) +} + +func TestClient_GetAllRepositories_FetchesTheNextPageAfterAFullOne(t *testing.T) { + // Given: a full first page and a short second one + rest, client := getTestClient() + rest.On("Get", "orgs/codiform/repos?per_page=100&page=1", mock.Anything). + Return(nil). + Run(func(args mock.Arguments) { + ars := args.Get(1).(*[]*Repository) + for i := range perPage { + *ars = append(*ars, &Repository{ID: uint(i), Name: fmt.Sprintf("repo-%d", i)}) + } + }) + rest.On("Get", "orgs/codiform/repos?per_page=100&page=2", mock.Anything). + Return(nil). + Run(func(args mock.Arguments) { + ars := args.Get(1).(*[]*Repository) + *ars = append(*ars, &Repository{ID: perPage, Name: "repo-100"}) + }) + owner := &User{ID: 1, Login: "codiform", Type: "Organization"} + + // When + repos, err := client.GetAllRepositories(owner) + + // Then: both pages are returned, and no third page is requested + require.NoError(t, err) + assert.Len(t, repos, perPage+1) + rest.AssertNumberOfCalls(t, "Get", 2) +} + // Success Case func TestClient_GetAllRepositories(t *testing.T) { // Given