From f38707e92ad42695eaa02334897ffb5ed9f4f54b Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Wed, 27 Jan 2021 12:43:28 +0000 Subject: [PATCH 1/4] spec_helper: fix and improve retry logic. - always retry each test at least once (confusingly this means a retry count of 2 rather than 1) - always wait at least 1 second between retries - set a default retry metadata for integration tests rather than overriding any specified values - use `example.run` rather than `example.run_with_retry` for integration tests because, confusingly, this avoids having the retry count be half what it should be (because the attempts increases by one for each `run_with_retry` call) - use 4 retries for integration tests with 2**attempts*retry_wait (retry wait now being 2) to actually have more retries than before this commit (but with exponential backoff) This should generally improve test flakiness in CI but particularly improve the cleanup test flake we've seen recently. --- Library/Homebrew/test/spec_helper.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Library/Homebrew/test/spec_helper.rb b/Library/Homebrew/test/spec_helper.rb index 1a4bf7e1a4..da055f6c4e 100644 --- a/Library/Homebrew/test/spec_helper.rb +++ b/Library/Homebrew/test/spec_helper.rb @@ -80,6 +80,8 @@ RSpec.configure do |config| if ENV["CI"] config.verbose_retry = true config.display_try_failure_messages = true + config.default_retry_count = 2 + config.default_sleep_interval = 1 config.around(:each, :integration_test) do |example| example.metadata[:timeout] ||= 120 @@ -88,7 +90,10 @@ RSpec.configure do |config| config.around(:each, :needs_network) do |example| example.metadata[:timeout] ||= 120 - example.run_with_retry retry: 5, retry_wait: 5 + example.metadata[:retry] ||= 4 + example.metadata[:retry_wait] ||= 2 + example.metadata[:exponential_backoff] ||= true + example.run end end From da94957b018ff5d97ee1478f06cdfcf721d8b668 Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Wed, 27 Jan 2021 15:04:13 +0000 Subject: [PATCH 2/4] tests: verbose tests with --verbose or --debug. The `VERBOSE_TESTS` variable was from cask and never gets set (and is unset by `bin/brew`). Replace it with `HOMEBREW_VERBOSE_TESTS` and set it by `--verbose` or `--debug`. While we're here, remove an unneeded `VERBOSE` delete (as it's already done by `bin/brew`). --- Library/Homebrew/dev-cmd/tests.rb | 4 +++- Library/Homebrew/test/spec_helper.rb | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/Library/Homebrew/dev-cmd/tests.rb b/Library/Homebrew/dev-cmd/tests.rb index ef58dd8bd7..6725bd3f48 100644 --- a/Library/Homebrew/dev-cmd/tests.rb +++ b/Library/Homebrew/dev-cmd/tests.rb @@ -48,7 +48,6 @@ module Homebrew ENV.delete("HOMEBREW_NO_COLOR") ENV.delete("HOMEBREW_VERBOSE") ENV.delete("HOMEBREW_DEBUG") - ENV.delete("VERBOSE") ENV.delete("HOMEBREW_CASK_OPTS") ENV.delete("HOMEBREW_TEMP") ENV.delete("HOMEBREW_NO_GITHUB_API") @@ -68,6 +67,9 @@ module Homebrew # to use GPG to sign by default ENV["HOME"] = "#{HOMEBREW_LIBRARY_PATH}/test" + # Print verbose output when requesting debug or verbose output. + ENV["HOMEBREW_VERBOSE_TESTS"] = "1" if args.debug? || args.verbose? + if args.coverage? ENV["HOMEBREW_TESTS_COVERAGE"] = "1" FileUtils.rm_f "test/coverage/.resultset.json" diff --git a/Library/Homebrew/test/spec_helper.rb b/Library/Homebrew/test/spec_helper.rb index da055f6c4e..df5a9906d3 100644 --- a/Library/Homebrew/test/spec_helper.rb +++ b/Library/Homebrew/test/spec_helper.rb @@ -201,7 +201,7 @@ RSpec.configure do |config| @__stderr = $stderr.clone begin - if (example.metadata.keys & [:focus, :byebug]).empty? && !ENV.key?("VERBOSE_TESTS") + if (example.metadata.keys & [:focus, :byebug]).empty? && !ENV.key?("HOMEBREW_VERBOSE_TESTS") $stdout.reopen(File::NULL) $stderr.reopen(File::NULL) end From 3ebc64a71dd885d1fea615dae53e658c9cee74ed Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Wed, 27 Jan 2021 15:04:35 +0000 Subject: [PATCH 3/4] rubocop_rspec: don't autocorrect focus. It's annoying to have these autoremoved. --- Library/.rubocop_rspec.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Library/.rubocop_rspec.yml b/Library/.rubocop_rspec.yml index 6b72706d06..5a47d33451 100644 --- a/Library/.rubocop_rspec.yml +++ b/Library/.rubocop_rspec.yml @@ -35,3 +35,7 @@ RSpec/NestedGroups: Max: 5 RSpec/MultipleMemoizedHelpers: Max: 12 + +# Annoying to have these autoremoved. +RSpec/Focus: + AutoCorrect: false From 3724c739f880945ec76a16ef8b9209c6440f0035 Mon Sep 17 00:00:00 2001 From: Mike McQuaid Date: Wed, 27 Jan 2021 15:06:06 +0000 Subject: [PATCH 4/4] cleanup_spec: inline path creation. Let's see if this makes the test more reliable. --- Library/Homebrew/test/cleanup_spec.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Library/Homebrew/test/cleanup_spec.rb b/Library/Homebrew/test/cleanup_spec.rb index f8c363ad44..bdff8c4f7a 100644 --- a/Library/Homebrew/test/cleanup_spec.rb +++ b/Library/Homebrew/test/cleanup_spec.rb @@ -199,11 +199,8 @@ describe Homebrew::Cleanup do describe "::cleanup_logs" do let(:path) { (HOMEBREW_LOGS/"delete_me") } - before do - path.mkpath - end - it "cleans all logs if prune is 0" do + path.mkpath described_class.new(days: 0).cleanup_logs expect(path).not_to exist end @@ -211,6 +208,7 @@ describe Homebrew::Cleanup do it "cleans up logs if older than 30 days" do allow_any_instance_of(Pathname).to receive(:ctime).and_return(31.days.ago) allow_any_instance_of(Pathname).to receive(:mtime).and_return(31.days.ago) + path.mkpath subject.cleanup_logs expect(path).not_to exist end @@ -218,6 +216,7 @@ describe Homebrew::Cleanup do it "does not clean up logs less than 30 days old" do allow_any_instance_of(Pathname).to receive(:ctime).and_return(15.days.ago) allow_any_instance_of(Pathname).to receive(:mtime).and_return(15.days.ago) + path.mkpath subject.cleanup_logs expect(path).to exist end