Fix flaky specs (#2561)

* Set up webmock for specs

* Replace faulty spec with something more useful

Not sure what the intention was but this spec
did not test any faraday error handling. What
actually happened was that HTTPTransport really
tried to hit the API and ended up with error.

We *do not* want to hit any APIs like that in specs,
this causes instability of the spec suite.

That's why I added webmock to ensure we're not sending
any HTTP requests in specs.

* Stub requests in event sending spec

This spec was very flaky

* Fix faraday spec even though it sends HTTP request

I'll figure it out eventually

* Use sentry request stubbing when needed

* DRY up toggling webmock in specs

* Fix problematic spec

Author: solnic

sentry-ruby/Gemfile

@@ -33,3 +33,4 @@ gem "yard", github: "lsegal/yard"
 gem "webrick"
 gem "faraday"
 gem "excon"
+gem "webmock"

sentry-ruby/lib/sentry/transport/http_transport.rb

@@ -23,7 +23,6 @@ class HTTPTransport < Transport
       Zlib::BufError, Errno::EHOSTUNREACH, Errno::ECONNREFUSED
     ].freeze
 
-
     def initialize(*args)
       super
       log_debug("Sentry HTTP Transport will connect to #{@dsn.server}") if @dsn

sentry-ruby/spec/contexts/with_request_mock.rb

@@ -12,7 +12,13 @@ def setsockopt(*args); end
     allow(TCPSocket).to receive(:open).and_return(FakeSocket.new)
   end
 
-  def stub_request(fake_response, &block)
+  around do |example|
+    WebMock.disable!
+    example.run
+    WebMock.enable!
+  end
+
+  def sentry_stub_request(fake_response, &block)
     allow_any_instance_of(Net::HTTP).to receive(:connect)
     allow_any_instance_of(Net::HTTP).to receive(:transport_request) do |http_obj, request|
       block.call(request, http_obj) if block
@@ -32,10 +38,10 @@ def build_fake_response(status, body: {}, headers: {})
 
   def stub_sentry_response
     # use bad request as an example is easier for verifying with error messages
-    stub_request(build_fake_response("400", body: { data: "bad sentry DSN public key" }))
+    sentry_stub_request(build_fake_response("400", body: { data: "bad sentry DSN public key" }))
   end
 
   def stub_normal_response(code: "200", &block)
-    stub_request(build_fake_response(code), &block)
+    sentry_stub_request(build_fake_response(code), &block)
   end
 end

sentry-ruby/spec/sentry/client/event_sending_spec.rb

@@ -10,6 +10,11 @@
       config.transport.transport_class = Sentry::DummyTransport
     end
   end
+
+  before do
+    stub_request(:post, Sentry::TestHelper::DUMMY_DSN)
+  end
+
   subject(:client) { Sentry::Client.new(configuration) }
 
   let(:hub) do
@@ -470,13 +475,16 @@
         before do
           configuration.background_worker_threads = 0
           Sentry.background_worker = Sentry::BackgroundWorker.new(configuration)
+
+          stub_request(:post, "http://sentry.localdomain/sentry/api/42/envelope/")
+            .to_raise(Timeout::Error)
         end
 
         it "swallows and logs Sentry::ExternalError (caused by transport's networking error)" do
           expect(client.capture_event(event, scope)).to be_nil
 
-          expect(string_io.string).to match(/Event sending failed: Failed to open TCP connection/)
-          expect(string_io.string).to match(/Event capturing failed: Failed to open TCP connection/)
+          expect(string_io.string).to match(/Event sending failed: Exception from WebMock/)
+          expect(string_io.string).to match(/Event capturing failed: Exception from WebMock/)
         end
 
         it "swallows and logs errors caused by the user (like in before_send)" do
@@ -502,13 +510,16 @@
       context "when sending events in background causes error", retry: 3 do
         before do
           Sentry.background_worker = Sentry::BackgroundWorker.new(configuration)
