Simplify and clean up Sentry::RequestInterface's tests (#1660)

st0012 · web-flow · commit d5d045ba5008 · 2022-01-03T05:18:23.000+08:00
diff --git a/sentry-ruby/spec/sentry/interfaces/request_interface_spec.rb b/sentry-ruby/spec/sentry/interfaces/request_interface_spec.rb
@@ -3,10 +3,11 @@
 require 'spec_helper'
 
 RSpec.describe Sentry::RequestInterface do
-  let(:exception) { ZeroDivisionError.new("divided by 0") }
-  let(:additional_headers) { {} }
-  let(:env) { Rack::MockRequest.env_for("/test", additional_headers) }
-  let(:interface) { described_class.build(env: env) }
+  let(:env) { Rack::MockRequest.env_for("/test") }
+
+  subject do
+    described_class.build(env: env)
+  end
 
   before do
     Sentry.init do |config|
@@ -15,56 +16,49 @@
   end
 
   describe "rack_env_whitelist" do
-    it 'excludes non whitelisted params from rack env' do
-      additional_env = { "random_param" => "text", "query_string" => "test" }
-      new_env = env.merge(additional_env)
-      interface = described_class.build(env: new_env)
+    let(:additional_env) { { "random_param" => "text", "query_string" => "test" } }
+
+    before do
+      env.merge!(additional_env)
+    end
 
-      expect(interface.env).to_not include(additional_env)
+    it 'excludes non whitelisted params from rack env' do
+      expect(subject.env).to_not include(additional_env)
     end
 
     it 'formats rack env according to the provided whitelist' do
       Sentry.configuration.rack_env_whitelist = %w(random_param query_string)
-      additional_env = { "random_param" => "text", "query_string" => "test" }
-      new_env = env.merge(additional_env)
-      interface = described_class.build(env: new_env)
-
-      expect(interface.env).to eq(additional_env)
+      expect(subject.env).to eq(additional_env)
     end
 
     it 'keeps the original env intact when an empty whitelist is provided' do
       Sentry.configuration.rack_env_whitelist = []
-      interface = described_class.build(env: env)
 
-      expect(interface.env).to eq(env)
+      expect(subject.env).to eq(env)
     end
   end
 
   describe 'format headers' do
     let(:additional_headers) { { "HTTP_VERSION" => "HTTP/1.1", "HTTP_COOKIE" => "test", "HTTP_X_REQUEST_ID" => "12345678" } }
+    let(:env) { Rack::MockRequest.env_for("/test", additional_headers) }
 
     it 'transforms headers to conform with the interface' do
-      interface = described_class.build(env: env)
-
-      expect(interface.headers).to eq("Content-Length" => "0", "Version" => "HTTP/1.1", "X-Request-Id" => "12345678")
+      expect(subject.headers).to eq("Content-Length" => "0", "Version" => "HTTP/1.1", "X-Request-Id" => "12345678")
     end
 
     context 'from Rails middleware' do
       let(:additional_headers) { { "action_dispatch.request_id" => "12345678" } }
 
       it 'transforms headers to conform with the interface' do
-        interface = described_class.build(env: env)
-
-        expect(interface.headers).to eq("Content-Length" => "0", "X-Request-Id" => "12345678")
+        expect(subject.headers).to eq("Content-Length" => "0", "X-Request-Id" => "12345678")
       end
     end
 
     context 'with special characters' do
       let(:additional_headers) { { "HTTP_FOO" => "Tekirda\xC4" } }
 
       it "doesn't cause any issue" do
-        interface = described_class.build(env: env)
-        json = JSON.generate(interface.to_hash)
+        json = JSON.generate(subject.to_hash)
 
         expect(JSON.parse(json)["headers"]).to eq({"Content-Length"=>"0", "Foo"=>"Tekirda�"})
       end
@@ -83,56 +77,42 @@
   end
 
   it "doesn't capture cookies info" do
-    new_env = env.merge(
-      ::Rack::RACK_REQUEST_COOKIE_HASH => "cookies!"
-    )
+    env.merge!(::Rack::RACK_REQUEST_COOKIE_HASH => "cookies!")
 
-    interface = described_class.build(env: new_env)
-
-    expect(interface.cookies).to eq(nil)
-    expect(interface.env["COOKIE"]).to eq(nil)
+    expect(subject.cookies).to eq(nil)
+    expect(subject.env["COOKIE"]).to eq(nil)
   end
 
   describe "headers filtering" do
     it "filters out HTTP_COOKIE header" do
-      new_env = env.merge(
-        "HTTP_COOKIE" => "cookies!"
-      )
+      env.merge!("HTTP_COOKIE" => "cookies!")
 
-      interface = described_class.build(env: new_env)
-
-      expect(interface.headers["Cookie"]).to eq(nil)
+      expect(subject.headers["Cookie"]).to eq(nil)
     end
 
     it "filters out non-http headers" do
-      expect(interface.headers["Request-Method"]).to eq(nil)
+      expect(subject.headers["Request-Method"]).to eq(nil)
     end
 
     it "doesn't filter out CONTENT_TYPE or CONTENT_LENGTH headers" do
