From a4c2e0e1b3b2c668e0849b65101c4326cfd47502 Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Tue, 29 Jun 2021 12:43:47 +0100 Subject: [PATCH] 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 | 15 ++++++++++- Library/Homebrew/dev-cmd/tests.rb | 32 +++++++++++++++++++++++ Library/Homebrew/test/spec_helper.rb | 38 ++++++++++++++++++---------- 3 files changed, 70 insertions(+), 15 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 2ec50a9860..732b515ce6 100644 --- a/.github/workflows/tests.yml +++ b/.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 diff --git a/Library/Homebrew/dev-cmd/tests.rb b/Library/Homebrew/dev-cmd/tests.rb index 57e0223a56..b372914455 100644 --- a/Library/Homebrew/dev-cmd/tests.rb +++ b/Library/Homebrew/dev-cmd/tests.rb @@ -36,6 +36,32 @@ module Homebrew 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 @@ module Homebrew # 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 diff --git a/Library/Homebrew/test/spec_helper.rb b/Library/Homebrew/test/spec_helper.rb index 6f68507de9..cfe778f7c5 100644 --- a/Library/Homebrew/test/spec_helper.rb +++ b/Library/Homebrew/test/spec_helper.rb @@ -77,25 +77,35 @@ RSpec.configure do |config| 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.