+
+          stub_request(:post, "http://sentry.localdomain/sentry/api/42/envelope/")
+            .to_raise(Timeout::Error)
         end
 
         it "swallows and logs Sentry::ExternalError (caused by transport's networking error)" do
           expect(client.capture_event(event, scope)).to be_a(Sentry::ErrorEvent)
           sleep(0.2)
 
-          expect(string_io.string).to match(/Event sending failed: Failed to open TCP connection/)
+          expect(string_io.string).to match(/Event sending failed: Exception from WebMock/)
         end
 
         it "swallows and logs errors caused by the user (like in before_send)" do
@@ -560,11 +571,14 @@
     describe "#send_event" do
       context "error happens when sending the event" do
         it "raises the error" do
+          stub_request(:post, "http://sentry.localdomain/sentry/api/42/envelope/")
+            .to_raise(Timeout::Error)
+
           expect do
             client.send_event(event)
           end.to raise_error(Sentry::ExternalError)
 
-          expect(string_io.string).to match(/Event sending failed: Failed to open TCP connection/)
+          expect(string_io.string).to match(/Event sending failed: Exception from WebMock/)
         end
       end
 
@@ -635,6 +649,9 @@
         end
 
         it "records lost span delta client reports" do
+          stub_request(:post, "http://sentry.localdomain/sentry/api/42/envelope/")
+            .to_raise(Timeout::Error)
+
           expect { client.send_event(transaction_event) }.to raise_error(Sentry::ExternalError)
           expect(client.transport).to have_recorded_lost_event(:before_send, 'span', num: 2)
         end

sentry-ruby/spec/sentry/faraday_spec.rb

@@ -271,7 +271,7 @@
 
     let(:url) { "http://example.com" }
 
-    it "skips instrumentation" do
+    it "skips instrumentation", webmock: false do
       transaction = Sentry.start_transaction
       Sentry.get_current_scope.set_span(transaction)
 

sentry-ruby/spec/sentry/net/http_spec.rb

@@ -11,38 +11,32 @@
     ::Logger.new(string_io)
   end
 
-  context "with IPv6 addresses" do
+  context "with tracing enabled" do
     before do
       perform_basic_setup do |config|
         config.traces_sample_rate = 1.0
+        config.transport.transport_class = Sentry::HTTPTransport
+        config.logger = logger
+        # the dsn needs to have a real host so we can make a real connection before sending a failed request
+        config.dsn = 'http://[email protected]/5434472'
       end
     end
 
-    it "correctly parses the short-hand IPv6 addresses" do
-      stub_normal_response
-
-      transaction = Sentry.start_transaction
-      Sentry.get_current_scope.set_span(transaction)
+    context "with IPv6 addresses" do
+      it "correctly parses the short-hand IPv6 addresses" do
+        stub_normal_response
 
-      _ = Net::HTTP.get("::1", "/path", 8080)
+        transaction = Sentry.start_transaction
+        Sentry.get_current_scope.set_span(transaction)
 
-      expect(transaction.span_recorder.spans.count).to eq(2)
+        _ = Net::HTTP.get("::1", "/path", 8080)
 
-      request_span = transaction.span_recorder.spans.last
-      expect(request_span.data).to eq(
-        { "url" => "http://[::1]/path", "http.request.method" => "GET", "http.response.status_code" => 200 }
-      )
-    end
-  end
+        expect(transaction.span_recorder.spans.count).to eq(2)
 
-  context "with tracing enabled" do
-    before do
-      perform_basic_setup do |config|
-        config.traces_sample_rate = 1.0
-        config.transport.transport_class = Sentry::HTTPTransport
-        config.logger = logger
-        # the dsn needs to have a real host so we can make a real connection before sending a failed request
-        config.dsn = 'http://[email protected]/5434472'
+        request_span = transaction.span_recorder.spans.last
+        expect(request_span.data).to eq(
+          { "url" => "http://[::1]/path", "http.request.method" => "GET", "http.response.status_code" => 200 }
+        )
       end
     end
 

sentry-ruby/spec/sentry/rspec/matchers_spec.rb

@@ -12,6 +12,7 @@
       config.dsn = 'https://[email protected]/5434472'
       config.enabled_environments = ["production"]
       config.environment = :test
