Deduplicate regression issue reporting (#3767)

Author: justin808

benchmarks/lib/github_cli.rb

@@ -12,8 +12,8 @@ def capture(*args, env: {}, error_message: nil, stdin_data: nil)
     [stdout, status]
   end
 
-  def capture_success(*args, error_message:, env: {})
-    stdout, status = capture(*args, env: env, error_message: error_message)
+  def capture_success(*args, error_message:, env: {}, stdin_data: nil)
+    stdout, status = capture(*args, env: env, error_message: error_message, stdin_data: stdin_data)
     return stdout if status.success?
 
     nil

benchmarks/report_regressions.rb

@@ -45,15 +45,20 @@
 # rubocop:disable Metrics/ClassLength
 class RegressionIssueReporter
   LABEL = "performance-regression"
+  CACHE_MISS = :cache_miss
+  ISSUE_NUMBER_UNKNOWN_AFTER_CREATE = :issue_number_unknown_after_create
+  COMMENT_ID_UNKNOWN_AFTER_CREATE = :comment_id_unknown_after_create
 
   def self.report(summary:, **attributes)
     new(**attributes).report(summary)
   end
 
-  def initialize(suite_name:, github_run_url:, bencher_url:)
+  def initialize(suite_name:, github_run_url:, bencher_url:, issue_number_cache: nil, report_comment_id_cache: nil)
     @suite_name = suite_name
     @github_run_url = github_run_url
     @bencher_url = bencher_url
+    @issue_number_cache = issue_number_cache
+    @report_comment_id_cache = report_comment_id_cache
     @commit_short = ENV.fetch("GITHUB_SHA")[0, 7]
   end
 
@@ -71,7 +76,7 @@ def report(summary)
 
   private
 
-  attr_reader :suite_name, :github_run_url, :bencher_url, :commit_short
+  attr_reader :suite_name, :github_run_url, :bencher_url, :issue_number_cache, :report_comment_id_cache, :commit_short
 
   def ensure_regression_label
     GithubCli.run(
@@ -127,6 +132,9 @@ def section_end
   end
 
   def regression_comment_id(issue_number)
+    cached_comment_id = cached_regression_comment_id
+    return cached_comment_id unless cached_comment_id == CACHE_MISS
+
     stdout = GithubCli.capture_success(
       "gh", "api", "repos/#{github_repository}/issues/#{issue_number}/comments",
       "--paginate",
@@ -138,7 +146,9 @@ def regression_comment_id(issue_number)
     # existing report comment yet (caller should create one).
     return nil unless stdout
 
-    stdout.lines.first.to_s.strip
+    stdout.lines.first.to_s.strip.tap do |comment_id|
+      cache_regression_comment_id(comment_id) unless comment_id.empty?
+    end
   end
 
   def regression_comment_body(comment_id)
@@ -197,10 +207,7 @@ def create_or_update_regression_comment(issue_number, summary)
 
     if comment_id.empty?
       body = "#{comment_header}#{section}"
-      return GithubCli.run(
-        "gh", "issue", "comment", issue_number, "--body", body,
-        error_message: "Failed to create regression report comment on issue ##{issue_number}"
-      )
+      return create_regression_comment(issue_number, body)
     end
 
     existing_body = regression_comment_body(comment_id)
@@ -219,11 +226,70 @@ def create_or_update_regression_comment(issue_number, summary)
   end
 
   def find_or_create_regression_issue
+    cached_issue_number = cached_regression_issue_number
+    return cached_issue_number unless cached_issue_number == CACHE_MISS
+
     issue_number = existing_regression_issue
     return nil if issue_number.nil?
-    return issue_number unless issue_number.empty?
 
-    create_regression_issue
+    return cache_regression_issue_number(issue_number) unless issue_number.empty?
+
+    create_regression_issue.tap do |created_issue_number|
+      # nil means create_regression_issue already stored ISSUE_NUMBER_UNKNOWN_AFTER_CREATE;
+      # this no-ops for nil so later suites still avoid duplicate issue creation.
+      cache_regression_issue_number(created_issue_number)
+    end
+  end
+
+  def cached_regression_issue_number
+    return CACHE_MISS unless issue_number_cache
+
+    issue_number_cache.fetch(issue_title, CACHE_MISS).then do |cached_issue_number|
+      cached_issue_number == ISSUE_NUMBER_UNKNOWN_AFTER_CREATE ? nil : cached_issue_number
+    end
+  end
+
+  def cache_regression_issue_number(issue_number)
+    issue_number_cache&.store(issue_title, issue_number) unless issue_number.to_s.empty?
+    issue_number
+  end
+
+  def cached_regression_comment_id
+    return CACHE_MISS unless report_comment_id_cache
+
+    report_comment_id_cache.fetch(comment_marker, CACHE_MISS).then do |cached_comment_id|
+      cached_comment_id == COMMENT_ID_UNKNOWN_AFTER_CREATE ? nil : cached_comment_id
+    end
+  end
+
+  def cache_regression_comment_id(comment_id)
+    report_comment_id_cache&.store(comment_marker, comment_id) unless comment_id.to_s.empty?
+    comment_id
+  end
+
+  def create_regression_comment(issue_number, body)
+    stdout = GithubCli.capture_success(
+      "gh", "api", "-X", "POST",
+      "repos/#{github_repository}/issues/#{issue_number}/comments",
+      "--input", "-",
+      "--jq", ".id",
+      error_message: "Failed to create regression report comment on issue ##{issue_number}",
+      stdin_data: JSON.generate(body: body)
+    )
+    return false unless stdout
+
+    comment_id = stdout.lines.first.to_s.strip
+    if comment_id.empty?
+      report_comment_id_cache&.store(comment_marker, COMMENT_ID_UNKNOWN_AFTER_CREATE)
+      Github.warning(
+        "Created regression report comment on issue ##{issue_number} but could not parse its id; " \
+        "this suite and later suites in this run will be marked as failed to avoid duplicate comments."
+      )
+      return false
+    end
+
+    cache_regression_comment_id(comment_id)
+    true
   end
 
   def create_regression_issue
@@ -244,6 +310,7 @@ def create_regression_issue
     # so a parse miss is a warning, not an error.
     number = stdout[%r{/issues/(\d+)}, 1]
     unless number
+      issue_number_cache&.store(issue_title, ISSUE_NUMBER_UNKNOWN_AFTER_CREATE)
       Github.warning(
         "Created the issue but could not parse its number from gh output " \
         "(#{stdout.strip.inspect}); its comment section may be missing"
@@ -362,13 +429,22 @@ 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) }
-  reported_ok = by_suite.keys.sort.map { |suite_name| report_suite(suite_name, by_suite.fetch(suite_name)) }.all?
+  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),
+      issue_number_cache: issue_number_cache,
+      report_comment_id_cache: report_comment_id_cache
+    )
+  end.all?
 
   # Fail if any payload was unreadable: a lost report must not pass as success.
   reported_ok && readable.size == payloads.size
 end
 
