Skip to content

Name the regressed benchmark+measure pairs in the issue body #3830

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
justin808 merged 1 commit into from
Jun 9, 2026
Merged

Name the regressed benchmark+measure pairs in the issue body #3830

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 48 additions & 4 deletions benchmarks/report_regressions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,12 @@ def self.report(summary:, **attributes)
new(**attributes).report(summary)
end

def initialize(suite_name:, github_run_url:, bencher_url:, issue_number_cache: nil, report_comment_id_cache: nil)
def initialize(suite_name:, github_run_url:, bencher_url:, regressed_overview: "",
issue_number_cache: nil, report_comment_id_cache: nil)
@suite_name = suite_name
@github_run_url = github_run_url
@bencher_url = bencher_url
@regressed_overview = regressed_overview
@issue_number_cache = issue_number_cache
@report_comment_id_cache = report_comment_id_cache
@commit_short = ENV.fetch("GITHUB_SHA")[0, 7]
Expand All @@ -63,7 +65,8 @@ def report(summary)

private

attr_reader :suite_name, :github_run_url, :bencher_url, :issue_number_cache, :report_comment_id_cache, :commit_short
attr_reader :suite_name, :github_run_url, :bencher_url, :regressed_overview, :issue_number_cache,
:report_comment_id_cache, :commit_short