-      new_env = env.merge(
+      env.merge!(
         "CONTENT_LENGTH" => 10,
         "CONTENT_TYPE" => "text/html"
       )
 
-      interface = described_class.build(env: new_env)
-
-      expect(interface.headers["Content-Length"]).to eq("10")
-      expect(interface.headers["Content-Type"]).to eq("text/html")
+      expect(subject.headers["Content-Length"]).to eq("10")
+      expect(subject.headers["Content-Type"]).to eq("text/html")
     end
 
     it 'does not ignore version headers which do not match SERVER_PROTOCOL' do
-      new_env = env.merge("SERVER_PROTOCOL" => "HTTP/1.1", "HTTP_VERSION" => "HTTP/2.0")
+      env.merge!("SERVER_PROTOCOL" => "HTTP/1.1", "HTTP_VERSION" => "HTTP/2.0")
 
-      interface = described_class.build(env: new_env)
-
-      expect(interface.headers["Version"]).to eq("HTTP/2.0")
+      expect(subject.headers["Version"]).to eq("HTTP/2.0")
     end
 
     it 'retains any literal "HTTP-" in the actual header name' do
-      new_env = env.merge("HTTP_HTTP_CUSTOM_HTTP_HEADER" => "test")
-      interface = described_class.build(env: new_env)
-
-      expect(interface.headers).to include("Http-Custom-Http-Header" => "test")
+      env.merge!("HTTP_HTTP_CUSTOM_HTTP_HEADER" => "test")
+      expect(subject.headers).to include("Http-Custom-Http-Header" => "test")
     end
 
     it 'does not fail if an object in the env cannot be cast to string' do
@@ -142,39 +122,28 @@ def to_s
         end
       end.new
 
-      new_env = env.merge("HTTP_FOO" => "BAR", "rails_object" => obj)
+      env.merge!("HTTP_FOO" => "BAR", "rails_object" => obj)
 
-      expect { described_class.build(env: new_env) }.to_not raise_error
+      expect { described_class.build(env: env) }.to_not raise_error
     end
   end
 
   it "doesn't store request body by default" do
-    new_env = env.merge(
-      "REQUEST_METHOD" => "POST",
-      ::Rack::RACK_INPUT => StringIO.new("data=ignore me")
-    )
+    env.merge!("REQUEST_METHOD" => "POST", ::Rack::RACK_INPUT => StringIO.new("data=ignore me"))
 
-    interface = described_class.build(env: new_env)
-
-    expect(interface.data).to eq(nil)
+    expect(subject.data).to eq(nil)
   end
 
   it "doesn't store request body by default" do
-    new_env = env.merge(::Rack::RACK_INPUT => StringIO.new("ignore me"))
-
-    interface = described_class.build(env: new_env)
+    env.merge!(::Rack::RACK_INPUT => StringIO.new("ignore me"))
 
-    expect(interface.data).to eq(nil)
+    expect(subject.data).to eq(nil)
   end
 
   it "doesn't store query_string by default" do
-    new_env = env.merge(
-      "QUERY_STRING" => "token=xxxx"
-    )
+    env.merge!("QUERY_STRING" => "token=xxxx")
 
-    interface = described_class.build(env: new_env)
-
-    expect(interface.query_string).to eq(nil)
+    expect(subject.query_string).to eq(nil)
   end
 
   context "with config.send_default_pii = true" do
@@ -183,51 +152,34 @@ def to_s
     end
 
     it "stores cookies" do
-      new_env = env.merge(
-        ::Rack::RACK_REQUEST_COOKIE_HASH => "cookies!"
-      )
-
-      interface = described_class.build(env: new_env)
+      env.merge!(::Rack::RACK_REQUEST_COOKIE_HASH => "cookies!")
 
-      expect(interface.cookies).to eq("cookies!")
+      expect(subject.cookies).to eq("cookies!")
     end
 
     it "stores form data" do
-      new_env = env.merge(
-        "REQUEST_METHOD" => "POST",
-        ::Rack::RACK_INPUT => StringIO.new("data=catch me")
-      )
-
-      interface = described_class.build(env: new_env)
+      env.merge!("REQUEST_METHOD" => "POST", ::Rack::RACK_INPUT => StringIO.new("data=catch me"))
 
-      expect(interface.data).to eq({ "data" => "catch me" })
+      expect(subject.data).to eq({ "data" => "catch me" })
     end
 
     it "stores query string" do
-      new_env = env.merge(
-        "QUERY_STRING" => "token=xxxx"
-      )
-
-      interface = described_class.build(env: new_env)
+      env.merge!("QUERY_STRING" => "token=xxxx")
 
-      expect(interface.query_string).to eq("token=xxxx")
+      expect(subject.query_string).to eq("token=xxxx")
     end
 
     it "stores request body" do
-      new_env = env.merge(::Rack::RACK_INPUT => StringIO.new("catch me"))
+      env.merge!(::Rack::RACK_INPUT => StringIO.new("catch me"))
 
-      interface = described_class.build(env: new_env)
-
-      expect(interface.data).to eq("catch me")
+      expect(subject.data).to eq("catch me")
     end
 
     it "force encodes request body to avoid encoding issue" do
-      new_env = env.merge(::Rack::RACK_INPUT => StringIO.new("あ"))
-
-      interface = described_class.build(env: new_env)
+      env.merge!(::Rack::RACK_INPUT => StringIO.new("あ"))
 
       expect do
-        JSON.generate(interface.to_hash)
+        JSON.generate(subject.to_hash)
       end.not_to raise_error
     end
 
@@ -241,12 +193,10 @@ def to_s
         "HTTP_X_FORWARDED_FOR" => ip
       )
 
-      interface = described_class.build(env: env)
-
-      expect(interface.env).to include("REMOTE_ADDR")
-      expect(interface.headers.keys).to include("Client-Ip")
-      expect(interface.headers.keys).to include("X-Real-Ip")
-      expect(interface.headers.keys).to include("X-Forwarded-For")
+      expect(subject.env).to include("REMOTE_ADDR")
+      expect(subject.headers.keys).to include("Client-Ip")
+      expect(subject.headers.keys).to include("X-Real-Ip")
+      expect(subject.headers.keys).to include("X-Forwarded-For")
     end
   end
 end