-def report_suite(suite_name, payloads)
+def report_suite(suite_name, payloads, 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
@@ -381,7 +457,9 @@ def report_suite(suite_name, payloads)
     suite_name: suite_name,
     github_run_url: Github.run_url,
     bencher_url: BENCHER_URL,
-    summary: summary
+    summary: summary,
+    issue_number_cache: issue_number_cache,
+    report_comment_id_cache: report_comment_id_cache
   )
 
   if issue_number.empty?

benchmarks/spec/report_regressions_spec.rb

@@ -42,9 +42,10 @@ def stub_env
     let(:posted_bodies) { {} }
 
     before do
-      allow(GithubCli).to receive(:capture_success) do |*args, **_kwargs|
+      allow(GithubCli).to receive(:capture_success) do |*args, **kwargs|
         key = capture_key(args)
         calls << key
+        posted_bodies[key] = extract_body(args, kwargs)
         capture_responses.fetch(key) { raise "unexpected capture_success: #{key}" }
       end
 
@@ -69,6 +70,7 @@ def extract_body(args, kwargs)
     def capture_key(args)
       return "issue list" if args[1..2] == %w[issue list]
       return "issue create" if args[1..2] == %w[issue create]
+      return "comment create" if args[1..3] == %w[api -X POST]
       return "comment list" if args[1] == "api" && args[2].match?(%r{/issues/\d+/comments\z})
       return "comment body" if args[1] == "api" && args[2].match?(%r{/issues/comments/\d+\z})
 
