From 0cdec780d59c0d143e1ce1093c901b5a7558a8cb Mon Sep 17 00:00:00 2001
From: Steven Webb <steve@buildkite.com>
Date: Thu, 28 Jul 2022 20:45:36 +0800
Subject: [PATCH 1/2] Suppress errors when connecting to Buildkite

It's important the collector does not raise exceptions and cause test
suites to fail. Some customers have seen exceptions raised when
connecting to Buildkite (not a surprise as networks are complicated and
unreliable).

This PR modifies the connection process to catch all StandardErrors and
log them. Trace data will be lost; however, the customer's build will
not fail which is more important.

We also intend to address the underlying issues; however, this PR is an
important defensive measure to protect customer builds.
---
 lib/buildkite/test_collector.rb               |  6 +++++
 .../test_collector/library_hooks/minitest.rb  |  2 +-
 .../test_collector/library_hooks/rspec.rb     |  2 +-
 spec/test_collector_spec.rb                   | 23 +++++++++++++++++++
 4 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/lib/buildkite/test_collector.rb b/lib/buildkite/test_collector.rb
index 010175b..535d68e 100644
--- a/lib/buildkite/test_collector.rb
+++ b/lib/buildkite/test_collector.rb
@@ -101,5 +101,11 @@ def self.enable_tracing!
         Buildkite::TestCollector::Uploader.tracer&.backfill(:sql, finish - start, **{ query: payload[:sql] })
       end
     end
+
+    def self.safe(&block)
+      block.call
+    rescue StandardError => e
+      logger.error("Buildkite::TestCollector received exception: #{e}")
+    end
   end
 end
diff --git a/lib/buildkite/test_collector/library_hooks/minitest.rb b/lib/buildkite/test_collector/library_hooks/minitest.rb
index 0a302aa..07aeb4e 100644
--- a/lib/buildkite/test_collector/library_hooks/minitest.rb
+++ b/lib/buildkite/test_collector/library_hooks/minitest.rb
@@ -12,4 +12,4 @@ class Minitest::Test
 
 Buildkite::TestCollector.enable_tracing!
 
-Buildkite::TestCollector::Uploader.configure
+Buildkite::TestCollector.safe { Buildkite::TestCollector::Uploader.configure }
diff --git a/lib/buildkite/test_collector/library_hooks/rspec.rb b/lib/buildkite/test_collector/library_hooks/rspec.rb
index edc25f0..fc385b6 100644
--- a/lib/buildkite/test_collector/library_hooks/rspec.rb
+++ b/lib/buildkite/test_collector/library_hooks/rspec.rb
@@ -12,7 +12,7 @@
   config.before(:suite) do
     config.add_formatter Buildkite::TestCollector::RSpecPlugin::Reporter
 
-    Buildkite::TestCollector::Uploader.configure
+    Buildkite::TestCollector.safe { Buildkite::TestCollector::Uploader.configure }
   end
 
   config.around(:each) do |example|
diff --git a/spec/test_collector_spec.rb b/spec/test_collector_spec.rb
index d8318a8..34123f8 100644
--- a/spec/test_collector_spec.rb
+++ b/spec/test_collector_spec.rb
@@ -24,4 +24,27 @@
       expect(analytics.url).to eq "https://analytics-api.buildkite.com/v1/uploads"
     end
   end
+
+  describe ".safe" do
+    let(:logger) { TestLogger.new }
+
+    class TestLogger
+      attr_reader :errors
+
+      def initialize
+        @errors = []
+      end
+
+      def error(message)
+        @errors << message
+      end
+    end
+
+    before { Buildkite::TestCollector.logger = logger }
+
+    it "suppresses exceptions and logs them to logger.error" do
+      expect{ described_class.safe { raise "penguines dance" } }.to_not raise_error
+      expect(logger.errors).to eq(["Buildkite::TestCollector received exception: penguines dance"])
+    end
+  end
 end

From 538d26a30855b64b669fe149c08bcd1e684e6fcc Mon Sep 17 00:00:00 2001
From: Steven Webb <steve@buildkite.com>
Date: Fri, 29 Jul 2022 11:59:05 +0800
Subject: [PATCH 2/2] Log backtrace

---
 lib/buildkite/test_collector.rb | 1 +
 spec/test_collector_spec.rb     | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/buildkite/test_collector.rb b/lib/buildkite/test_collector.rb
index 535d68e..059a1b5 100644
--- a/lib/buildkite/test_collector.rb
+++ b/lib/buildkite/test_collector.rb
@@ -106,6 +106,7 @@ def self.safe(&block)
       block.call
     rescue StandardError => e
       logger.error("Buildkite::TestCollector received exception: #{e}")
+      logger.error("Backtrace:\n#{e.backtrace.join("\n")}")
     end
   end
 end
diff --git a/spec/test_collector_spec.rb b/spec/test_collector_spec.rb
index 34123f8..31c7fe2 100644
--- a/spec/test_collector_spec.rb
+++ b/spec/test_collector_spec.rb
@@ -44,7 +44,7 @@ def error(message)
 
     it "suppresses exceptions and logs them to logger.error" do
       expect{ described_class.safe { raise "penguines dance" } }.to_not raise_error
-      expect(logger.errors).to eq(["Buildkite::TestCollector received exception: penguines dance"])
+      expect(logger.errors.first).to eq("Buildkite::TestCollector received exception: penguines dance")
     end
   end
 end