Tweak BuildPulse/rspec-retry logic

BuildPulse is trying to find flaky tests for us but, given the previous
model of using `rspec-retry`, it would rarely find them. Instead, let's
try to always rerun `brew tests` multiple times, report to BuildPulse
each time (by moving the reporting logic into `brew tests`) and disable
`rspec-retry` when using BuildPulse.

While we're here, let's enable `rspec-retry` locally so we don't have
flaky tests biting maintainers/contributors there.

.github/workflows/tests.yml

@@ -311,11 +311,24 @@ jobs:
           restore-keys: ${{ runner.os }}-parallel_runtime_rspec-
 
       - name: Run brew tests
-        run: brew tests --online --coverage
+        run: |
+          # Retry multiple times when using BuildPulse to detect and submit
+          # flakiness (because rspec-retry is disabled).
+          if [ -n "$HOMEBREW_BUILDPULSE_ACCESS_KEY_ID" ]; then
+            brew tests --online --coverage || \
+            brew tests --online --coverage || \
+            brew tests --online --coverage
+          else
+            brew tests --online --coverage
+          fi
         env:
           HOMEBREW_GITHUB_API_TOKEN: ${{ secrets.GITHUB_TOKEN }}
           # These cannot be queried at the macOS level on GitHub Actions.
           HOMEBREW_LANGUAGES: en-GB
+          HOMEBREW_BUILDPULSE_ACCESS_KEY_ID: ${{ secrets.BUILDPULSE_ACCESS_KEY_ID }}
+          HOMEBREW_BUILDPULSE_SECRET_ACCESS_KEY: ${{ secrets.BUILDPULSE_SECRET_ACCESS_KEY }}
+          HOMEBREW_BUILDPULSE_ACCOUNT_ID: 1503512
+          HOMEBREW_BUILDPULSE_REPOSITORY_ID: 53238813
 
       - run: brew test-bot --only-formulae --test-default-formula
 

Library/Homebrew/dev-cmd/tests.rb

@@ -36,6 +36,32 @@ def tests_args
     end
   end
 
+  def use_buildpulse?
+    return @use_buildpulse if defined?(@use_buildpulse)
+
+    @use_buildpulse = ENV["HOMEBREW_BUILDPULSE_ACCESS_KEY_ID"].present? &&
+                      ENV["HOMEBREW_BUILDPULSE_SECRET_ACCESS_KEY"].present? &&
+                      ENV["HOMEBREW_BUILDPULSE_ACCOUNT_ID"].present? &&
+                      ENV["HOMEBREW_BUILDPULSE_REPOSITORY_ID"].present?
+  end
+
+  def run_buildpulse
+    require "formula"
+
+    unless Formula["buildpulse-test-reporter"].any_version_installed?
+      ohai "Installing `buildpulse-test-reporter` for reporting test flakiness..."
+      safe_system HOMEBREW_BREW_FILE, "install", "buildpulse-test-reporter"
+    end
+
+    ENV["BUILDPULSE_ACCESS_KEY_ID"] = ENV["HOMEBREW_BUILDPULSE_ACCESS_KEY_ID"]
+    ENV["BUILDPULSE_SECRET_ACCESS_KEY"] = ENV["HOMEBREW_BUILDPULSE_SECRET_ACCESS_KEY"]
+
+    safe_system Formula["buildpulse-test-reporter"].opt_bin/"buildpulse-test-reporter",
+                "submit", "Library/Homebrew/test/junit",
+                "--account-id", ENV["HOMEBREW_BUILDPULSE_ACCOUNT_ID"],
+                "--repository-id", ENV["HOMEBREW_BUILDPULSE_REPOSITORY_ID"]
+  end
+
   def tests
     args = tests_args.parse
 
@@ -154,12 +180,18 @@ def tests
       # Let `bundle` in PATH find its gem.
       ENV["GEM_PATH"] = "#{ENV["GEM_PATH"]}:#{gem_user_dir}"
 
+      # Submit test flakiness information using BuildPulse
+      # BUILDPULSE used in spec_helper.rb
+      ENV["BUILDPULSE"] = "1" if use_buildpulse?
+
       if parallel
         system "bundle", "exec", "parallel_rspec", *parallel_args, "--", *bundle_args, "--", *files
       else
         system "bundle", "exec", "rspec", *bundle_args, "--", *files
       end
 
+      run_buildpulse if use_buildpulse?
+
       return if $CHILD_STATUS.success?
 
       Homebrew.failed = true

Library/Homebrew/test/spec_helper.rb

@@ -77,25 +77,35 @@
     c.max_formatted_output_length = 200
   end
 
-  # Use rspec-retry in CI.
+  # Use rspec-retry to handle flaky tests.
+  config.default_sleep_interval = 1
+
+  # Don't make retries as noisy unless in CI.
   if ENV["CI"]
     config.verbose_retry = true
     config.display_try_failure_messages = true
-    config.default_retry_count = 2
-    config.default_sleep_interval = 1
+  end
 
-    config.around(:each, :integration_test) do |example|
-      example.metadata[:timeout] ||= 120
-      example.run
-    end
+  # Don't want the nicer default retry behaviour when using BuildPulse to
+  # identify flaky tests.
+  config.default_retry_count = 2 unless ENV["BUILDPULSE"]
 
-    config.around(:each, :needs_network) do |example|
-      example.metadata[:timeout] ||= 120
-      example.metadata[:retry] ||= 4
-      example.metadata[:retry_wait] ||= 2
-      example.metadata[:exponential_backoff] ||= true
-      example.run
-    end
+  # Increase timeouts for integration tests (as we expect them to take longer).
+  config.around(:each, :integration_test) do |example|
+    example.metadata[:timeout] ||= 120
+    example.run
+  end
+
+  config.around(:each, :needs_network) do |example|
+    example.metadata[:timeout] ||= 120
+
+    # Don't want the nicer default retry behaviour when using BuildPulse to
+    # identify flaky tests.
+    example.metadata[:retry] ||= 4 unless ENV["BUILDPULSE"]
+
+    example.metadata[:retry_wait] ||= 2
+    example.metadata[:exponential_backoff] ||= true
+    example.run
   end
 
   # Never truncate output objects.