+      config.transport.transport_class = Sentry::DummyTransport
     end
 
     setup_sentry_test

sentry-ruby/spec/sentry/transport/http_transport_rate_limiting_spec.rb

@@ -107,7 +107,7 @@
 
   describe "rate limit header processing" do
     before do
-      stub_request(fake_response)
+      sentry_stub_request(fake_response)
     end
 
     shared_examples "rate limiting headers handling" do

sentry-ruby/spec/sentry/transport/http_transport_spec.rb

@@ -22,7 +22,7 @@
   subject { client.transport }
 
   it "logs a debug message only during initialization" do
-    stub_request(build_fake_response("200"))
+    sentry_stub_request(build_fake_response("200"))
     string_io = StringIO.new
     configuration.logger = Logger.new(string_io)
 
@@ -39,7 +39,7 @@
   end
 
   it "initializes new Net::HTTP instance for every request" do
-    stub_request(build_fake_response("200")) do |request|
+    sentry_stub_request(build_fake_response("200")) do |request|
       expect(request["User-Agent"]).to eq("sentry-ruby/#{Sentry::VERSION}")
     end
 
@@ -87,7 +87,7 @@
     let(:fake_response) { build_fake_response("200") }
 
     it 'sets default User-Agent' do
-      stub_request(fake_response) do |request|
+      sentry_stub_request(fake_response) do |request|
         expect(request["User-Agent"]).to eq("sentry-ruby/#{Sentry::VERSION}")
       end
 
@@ -97,7 +97,7 @@
     it "accepts custom proxy" do
       configuration.transport.proxy = { uri:  URI("https://example.com"), user: "stan", password: "foobar" }
 
-      stub_request(fake_response) do |_, http_obj|
+      sentry_stub_request(fake_response) do |_, http_obj|
         expect(http_obj.proxy_address).to eq("example.com")
         expect(http_obj.proxy_user).to eq("stan")
         expect(http_obj.proxy_pass).to eq("foobar")
@@ -109,7 +109,7 @@
     it "accepts a custom proxy string" do
       configuration.transport.proxy = "https://stan:[email protected]:8080"
 
-      stub_request(fake_response) do |_, http_obj|
+      sentry_stub_request(fake_response) do |_, http_obj|
         expect(http_obj.proxy_address).to eq("example.com")
         expect(http_obj.proxy_user).to eq("stan")
         expect(http_obj.proxy_pass).to eq("foobar")
@@ -122,7 +122,7 @@
     it "accepts a custom proxy URI" do
       configuration.transport.proxy = URI("https://stan:[email protected]:8080")
 
-      stub_request(fake_response) do |_, http_obj|
+      sentry_stub_request(fake_response) do |_, http_obj|
         expect(http_obj.proxy_address).to eq("example.com")
         expect(http_obj.proxy_user).to eq("stan")
         expect(http_obj.proxy_pass).to eq("foobar")
@@ -136,7 +136,7 @@
       begin
         ENV["http_proxy"] = "https://stan:[email protected]:8080"
 
-        stub_request(fake_response) do |_, http_obj|
+        sentry_stub_request(fake_response) do |_, http_obj|
           expect(http_obj.proxy_address).to eq("example.com")
           expect(http_obj.proxy_port).to eq(8080)
 
@@ -155,7 +155,7 @@
     it "accepts custom timeout" do
       configuration.transport.timeout = 10
 
-      stub_request(fake_response) do |_, http_obj|
+      sentry_stub_request(fake_response) do |_, http_obj|
         expect(http_obj.read_timeout).to eq(10)
 
         if Gem::Version.new(RUBY_VERSION) >= Gem::Version.new("2.6")
@@ -169,7 +169,7 @@
     it "accepts custom open_timeout" do
       configuration.transport.open_timeout = 10
 
-      stub_request(fake_response) do |_, http_obj|
+      sentry_stub_request(fake_response) do |_, http_obj|
         expect(http_obj.open_timeout).to eq(10)
       end
 
@@ -178,7 +178,7 @@
 
     describe "ssl configurations" do
       it "has the corrent default" do