def ensure_regression_label
GithubCli.run(
Expand Down Expand Up @@ -321,7 +324,7 @@ def issue_body
| **Pushed by** | @#{github_actor} |
| **Workflow run** | [Run ##{github_run_number}](#{github_run_url}) |
| **Bencher dashboard** | [View history](#{bencher_url}) |

#{regressed_section}

@claude claude Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When regressed_section is non-empty it ends with \n\n (from the <<~MARKDOWN trailing blank line), and this heredoc line contributes another \n, producing two blank lines before ### What to do instead of one. GitHub Markdown collapses them on render so there's no visual difference, but it's worth noting if the raw markdown ever needs to be exact.

If you want a single blank line consistently, you can strip the trailing newline from regressed_section:

      #{regressed_section.chomp}

or trim the trailing blank line inside the regressed_section heredoc. Not a blocker.

### What to do

1. Check the workflow run for the full Bencher HTML report
Expand All @@ -335,6 +338,24 @@ def issue_body
MARKDOWN
end

# A "What regressed" section naming the confirmed benchmark+measure pairs, so the issue
# body itself says what regressed (the per-suite tables flag only rps/p50, never the
# column-less failed_pct). Empty when no structured pairs were handed off, leaving the
# body unchanged.
def regressed_section
return "" if regressed_overview.to_s.strip.empty?

@claude claude Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regressed_overview is always a String — initialized to "" and only accepted as a keyword arg with that default — so .to_s is a no-op. Minor nit:

Suggested change
return "" if regressed_overview.to_s.strip.empty?
return "" if regressed_overview.strip.empty?


<<~MARKDOWN

### What regressed

The fresh-runner confirmation re-alerted on these benchmark + measure pairs:

#{regressed_overview}

MARKDOWN
Comment on lines +354 to +356

@greptile-apps greptile-apps Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 The trailing blank line inside regressed_section's heredoc means the method returns a string ending with `

. The outer issue_bodyheredoc's own line-ending
then appends a third newline, producing two blank lines between the last bullet and### What to do`. Dropping the trailing blank line inside the inner heredoc leaves exactly one blank line, matching standard Markdown section spacing.

Suggested change
#{regressed_overview}
MARKDOWN
#{regressed_overview}
MARKDOWN

end

def github_actor
@github_actor ||= ENV.fetch("GITHUB_ACTOR")
end
Expand Down Expand Up @@ -400,6 +421,23 @@ def shard_summary(payload)
MARKDOWN
end

# A flat, deduped list of the confirmed benchmark+measure pairs across all suites, for the
# issue body — so a reader sees what regressed without opening the per-suite tables. Built
# from each payload's structured ALERTS ({benchmark, measure}); "" when no payload carried
# them (older writer, or alerts with no benchmark name), which leaves the body unchanged.
def regressed_overview_markdown(payloads)
payloads
.flat_map { |payload| payload[RegressionReport::ALERTS] || [] }
.select { |pair| pair.is_a?(Hash) && pair[RegressionReport::ALERT_BENCHMARK] }
.uniq

@claude claude Bot Jun 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.uniq deduplicates by full Hash equality, which works today because RegressionReport.alert_hash always creates exactly {benchmark, measure} — no extra keys. If the ALERTS format ever gains a third key (e.g. a timestamp or suite tag), identical benchmark+measure pairs from different shards would no longer be deduped.

Suggested change
.uniq
.uniq { |pair| [pair[RegressionReport::ALERT_BENCHMARK], pair[RegressionReport::ALERT_MEASURE]] }

This locks the dedup key to the two semantically meaningful fields regardless of future payload growth.

.map do |pair|
benchmark = pair[RegressionReport::ALERT_BENCHMARK]
measure = pair[RegressionReport::ALERT_MEASURE]
measure ? "- `#{benchmark}` — **#{measure}**" : "- `#{benchmark}`"
end
.join("\n")
end

# Reports the confirmed regressions. Returns:
# :clean — no confirmed payloads (every first-run alert cleared as noise / ignored)
# :filed — at least one confirmed regression filed/updated successfully
Expand All @@ -418,12 +456,17 @@ def report_regressions(artifacts_dir)
# One section per suite: a sharded suite emits one payload per shard, so combine
# them rather than filing a section per shard. Suites sorted for stable output.
by_suite = readable.group_by { |payload| payload.fetch(RegressionReport::SUITE_NAME) }
# Name every confirmed benchmark+measure pair across all suites in the issue body so it is
# actionable on its own. Computed once and passed to each suite's reporter; only the
# reporter that creates the issue renders the body.
regressed_overview = regressed_overview_markdown(readable)
issue_number_cache = {}
report_comment_id_cache = {}
reported_ok = by_suite.keys.sort.map do |suite_name|
report_suite(
suite_name,
by_suite.fetch(suite_name),
regressed_overview:,
issue_number_cache:,
report_comment_id_cache:
)
Expand All @@ -435,7 +478,7 @@ def report_regressions(artifacts_dir)
:filed
end

def report_suite(suite_name, payloads, issue_number_cache: nil, report_comment_id_cache: nil)
def report_suite(suite_name, payloads, regressed_overview: "", issue_number_cache: nil, report_comment_id_cache: nil)
# Order shard summaries by shard number ("2/5" before "10/5"); each already
# self-labels with its shard in its headers, so concatenation reads cleanly.
summary = payloads
Expand All @@ -449,6 +492,7 @@ def report_suite(suite_name, payloads, issue_number_cache: nil, report_comment_i
github_run_url: Github.run_url,
bencher_url: BENCHER_URL,
summary:,
regressed_overview:,
issue_number_cache:,
report_comment_id_cache:
)
Expand Down
37 changes: 37 additions & 0 deletions benchmarks/spec/report_regressions_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,24 @@ def report_with_cache(suite_name, issue_number_cache, report_comment_id_cache =
expect(calls).to eq(["label create", "issue list", "issue create", "comment list", "comment create"])
end

it "names the confirmed benchmark+measure pairs in the created issue body" do
described_class.report(
summary: "core regressed",
suite_name: "core",
github_run_url: "https://github.com/run/1",
bencher_url: "https://bencher.dev/dash",
regressed_overview: "- `/x: Core` — **rps**"
)
body = posted_bodies.fetch("issue create")
expect(body).to include("### What regressed")
expect(body).to include("- `/x: Core` — **rps**")
end
Comment on lines +120 to +131

@greptile-apps greptile-apps Bot Jun 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing multi-line overview coverage in issue-body test

The regressed_overview_markdown unit test confirms that the helper produces a multi-line string for multiple pairs (e.g. "- \/a: Core` — rps\n- `/b: Pro`"), but the RegressionIssueReporterbody test only exercises a single-lineregressed_overview. Because the overview is interpolated inside a <<~MARKDOWNheredoc inside another<<~MARKDOWN` heredoc, the indentation-stripping of the outer heredoc applies to each logical line of the interpolated value; lines beyond the first have zero leading spaces, so the strip is effectively a no-op on them, but this behaviour is untested end-to-end. A test passing a two-pair overview and asserting both bullets appear in the created issue body would close that gap.


it "omits the what-regressed section when no pairs were handed off" do
report
expect(posted_bodies.fetch("issue create")).not_to include("What regressed")
end

it "reuses a just-created issue number for later suites in the same reporter run" do
issue_number_cache = {}

Expand Down Expand Up @@ -274,6 +292,25 @@ def section_for(suite, content = "")
# first-run/confirmation evidence rendering, and the exit status. A confirmed
# regression fails the workflow (exit 1) even though the issue was filed successfully —
# this job owns the final pass/fail.
describe "regressed_overview_markdown" do
it "renders deduped benchmark+measure bullets from each payload's ALERTS" do
payloads = [
{ RegressionReport::ALERTS => [
{ RegressionReport::ALERT_BENCHMARK => "/a: Core", RegressionReport::ALERT_MEASURE => "rps" },
{ RegressionReport::ALERT_BENCHMARK => "/a: Core", RegressionReport::ALERT_MEASURE => "rps" }
] },
{ RegressionReport::ALERTS => [
{ RegressionReport::ALERT_BENCHMARK => "/b: Pro", RegressionReport::ALERT_MEASURE => nil }
] }
]
expect(regressed_overview_markdown(payloads)).to eq("- `/a: Core` — **rps**\n- `/b: Pro`")
end

it "is empty when no payload carries structured alert pairs" do
expect(regressed_overview_markdown([{ RegressionReport::SUITE_NAME => "Core" }])).to eq("")
end
end

describe "report_regressions.rb (script)" do
script = File.expand_path("../report_regressions.rb", __dir__)

Expand Down
Loading