@@ -77,7 +79,6 @@ def capture_key(args)
 
     def run_key(args)
       return "label create" if args[1..2] == %w[label create]
-      return "issue comment" if args[1..2] == %w[issue comment]
       return "comment patch" if args[1..3] == %w[api -X PATCH]
 
       raise "unrecognized run args: #{args.inspect}"
@@ -92,16 +93,66 @@ def report
       )
     end
 
+    def report_with_cache(suite_name, issue_number_cache, report_comment_id_cache = nil)
+      described_class.report(
+        summary: "#{suite_name} regressed",
+        suite_name: suite_name,
+        github_run_url: "https://github.com/run/1",
+        bencher_url: "https://bencher.dev/dash",
+        issue_number_cache: issue_number_cache,
+        report_comment_id_cache: report_comment_id_cache
+      )
+    end
+
     context "when no regression issue exists yet" do
       before do
         capture_responses["issue list"] = ""
         capture_responses["issue create"] = "https://github.com/shakacode/react_on_rails/issues/123\n"
         capture_responses["comment list"] = ""
+        capture_responses["comment create"] = "999\n"
       end
 
       it "creates the issue, parses the number from the URL, and posts the first comment" do
         expect(report).to eq("123")
-        expect(calls).to eq(["label create", "issue list", "issue create", "comment list", "issue comment"])
+        expect(calls).to eq(["label create", "issue list", "issue create", "comment list", "comment create"])
+      end
+
+      it "reuses a just-created issue number for later suites in the same reporter run" do
+        issue_number_cache = {}
+
+        expect(report_with_cache("Core", issue_number_cache)).to eq("123")
+        expect(report_with_cache("Pro", issue_number_cache)).to eq("123")
+
+        expect(calls.count("issue list")).to eq(1)
+        expect(calls.count("issue create")).to eq(1)
+      end
+
+      it "reuses a just-created report comment for later suites in the same reporter run" do
+        issue_number_cache = {}
+        report_comment_id_cache = {}
+        capture_responses["comment body"] = "Core regressed"
+
+        expect(report_with_cache("Core", issue_number_cache, report_comment_id_cache)).to eq("123")
+        expect(report_with_cache("Pro", issue_number_cache, report_comment_id_cache)).to eq("123")
+
+        expect(calls.count("comment list")).to eq(1)
+        expect(calls.count("comment create")).to eq(1)
+        expect(calls).to include("comment patch")
+        expect(posted_bodies.fetch("comment patch")).to include("Core regressed")
+        expect(posted_bodies.fetch("comment patch")).to include("Pro regressed")
+      end
+
+      it "does not create duplicate same-run comments after a comment id parse miss" do
+        issue_number_cache = {}
+        report_comment_id_cache = {}
+        capture_responses["comment create"] = "\n"
+
+        expect(report_with_cache("Core", issue_number_cache, report_comment_id_cache)).to eq("")
+        expect(report_with_cache("Pro", issue_number_cache, report_comment_id_cache)).to eq("")
+
+        expect(calls.count("comment list")).to eq(1)
+        expect(calls.count("comment create")).to eq(1)
+        expect(calls).not_to include("comment patch")
       end
     end
 
@@ -163,7 +214,7 @@ def section_for(suite, content = "")
 
       it "aborts without posting a duplicate comment" do
         expect(report).to eq("")
-        expect(calls).not_to include("issue comment")
+        expect(calls).not_to include("comment create")
         expect(calls).not_to include("comment patch")
       end
     end