-        stub_request(fake_response) do |_, http_obj|
+        sentry_stub_request(fake_response) do |_, http_obj|
           expect(http_obj.verify_mode).to eq(1)
           expect(http_obj.ca_file).to eq(nil)
         end
@@ -189,7 +189,7 @@
       it "accepts custom ssl_verification configuration" do
         configuration.transport.ssl_verification = false
 
-        stub_request(fake_response) do |_, http_obj|
+        sentry_stub_request(fake_response) do |_, http_obj|
           expect(http_obj.verify_mode).to eq(0)
           expect(http_obj.ca_file).to eq(nil)
         end
@@ -200,7 +200,7 @@
       it "accepts custom ssl_ca_file configuration" do
         configuration.transport.ssl_ca_file = "/tmp/foo"
 
-        stub_request(fake_response) do |_, http_obj|
+        sentry_stub_request(fake_response) do |_, http_obj|
           expect(http_obj.verify_mode).to eq(1)
           expect(http_obj.ca_file).to eq("/tmp/foo")
         end
@@ -211,7 +211,7 @@
       it "accepts custom ssl configuration" do
         configuration.transport.ssl  = { verify: false, ca_file: "/tmp/foo" }
 
-        stub_request(fake_response) do |_, http_obj|
+        sentry_stub_request(fake_response) do |_, http_obj|
           expect(http_obj.verify_mode).to eq(0)
           expect(http_obj.ca_file).to eq("/tmp/foo")
         end
@@ -225,7 +225,7 @@
     let(:fake_response) { build_fake_response("200") }
 
     it "compresses data by default" do
-      stub_request(fake_response) do |request|
+      sentry_stub_request(fake_response) do |request|
         expect(request["Content-Type"]).to eq("application/x-sentry-envelope")
         expect(request["Content-Encoding"]).to eq("gzip")
 
@@ -238,7 +238,7 @@
     end
 
     it "doesn't compress small event" do
-      stub_request(fake_response) do |request|
+      sentry_stub_request(fake_response) do |request|
         expect(request["Content-Type"]).to eq("application/x-sentry-envelope")
         expect(request["Content-Encoding"]).to eq("")
 
@@ -255,7 +255,7 @@
     it "doesn't compress data if the encoding is not gzip" do
       configuration.transport.encoding = "json"
 
-      stub_request(fake_response) do |request|
+      sentry_stub_request(fake_response) do |request|
         expect(request["Content-Type"]).to eq("application/x-sentry-envelope")
         expect(request["Content-Encoding"]).to eq("")
 
@@ -296,7 +296,7 @@
       let(:fake_response) { build_fake_response("404") }
 
       it "raises an error" do
-        stub_request(fake_response)
+        sentry_stub_request(fake_response)
 
         expect { subject.send_data(data) }.to raise_error(Sentry::ExternalError, /the server responded with status 404/)
       end
@@ -306,7 +306,7 @@
       let(:fake_response) { build_fake_response("500") }
 
       it "raises an error" do
-        stub_request(fake_response)
+        sentry_stub_request(fake_response)
 
         expect { subject.send_data(data) }.to raise_error(Sentry::ExternalError, /the server responded with status 500/)
       end
@@ -318,7 +318,7 @@
       end
 
       it "raises an error with header" do
-        stub_request(error_response)
+        sentry_stub_request(error_response)
 
         expect { subject.send_data(data) }.to raise_error(Sentry::ExternalError, /error_in_header/)
       end

sentry-ruby/spec/sentry/transport_spec.rb

@@ -531,11 +531,14 @@
         end
       end
 
-      context "with Faraday::Error" do
-        it "raises the error" do
-          expect do
-            subject.send_event(event)
-          end.to raise_error(Sentry::ExternalError)
+      context "with an HTTP exception" do
+        before do
+          stub_request(:post, "http://sentry.localdomain:80/sentry/api/42/envelope/").
+          to_raise(Timeout::Error)
+        end
+
+        it "raises Sentry::ExternalError" do
+          expect { subject.send_event(event) }.to raise_error(Sentry::ExternalError)
         end
       end
     end