@@ -205,6 +256,16 @@ def section_for(suite, content = "")
           .and output("").to_stderr
         expect(calls).not_to include("comment list")
       end
+
+      it "does not create duplicate same-run issues after the parse miss" do
+        issue_number_cache = {}
+
+        expect(report_with_cache("Core", issue_number_cache)).to eq("")
+        expect(report_with_cache("Pro", issue_number_cache)).to eq("")
+
+        expect(calls.count("issue list")).to eq(1)
+        expect(calls.count("issue create")).to eq(1)
+      end
     end
   end
 
@@ -221,11 +282,13 @@ def section_for(suite, content = "")
       #!/usr/bin/env bash
       if [ "$1" = "issue" ] && [ "$2" = "create" ]; then
         echo "https://github.com/shakacode/react_on_rails/issues/7"
+      elif [ "$1" = "api" ] && [ "$2" = "-X" ] && [ "$3" = "POST" ]; then
+        echo "777"
       fi
       exit 0
     BASH
 
-    def run_script(script, artifacts_dir, gh_stub:)
+    def run_script(script, artifacts_dir, gh_stub:, extra_env: {})
       Dir.mktmpdir do |bin_dir|
         File.write(File.join(bin_dir, "gh"), gh_stub)
         File.chmod(0o755, File.join(bin_dir, "gh"))
@@ -238,7 +301,7 @@ def run_script(script, artifacts_dir, gh_stub:)
           "GITHUB_RUN_ID" => "999",
           "GITHUB_RUN_NUMBER" => "42",
           "GITHUB_ACTOR" => "octocat"
-        }
+        }.merge(extra_env)
         Open3.capture2e(env, "ruby", script, artifacts_dir)
       end
     end
@@ -275,6 +338,51 @@ def write_payload(dir, artifact:, suite:, shard_label: "1/1", summary: nil, regr
       end
     end
 
+    it "creates only one issue for multiple suites even when live issue lookup stays empty" do
+      Dir.mktmpdir do |dir|
+        write_payload(dir, artifact: "regression-core", suite: "Core")
+        write_payload(dir, artifact: "regression-pro", suite: "Pro")
+
+        call_log = File.join(dir, "gh-calls.log")
+        counting_gh = <<~BASH
+          #!/usr/bin/env bash
+          printf '%s\\n' "$*" >> "$GH_CALL_LOG"
+          if [ "$1" = "issue" ] && [ "$2" = "create" ]; then
+            echo "https://github.com/shakacode/react_on_rails/issues/7"
+          elif [ "$1" = "api" ] && [ "$2" = "-X" ] && [ "$3" = "POST" ]; then
+            echo "777"
+          fi
+          exit 0
+        BASH
+
+        output, status = run_script(script, dir, gh_stub: counting_gh, extra_env: { "GH_CALL_LOG" => call_log })
+
+        expect(status).to be_success
+        expect(output).to match(/issue #7/)
+        expect(File.readlines(call_log).count { |line| line.start_with?("issue create") }).to eq(1)
+        expect(File.readlines(call_log).count { |line| line.start_with?("api -X POST") }).to eq(1)
+      end
+    end
+
+    it "shares one issue-number cache across suite reports in the same run" do
+      Dir.mktmpdir do |dir|
+        write_payload(dir, artifact: "regression-core", suite: "Core")
+        write_payload(dir, artifact: "regression-pro", suite: "Pro")
+
+        caches = []
+        allow(Github).to receive(:run_url).and_return("https://github.com/run/1")
+        allow(RegressionIssueReporter).to receive(:report) do |issue_number_cache:, **_attributes|
+          caches << issue_number_cache
+          issue_number_cache["created"] = "7"
+          "7"
+        end
+
+        expect(report_regressions(dir)).to be(true)
+        expect(caches.size).to eq(2)
+        expect(caches[0]).to equal(caches[1])
+      end
+    end
+
     it "combines a sharded suite's payloads into a single report" do
       Dir.mktmpdir do |dir|
         write_payload(dir, artifact: "regression-pro-shard-1", suite: "Pro", shard_label: "1/2")