From 76905599ce7615663cdcde8156fa5b8de2882168 Mon Sep 17 00:00:00 2001 From: Tom de Bruijn Date: Mon, 1 Jul 2024 15:34:04 +0200 Subject: [PATCH] Test transaction contents through extension Refactor the test suite to not test if transactions are created by asserting method calls. Use the tooling to see if a transaction has been created or not and assert what's set on the transaction using the transaction matchers. Part of #299 Closes #252 --- spec/lib/appsignal/hooks/action_cable_spec.rb | 11 -- .../start_finish_shared_examples.rb | 3 - spec/lib/appsignal/hooks/net_http_spec.rb | 49 +----- .../appsignal/integrations/net_http_spec.rb | 33 ++++ .../lib/appsignal/integrations/object_spec.rb | 65 ++++--- .../appsignal/integrations/padrino_spec.rb | 112 ++++++------ .../appsignal/integrations/webmachine_spec.rb | 67 +++----- .../appsignal/rack/streaming_listener_spec.rb | 154 ++++++++--------- spec/lib/appsignal/transaction_spec.rb | 4 - spec/lib/appsignal_spec.rb | 159 +++++++++++------- spec/support/helpers/transaction_helpers.rb | 8 + spec/support/mocks/dummy_app.rb | 6 +- 12 files changed, 319 insertions(+), 352 deletions(-) create mode 100644 spec/lib/appsignal/integrations/net_http_spec.rb diff --git a/spec/lib/appsignal/hooks/action_cable_spec.rb b/spec/lib/appsignal/hooks/action_cable_spec.rb index a016430e5..f59f3dff8 100644 --- a/spec/lib/appsignal/hooks/action_cable_spec.rb +++ b/spec/lib/appsignal/hooks/action_cable_spec.rb @@ -53,14 +53,6 @@ def self.to_s set_current_transaction(transaction) - expect(Appsignal::Transaction).to receive(:create).with( - transaction_id, - Appsignal::Transaction::ACTION_CABLE, - kind_of(ActionDispatch::Request) - ) - .and_return(transaction) - allow(Appsignal::Transaction).to receive(:current).and_return(transaction) - # Stub transmit call for subscribe/unsubscribe tests allow(connection).to receive(:websocket) .and_return(instance_double("ActionCable::Connection::WebSocket", :transmit => nil)) @@ -122,9 +114,6 @@ def self.to_s kind_of(ActionDispatch::Request) ).and_return(action_transaction) allow(Appsignal::Transaction).to receive(:current).and_return(action_transaction) - # Stub complete call, stops it from being cleared in the extension - # And allows us to call `#to_h` on it after it's been completed. - expect(action_transaction.ext).to receive(:complete) instance.perform_action("message" => "foo", "action" => "speak") expect(action_transaction).to have_id(transaction_id) diff --git a/spec/lib/appsignal/hooks/active_support_notifications/start_finish_shared_examples.rb b/spec/lib/appsignal/hooks/active_support_notifications/start_finish_shared_examples.rb index 3ed1f7db4..bc3bcfe32 100644 --- a/spec/lib/appsignal/hooks/active_support_notifications/start_finish_shared_examples.rb +++ b/spec/lib/appsignal/hooks/active_support_notifications/start_finish_shared_examples.rb @@ -28,9 +28,6 @@ end it "does not instrument events whose name starts with a bang" do - expect(Appsignal::Transaction.current).not_to receive(:start_event) - expect(Appsignal::Transaction.current).not_to receive(:finish_event) - instrumenter.start("!sql.active_record", {}) instrumenter.finish("!sql.active_record", {}) diff --git a/spec/lib/appsignal/hooks/net_http_spec.rb b/spec/lib/appsignal/hooks/net_http_spec.rb index 369822a3f..8ebb1159c 100644 --- a/spec/lib/appsignal/hooks/net_http_spec.rb +++ b/spec/lib/appsignal/hooks/net_http_spec.rb @@ -1,51 +1,16 @@ describe Appsignal::Hooks::NetHttpHook do - before :context do - start_agent - end + before(:context) { start_agent } - context "with Net::HTTP instrumentation enabled" do - describe "#dependencies_present?" do - subject { described_class.new.dependencies_present? } + describe "#dependencies_present?" do + subject { described_class.new.dependencies_present? } + context "with Net::HTTP instrumentation enabled" do it { is_expected.to be_truthy } end - it "should instrument a http request" do - Appsignal::Transaction.create("uuid", Appsignal::Transaction::HTTP_REQUEST, "test") - expect(Appsignal::Transaction.current).to receive(:start_event) - .at_least(:once) - expect(Appsignal::Transaction.current).to receive(:finish_event) - .at_least(:once) - .with("request.net_http", "GET http://www.google.com", nil, 0) - - stub_request(:any, "http://www.google.com/") - - Net::HTTP.get_response(URI.parse("http://www.google.com")) - end - - it "should instrument a https request" do - Appsignal::Transaction.create("uuid", Appsignal::Transaction::HTTP_REQUEST, "test") - expect(Appsignal::Transaction.current).to receive(:start_event) - .at_least(:once) - expect(Appsignal::Transaction.current).to receive(:finish_event) - .at_least(:once) - .with("request.net_http", "GET https://www.google.com", nil, 0) - - stub_request(:any, "https://www.google.com/") - - uri = URI.parse("https://www.google.com") - http = Net::HTTP.new(uri.host, uri.port) - http.use_ssl = true - http.get(uri.request_uri) - end - end - - context "with Net::HTTP instrumentation disabled" do - before { Appsignal.config.config_hash[:instrument_net_http] = false } - after { Appsignal.config.config_hash[:instrument_net_http] = true } - - describe "#dependencies_present?" do - subject { described_class.new.dependencies_present? } + context "with Net::HTTP instrumentation disabled" do + before { Appsignal.config.config_hash[:instrument_net_http] = false } + after { Appsignal.config.config_hash[:instrument_net_http] = true } it { is_expected.to be_falsy } end diff --git a/spec/lib/appsignal/integrations/net_http_spec.rb b/spec/lib/appsignal/integrations/net_http_spec.rb new file mode 100644 index 000000000..bbfd8da04 --- /dev/null +++ b/spec/lib/appsignal/integrations/net_http_spec.rb @@ -0,0 +1,33 @@ +require "appsignal/integrations/net_http" + +describe Appsignal::Integrations::NetHttpIntegration do + let(:transaction) { http_request_transaction } + before(:context) { start_agent } + before { set_current_transaction transaction } + around { |example| keep_transactions { example.run } } + + it "instruments a http request" do + stub_request(:any, "http://www.google.com/") + + Net::HTTP.get_response(URI.parse("http://www.google.com")) + + expect(transaction).to include_event( + "name" => "request.net_http", + "title" => "GET http://www.google.com" + ) + end + + it "instruments a https request" do + stub_request(:any, "https://www.google.com/") + + uri = URI.parse("https://www.google.com") + http = Net::HTTP.new(uri.host, uri.port) + http.use_ssl = true + http.get(uri.request_uri) + + expect(transaction).to include_event( + "name" => "request.net_http", + "title" => "GET https://www.google.com" + ) + end +end diff --git a/spec/lib/appsignal/integrations/object_spec.rb b/spec/lib/appsignal/integrations/object_spec.rb index 6ce35fa5a..a59daee5c 100644 --- a/spec/lib/appsignal/integrations/object_spec.rb +++ b/spec/lib/appsignal/integrations/object_spec.rb @@ -1,6 +1,8 @@ require "appsignal/integrations/object" describe Object do + around { |example| keep_transactions { example.run } } + describe "#instrument_method" do context "with instance method" do let(:klass) do @@ -24,10 +26,8 @@ def call_with_arguments context "when active" do let(:transaction) { http_request_transaction } before do - Appsignal.config = project_fixture_config - expect(Appsignal::Transaction).to receive(:current) - .at_least(:once).and_return(transaction) - expect(Appsignal.active?).to be_truthy + start_agent + set_current_transaction(transaction) end after { Appsignal.config = nil } @@ -80,10 +80,9 @@ def splat(*args, **kwargs) context "with anonymous class" do it "instruments the method and calls it" do - expect(transaction).to receive(:start_event) - expect(transaction).to receive(:finish_event).with \ - "foo.AnonymousClass.other", nil, nil, Appsignal::EventFormatter::DEFAULT expect(call_with_arguments).to eq(["abc", { :foo => "bar" }, 2]) + + expect(transaction).to include_event("name" => "foo.AnonymousClass.other") end end @@ -100,10 +99,9 @@ def foo let(:klass) { NamedClass } it "instruments the method and calls it" do - expect(transaction).to receive(:start_event) - expect(transaction).to receive(:finish_event).with \ - "foo.NamedClass.other", nil, nil, Appsignal::EventFormatter::DEFAULT expect(instance.foo).to eq(1) + + expect(transaction).to include_event("name" => "foo.NamedClass.other") end end @@ -124,11 +122,11 @@ def bar let(:klass) { MyModule::NestedModule::NamedClass } it "instruments the method and calls it" do - expect(transaction).to receive(:start_event) - expect(transaction).to receive(:finish_event).with \ - "bar.NamedClass.NestedModule.MyModule.other", nil, nil, - Appsignal::EventFormatter::DEFAULT expect(instance.bar).to eq(2) + + expect(transaction).to include_event( + "name" => "bar.NamedClass.NestedModule.MyModule.other" + ) end end @@ -143,10 +141,11 @@ def foo end it "instruments with custom name" do - expect(transaction).to receive(:start_event) - expect(transaction).to receive(:finish_event).with \ - "my_method.group", nil, nil, Appsignal::EventFormatter::DEFAULT expect(instance.foo).to eq(1) + + expect(transaction).to include_event( + "name" => "my_method.group" + ) end end @@ -160,7 +159,7 @@ def foo end end - it "should yield the block" do + it "yields the block" do expect(instance.foo { 42 }).to eq(42) end end @@ -171,7 +170,6 @@ def foo it "does not instrument, but still calls the method" do expect(Appsignal.active?).to be_falsy - expect(transaction).to_not receive(:start_event) expect(call_with_arguments).to eq(["abc", { :foo => "bar" }, 2]) end end @@ -198,8 +196,7 @@ def call_with_arguments let(:transaction) { http_request_transaction } before do Appsignal.config = project_fixture_config - expect(Appsignal::Transaction).to receive(:current).at_least(:once) - .and_return(transaction) + set_current_transaction(transaction) end after { Appsignal.config = nil } @@ -253,10 +250,10 @@ def self.splat(*args, **kwargs) context "with anonymous class" do it "instruments the method and calls it" do expect(Appsignal.active?).to be_truthy - expect(transaction).to receive(:start_event) - expect(transaction).to receive(:finish_event).with \ - "bar.class_method.AnonymousClass.other", nil, nil, Appsignal::EventFormatter::DEFAULT expect(call_with_arguments).to eq(["abc", { :foo => "bar" }, 2]) + + transaction._sample + expect(transaction).to include_event("name" => "bar.class_method.AnonymousClass.other") end end @@ -274,10 +271,9 @@ def self.bar it "instruments the method and calls it" do expect(Appsignal.active?).to be_truthy - expect(transaction).to receive(:start_event) - expect(transaction).to receive(:finish_event).with \ - "bar.class_method.NamedClass.other", nil, nil, Appsignal::EventFormatter::DEFAULT expect(klass.bar).to eq(2) + + expect(transaction).to include_event("name" => "bar.class_method.NamedClass.other") end context "with nested named class" do @@ -298,11 +294,10 @@ def self.bar it "instruments the method and calls it" do expect(Appsignal.active?).to be_truthy - expect(transaction).to receive(:start_event) - expect(transaction).to receive(:finish_event).with \ - "bar.class_method.NamedClass.NestedModule.MyModule.other", nil, nil, - Appsignal::EventFormatter::DEFAULT expect(klass.bar).to eq(2) + expect(transaction).to include_event( + "name" => "bar.class_method.NamedClass.NestedModule.MyModule.other" + ) end end end @@ -319,10 +314,9 @@ def self.bar it "instruments with custom name" do expect(Appsignal.active?).to be_truthy - expect(transaction).to receive(:start_event) - expect(transaction).to receive(:finish_event).with \ - "my_method.group", nil, nil, Appsignal::EventFormatter::DEFAULT expect(klass.bar).to eq(2) + + expect(transaction).to include_event("name" => "my_method.group") end end @@ -336,7 +330,7 @@ def self.bar end end - it "should yield the block" do + it "yields the block" do expect(klass.bar { 42 }).to eq(42) end end @@ -347,7 +341,6 @@ def self.bar it "does not instrument, but still call the method" do expect(Appsignal.active?).to be_falsy - expect(transaction).to_not receive(:start_event) expect(call_with_arguments).to eq(["abc", { :foo => "bar" }, 2]) end end diff --git a/spec/lib/appsignal/integrations/padrino_spec.rb b/spec/lib/appsignal/integrations/padrino_spec.rb index 6483f1383..5cf694cf3 100644 --- a/spec/lib/appsignal/integrations/padrino_spec.rb +++ b/spec/lib/appsignal/integrations/padrino_spec.rb @@ -2,11 +2,6 @@ describe "Padrino integration" do require "appsignal/integrations/padrino" - before do - allow(Appsignal).to receive(:active?).and_return(true) - allow(Appsignal).to receive(:start).and_return(true) - end - describe Appsignal::Integrations::PadrinoPlugin do it "starts AppSignal on init" do expect(Appsignal).to receive(:start) @@ -54,20 +49,9 @@ class PadrinoClassWithRouter let(:env) { {} } # TODO: use an instance double let(:settings) { double(:name => "TestApp") } + around { |example| keep_transactions { example.run } } describe "routes" do - let(:transaction) do - instance_double "Appsignal::Transaction", - :set_http_or_background_action => nil, - :set_http_or_background_queue_start => nil, - :set_metadata => nil, - :set_action => nil, - :set_action_if_nil => nil, - :set_error => nil, - :start_event => nil, - :finish_event => nil, - :complete => nil - end let(:request_kind) { kind_of(Sinatra::Request) } let(:env) do { @@ -85,10 +69,6 @@ def self.name end end let(:response) { app.call(env) } - before do - allow(Appsignal::Transaction).to receive(:create).and_return(transaction) - allow(Appsignal::Transaction).to receive(:current).and_return(transaction) - end RSpec::Matchers.define :match_response do |expected_status, expected_content| match do |response| @@ -104,46 +84,41 @@ def self.name end def expect_a_transaction_to_be_created - expect(Appsignal::Transaction).to receive(:create).with( - kind_of(String), - Appsignal::Transaction::HTTP_REQUEST, - request_kind - ).and_return(transaction) - - expect(Appsignal).to receive(:instrument) - .at_least(:once) - .with("process_action.padrino") - .and_call_original - expect(transaction).to receive(:set_metadata).with("path", path) - expect(transaction).to receive(:set_metadata).with("method", "GET") - expect(transaction).to receive(:complete) - end - - def expect_no_transaction_to_be_created - expect(Appsignal::Transaction).to_not receive(:create) - expect(Appsignal).to_not receive(:instrument) + transaction = last_transaction + expect(transaction).to have_id + expect(transaction).to have_namespace(Appsignal::Transaction::HTTP_REQUEST) + expect(transaction).to include_metadata( + "path" => path, + "method" => "GET" + ) + expect(transaction).to include_event("name" => "process_action.padrino") + expect(transaction).to be_completed end context "when AppSignal is not active" do before { allow(Appsignal).to receive(:active?).and_return(false) } let(:path) { "/foo" } before { app.controllers { get(:foo) { "content" } } } - after { expect(response).to match_response(200, "content") } it "does not instrument the request" do - expect_no_transaction_to_be_created + expect do + expect(response).to match_response(200, "content") + end.to_not(change { created_transactions.count }) end end context "when AppSignal is active" do + before { start_agent } + context "with not existing route" do let(:path) { "/404" } it "instruments the request" do + expect(response).to match_response(404, /^GET /404/) + expect_a_transaction_to_be_created # Uses path for action name - expect(transaction).to receive(:set_action_if_nil).with("PadrinoTestApp#unknown") - expect(response).to match_response(404, /^GET /404/) + expect(last_transaction).to have_action("PadrinoTestApp#unknown") end end @@ -153,10 +128,11 @@ def expect_no_transaction_to_be_created env["sinatra.static_file"] = true app.controllers { get(:static) { "Static!" } } end - after { expect(response).to match_response(200, "Static!") } it "does not instrument the request" do - expect_no_transaction_to_be_created + expect do + expect(response).to match_response(200, "Static!") + end.to_not(change { created_transactions.count }) end end @@ -167,12 +143,14 @@ def expect_no_transaction_to_be_created allow_any_instance_of(Sinatra::Request).to receive(:action).and_return(nil) app.controllers { get(:my_original_path, :with => :id) { "content" } } end - after { expect(response).to match_response(200, "content") } it "falls back on Sinatra::Request#route_obj.original_path" do + expect do + expect(response).to match_response(200, "content") + end.to(change { created_transactions.count }.by(1)) + expect_a_transaction_to_be_created - expect(transaction) - .to receive(:set_action_if_nil).with("PadrinoTestApp:/my_original_path/:id") + expect(last_transaction).to have_action("PadrinoTestApp:/my_original_path/:id") end end @@ -183,11 +161,11 @@ def expect_no_transaction_to_be_created allow_any_instance_of(Sinatra::Request).to receive(:route_obj).and_return(nil) app.controllers { get(:my_original_path) { "content" } } end - after { expect(response).to match_response(200, "content") } it "falls back on app name" do + expect(response).to match_response(200, "content") expect_a_transaction_to_be_created - expect(transaction).to receive(:set_action_if_nil).with("PadrinoTestApp#unknown") + expect(last_transaction).to have_action("PadrinoTestApp#unknown") end end @@ -195,25 +173,25 @@ def expect_no_transaction_to_be_created context "with an exception in the controller" do let(:path) { "/exception" } before do - app.controllers { get(:exception) { raise ExampleException } } + app.controllers { get(:exception) { raise ExampleException, "error message" } } + expect { response }.to raise_error(ExampleException, "error message") expect_a_transaction_to_be_created end - after do - expect { response }.to raise_error(ExampleException) - end it "sets the action name based on the app name and action name" do - expect(transaction).to receive(:set_action_if_nil).with("PadrinoTestApp:#exception") + expect(last_transaction).to have_action("PadrinoTestApp:#exception") end it "sets the error on the transaction" do - expect(transaction).to receive(:set_error).with(ExampleException) + expect(last_transaction).to have_error("ExampleException", "error message") end end context "without an exception in the controller" do let(:path) { "/" } - after { expect(response).to match_response(200, "content") } + def make_request + expect(response).to match_response(200, "content") + end context "with action name as symbol" do context "with :index helper" do @@ -223,8 +201,9 @@ def expect_no_transaction_to_be_created end it "sets the action with the app name and action name" do + make_request expect_a_transaction_to_be_created - expect(transaction).to receive(:set_action_if_nil).with("PadrinoTestApp:#index") + expect(last_transaction).to have_action("PadrinoTestApp:#index") end end @@ -235,8 +214,9 @@ def expect_no_transaction_to_be_created end it "sets the action with the app name and action name" do + make_request expect_a_transaction_to_be_created - expect(transaction).to receive(:set_action_if_nil).with("PadrinoTestApp:#foo") + expect(last_transaction).to have_action("PadrinoTestApp:#foo") end end end @@ -249,8 +229,9 @@ def expect_no_transaction_to_be_created end it "sets the action with the app name and action path" do + make_request expect_a_transaction_to_be_created - expect(transaction).to receive(:set_action_if_nil).with("PadrinoTestApp:#/") + expect(last_transaction).to have_action("PadrinoTestApp:#/") end end @@ -261,8 +242,9 @@ def expect_no_transaction_to_be_created end it "sets the action with the app name and action path" do + make_request expect_a_transaction_to_be_created - expect(transaction).to receive(:set_action_if_nil).with("PadrinoTestApp:#/foo") + expect(last_transaction).to have_action("PadrinoTestApp:#/foo") end end end @@ -277,9 +259,9 @@ def expect_no_transaction_to_be_created end it "sets the action with the app name, controller name and action name" do + make_request expect_a_transaction_to_be_created - expect(transaction).to receive(:set_action_if_nil) - .with("PadrinoTestApp:my_controller#index") + expect(last_transaction).to have_action("PadrinoTestApp:my_controller#index") end end @@ -290,9 +272,9 @@ def expect_no_transaction_to_be_created end it "sets the action with the app name, controller name and action path" do + make_request expect_a_transaction_to_be_created - expect(transaction).to receive(:set_action_if_nil) - .with("PadrinoTestApp:/my_controller#index") + expect(last_transaction).to have_action("PadrinoTestApp:/my_controller#index") end end end diff --git a/spec/lib/appsignal/integrations/webmachine_spec.rb b/spec/lib/appsignal/integrations/webmachine_spec.rb index 156bf3887..9805e7bdc 100644 --- a/spec/lib/appsignal/integrations/webmachine_spec.rb +++ b/spec/lib/appsignal/integrations/webmachine_spec.rb @@ -17,70 +17,59 @@ def headers let(:request) do Webmachine::Request.new("GET", "http://google.com:80/foo", {}, nil) end - let(:resource) { double(:trace? => false, :handle_exception => true, :"code=" => nil) } - let(:response) { Response.new } - let(:transaction) { double(:set_action_if_nil => true) } + let(:resource) { double(:trace? => false, :handle_exception => true, :"code=" => nil) } + let(:response) { Response.new } let(:fsm) { Webmachine::Decision::FSM.new(resource, request, response) } before(:context) { start_agent } + around { |example| keep_transactions { example.run } } # Make sure the request responds to the method we need to get query params. describe "request" do - it "should respond to `query`" do + it "responds to #query" do expect(request).to respond_to(:query) end end describe "#run" do - before do - allow(SecureRandom).to receive(:uuid).and_return("uuid") - allow(Appsignal::Transaction).to receive(:create).and_return(transaction) - end + before { allow(fsm).to receive(:call).and_call_original } - it "should create a transaction" do - expect(Appsignal::Transaction).to receive(:create).with( - "uuid", - Appsignal::Transaction::HTTP_REQUEST, - request, - :params_method => :query - ).and_return(transaction) + it "creates a transaction" do + expect { fsm.run }.to(change { created_transactions.count }.by(1)) end - it "should set the action" do - expect(transaction).to receive(:set_action_if_nil).with("RSpec::Mocks::Double#GET") + it "sets the action" do + fsm.run + expect(last_transaction).to have_action("RSpec::Mocks::Double#GET") end - it "should call the original method" do - expect(fsm).to receive(:run) + it "records an instrumentation event" do + fsm.run + expect(last_transaction).to include_event("name" => "process_action.webmachine") end - it "should instrument the original method" do - expect(Appsignal).to receive(:instrument).with("process_action.webmachine") + it "closes the transaction" do + fsm.run + expect(last_transaction).to be_completed + expect(current_transaction?).to be_falsy end - it "should close the transaction" do - expect(Appsignal::Transaction).to receive(:complete_current!) - end - - after { fsm.run } - - describe "concerning the response" do - it "sets a response code" do - expect(fsm.response.code).to be_nil - fsm.run - expect(fsm.response.code).not_to be_nil - end + it "sets a response code" do + expect(fsm.response.code).to be_nil + fsm.run + expect(fsm.response.code).not_to be_nil end end describe "#handle_exceptions" do - let(:error) { ExampleException } + let(:error) { ExampleException.new("error message") } + let(:transaction) { http_request_transaction } - it "should catch the error and send it to AppSignal" do - expect(Appsignal).to receive(:set_error).with(error) - end + it "tracks the error" do + with_current_transaction(transaction) do + fsm.send(:handle_exceptions) { raise error } + end - after do - fsm.send(:handle_exceptions) { raise error } + expect(last_transaction).to have_error("ExampleException", "error message") end end end diff --git a/spec/lib/appsignal/rack/streaming_listener_spec.rb b/spec/lib/appsignal/rack/streaming_listener_spec.rb index 4b6d7fa82..e88fc6b26 100644 --- a/spec/lib/appsignal/rack/streaming_listener_spec.rb +++ b/spec/lib/appsignal/rack/streaming_listener_spec.rb @@ -1,8 +1,6 @@ require "appsignal/rack/streaming_listener" describe Appsignal::Rack::StreamingListener do - before(:context) { start_agent } - let(:headers) { {} } let(:env) do { "rack.input" => StringIO.new, @@ -11,114 +9,96 @@ "QUERY_STRING" => "param=something" } end - let(:app) { double(:call => [200, headers, "body"]) } + let(:app) { DummyApp.new } let(:listener) { Appsignal::Rack::StreamingListener.new(app, {}) } + before(:context) { start_agent } + around { |example| keep_transactions { example.run } } describe "#call" do - context "when Appsignal is active" do - before { allow(Appsignal).to receive(:active?).and_return(true) } - - it "should call `call_with_appsignal_monitoring`" do - expect(listener).to receive(:call_with_appsignal_monitoring) - end - end - context "when Appsignal is not active" do before { allow(Appsignal).to receive(:active?).and_return(false) } - it "should not call `call_with_appsignal_monitoring`" do - expect(listener).to_not receive(:call_with_appsignal_monitoring) + it "does not create a transaction" do + expect do + listener.call(env) + end.to_not(change { created_transactions.count }) end - end - after { listener.call(env) } - end - - describe "#call_with_appsignal_monitoring" do - let!(:transaction) do - Appsignal::Transaction.create( - SecureRandom.uuid, - Appsignal::Transaction::HTTP_REQUEST, - ::Rack::Request.new(env) - ) - end - let(:wrapper) { Appsignal::StreamWrapper.new("body", transaction) } - let(:raw_payload) { { :foo => :bar } } - - before do - allow(SecureRandom).to receive(:uuid).and_return("123") - allow(listener).to receive(:raw_payload).and_return(raw_payload) - allow(Appsignal::Transaction).to receive(:create).and_return(transaction) - end + it "calls the app" do + listener.call(env) - it "should create a transaction" do - expect(Appsignal::Transaction).to receive(:create) - .with("123", Appsignal::Transaction::HTTP_REQUEST, instance_of(Rack::Request)) - .and_return(transaction) - - listener.call_with_appsignal_monitoring(env) + expect(app).to be_called + end end - it "should instrument the call" do - expect(Appsignal).to receive(:instrument) - .with("process_action.rack") - .and_yield + context "when Appsignal is active" do + before { allow(Appsignal).to receive(:active?).and_return(true) } - listener.call_with_appsignal_monitoring(env) - end + let(:wrapper) { Appsignal::StreamWrapper.new("body", transaction) } + let(:raw_payload) { { :foo => :bar } } + before { allow(listener).to receive(:raw_payload).and_return(raw_payload) } - it "should add `appsignal.action` to the transaction" do - allow(Appsignal).to receive(:instrument).and_yield + it "creates a transaction" do + expect do + listener.call(env) + end.to(change { created_transactions.count }.by(1)) + end - env["appsignal.action"] = "Action" + it "instruments the call" do + listener.call(env) - expect(transaction).to receive(:set_action_if_nil).with("Action") + expect(last_transaction).to include_event("name" => "process_action.rack") + end - listener.call_with_appsignal_monitoring(env) - end + it "set `appsignal.action` to the action name" do + env["appsignal.action"] = "Action" - it "should add the path, method and queue start to the transaction" do - allow(Appsignal).to receive(:instrument).and_yield + listener.call(env) - expect(transaction).to receive(:set_metadata).with("path", "/homepage") - expect(transaction).to receive(:set_metadata).with("method", "GET") - expect(transaction).to receive(:set_http_or_background_queue_start) + expect(last_transaction).to have_action("Action") + end - listener.call_with_appsignal_monitoring(env) - end + it "adds the path, method and queue start to the transaction" do + listener.call(env) - context "with an exception in the instrumentation call" do - let(:error) { ExampleException } + expect(last_transaction).to include_metadata( + "path" => "/homepage", + "method" => "GET" + ) + expect(last_transaction).to have_queue_start + end - it "should add the exception to the transaction" do - allow(app).to receive(:call).and_raise(error) + context "with an exception in the instrumentation call" do + let(:error) { ExampleException.new("error message") } + let(:app) { DummyApp.new { raise error } } - expect(transaction).to receive(:set_error).with(error) + it "adds the exception to the transaction" do + expect do + listener.call(env) + end.to raise_error(error) - expect do - listener.call_with_appsignal_monitoring(env) - end.to raise_error(error) + expect(last_transaction).to have_error("ExampleException", "error message") + end end - end - it "should wrap the body in a wrapper" do - expect(Appsignal::StreamWrapper).to receive(:new) - .with("body", transaction) - .and_return(wrapper) + it "wraps the body in a wrapper" do + _, _, body = listener.call(env) - body = listener.call_with_appsignal_monitoring(env)[2] - - expect(body).to be_a(Appsignal::StreamWrapper) + expect(body).to be_a(Appsignal::StreamWrapper) + end end end end describe Appsignal::StreamWrapper do - let(:stream) { double } - let(:transaction) do - Appsignal::Transaction.create(SecureRandom.uuid, Appsignal::Transaction::HTTP_REQUEST, {}) - end + let(:stream) { double } + let(:transaction) { http_request_transaction } let(:wrapper) { Appsignal::StreamWrapper.new(stream, transaction) } + before do + start_agent + set_current_transaction(transaction) + end + around { |example| keep_transactions { example.run } } describe "#each" do it "calls the original stream" do @@ -128,14 +108,14 @@ end context "when #each raises an error" do - let(:error) { ExampleException } + let(:error) { ExampleException.new("error message") } it "records the exception" do allow(stream).to receive(:each).and_raise(error) - expect(transaction).to receive(:set_error).with(error) - expect { wrapper.send(:each) }.to raise_error(error) + + expect(transaction).to have_error("ExampleException", "error message") end end end @@ -143,21 +123,23 @@ describe "#close" do it "closes the original stream and completes the transaction" do expect(stream).to receive(:close) - expect(Appsignal::Transaction).to receive(:complete_current!) wrapper.close + + expect(current_transaction?).to be_falsy + expect(transaction).to be_completed end context "when #close raises an error" do - let(:error) { ExampleException } + let(:error) { ExampleException.new("error message") } it "records the exception and completes the transaction" do allow(stream).to receive(:close).and_raise(error) - expect(transaction).to receive(:set_error).with(error) - expect(transaction).to receive(:complete) - expect { wrapper.send(:close) }.to raise_error(error) + + expect(transaction).to have_error("ExampleException", "error message") + expect(transaction).to be_completed end end end diff --git a/spec/lib/appsignal/transaction_spec.rb b/spec/lib/appsignal/transaction_spec.rb index ed9fcdef8..515482d10 100644 --- a/spec/lib/appsignal/transaction_spec.rb +++ b/spec/lib/appsignal/transaction_spec.rb @@ -83,10 +83,6 @@ def create_transaction(id = transaction_id) end describe ".current" do - def current_transaction - Appsignal::Transaction.current - end - context "when there is a current transaction" do let!(:transaction) do Appsignal::Transaction.create(transaction_id, namespace, request, options) diff --git a/spec/lib/appsignal_spec.rb b/spec/lib/appsignal_spec.rb index ffc93016b..18a136520 100644 --- a/spec/lib/appsignal_spec.rb +++ b/spec/lib/appsignal_spec.rb @@ -232,27 +232,26 @@ end context "not active" do - describe ".monitor_transaction" do - let(:log_stream) { StringIO.new } - let(:log) { log_contents(log_stream) } - before do - Appsignal.config = project_fixture_config("not_active") - Appsignal.start - Appsignal.internal_logger = test_logger(log_stream) - end - after { Appsignal.internal_logger = nil } + before { Appsignal.config = project_fixture_config("not_active") } - it "should do nothing but still yield the block" do - expect(Appsignal::Transaction).to_not receive(:create) - expect(Appsignal).to_not receive(:instrument) - object = double - expect(object).to receive(:some_method).and_return(1) + describe ".monitor_transaction" do + it "does not create a transaction" do + object = double(:some_method => 1) expect do - expect(Appsignal.monitor_transaction("perform_job.nothing") do + Appsignal.monitor_transaction("perform_job.nothing") do object.some_method - end).to eq 1 - end.to_not raise_error + end + end.to_not(change { created_transactions.count }) + end + + it "returns the block's return value" do + object = double(:some_method => 1) + + return_value = Appsignal.monitor_transaction("perform_job.nothing") do + object.some_method + end + expect(return_value).to eq 1 end context "with an unknown event type" do @@ -263,8 +262,11 @@ end it "logs an error" do - Appsignal.monitor_transaction("unknown.sidekiq") {} # rubocop:disable Lint/EmptyBlock - expect(log).to contains_log( + logs = + capture_logs do + Appsignal.monitor_transaction("unknown.sidekiq") {} # rubocop:disable Lint/EmptyBlock + end + expect(logs).to contains_log( :error, "Unrecognized name 'unknown.sidekiq': names must start with either 'perform_job' " \ "(for jobs and tasks) or 'process_action' (for HTTP requests)" @@ -281,6 +283,14 @@ Appsignal.listen_for_error { raise error } end.to raise_error(error) end + + it "does not create a transaction" do + expect do + expect do + Appsignal.listen_for_error { raise error } + end.to raise_error(error) + end.to_not(change { created_transactions.count }) + end end describe ".send_error" do @@ -289,6 +299,12 @@ it "does not raise an error" do Appsignal.send_error(error) end + + it "does not create a transaction" do + expect do + Appsignal.send_error(error) + end.to_not(change { created_transactions.count }) + end end describe ".set_error" do @@ -297,6 +313,12 @@ it "does not raise an error" do Appsignal.set_error(error) end + + it "does not create a transaction" do + expect do + Appsignal.set_error(error) + end.to_not(change { created_transactions.count }) + end end describe ".set_namespace" do @@ -313,66 +335,70 @@ end context "with config and started" do - let(:log_stream) { StringIO.new } - let(:log) { log_contents(log_stream) } - before do - Appsignal.config = project_fixture_config - Appsignal.start - Appsignal.internal_logger = test_logger(log_stream) - end - after { Appsignal.internal_logger = nil } + before { start_agent } + around { |example| keep_transactions { example.run } } describe ".monitor_transaction" do context "with a successful call" do - it "should instrument and complete for a background job" do - expect(Appsignal).to receive(:instrument) - .with("perform_job.something").and_yield - expect(Appsignal::Transaction).to receive(:complete_current!) - object = double - expect(object).to receive(:some_method).and_return(1) - - expect(Appsignal.monitor_transaction( - "perform_job.something", - background_env_with_data - ) do - current = Appsignal::Transaction.current - expect(current.namespace).to eq Appsignal::Transaction::BACKGROUND_JOB - expect(current.request).to be_a(Appsignal::Transaction::GenericRequest) - object.some_method - end).to eq 1 + it "instruments and completes for a background job" do + return_value = nil + expect do + return_value = + Appsignal.monitor_transaction( + "perform_job.something", + { + :class => "BackgroundJob", + :method => "perform" + } + ) do + :return_value + end + end.to(change { created_transactions.count }.by(1)) + expect(return_value).to eq(:return_value) + + transaction = last_transaction + expect(transaction).to have_namespace(Appsignal::Transaction::BACKGROUND_JOB) + expect(transaction).to have_action("BackgroundJob#perform") + expect(transaction).to include_event("name" => "perform_job.something") + expect(transaction).to be_completed end - it "should instrument and complete for a http request" do - expect(Appsignal).to receive(:instrument) - .with("process_action.something").and_yield - expect(Appsignal::Transaction).to receive(:complete_current!) - object = double - expect(object).to receive(:some_method) + it "instruments and completes for a http request" do + return_value = nil + expect do + return_value = + Appsignal.monitor_transaction( + "process_action.something", + { + :controller => "BlogPostsController", + :action => "show" + } + ) do + :return_value + end + end.to(change { created_transactions.count }.by(1)) + expect(return_value).to eq(:return_value) - Appsignal.monitor_transaction( - "process_action.something", - http_request_env_with_data - ) do - current = Appsignal::Transaction.current - expect(current.namespace).to eq Appsignal::Transaction::HTTP_REQUEST - expect(current.request).to be_a(::Rack::Request) - object.some_method - end + transaction = last_transaction + expect(transaction).to have_namespace(Appsignal::Transaction::HTTP_REQUEST) + expect(transaction).to have_action("BlogPostsController#show") + expect(transaction).to include_event("name" => "process_action.something") + expect(transaction).to be_completed end end context "with an erroring call" do - let(:error) { ExampleException.new } - - it "should add the error to the current transaction and complete" do - expect_any_instance_of(Appsignal::Transaction).to receive(:set_error).with(error) - expect(Appsignal::Transaction).to receive(:complete_current!) + let(:error) { ExampleException.new("error message") } + it "adds the error to the current transaction and complete" do expect do Appsignal.monitor_transaction("perform_job.something") do raise error end end.to raise_error(error) + + expect(last_transaction).to have_error("ExampleException", "error message") + expect(last_transaction).to be_completed end end @@ -384,8 +410,11 @@ end it "logs an error" do - Appsignal.monitor_transaction("unknown.sidekiq") {} # rubocop:disable Lint/EmptyBlock - expect(log).to contains_log( + logs = + capture_logs do + Appsignal.monitor_transaction("unknown.sidekiq") {} # rubocop:disable Lint/EmptyBlock + end + expect(logs).to contains_log( :error, "Unrecognized name 'unknown.sidekiq': names must start with either 'perform_job' " \ "(for jobs and tasks) or 'process_action' (for HTTP requests)" diff --git a/spec/support/helpers/transaction_helpers.rb b/spec/support/helpers/transaction_helpers.rb index 38ce54e04..f67689a08 100644 --- a/spec/support/helpers/transaction_helpers.rb +++ b/spec/support/helpers/transaction_helpers.rb @@ -44,6 +44,14 @@ def last_transaction created_transactions.last end + def current_transaction? + Appsignal::Transaction.current? + end + + def current_transaction + Appsignal::Transaction.current + end + # Set current transaction manually. # Cleared by {clear_current_transaction!} # diff --git a/spec/support/mocks/dummy_app.rb b/spec/support/mocks/dummy_app.rb index 2a7cc1931..3905644d1 100644 --- a/spec/support/mocks/dummy_app.rb +++ b/spec/support/mocks/dummy_app.rb @@ -5,7 +5,11 @@ def initialize(&app) end def call(env) - @app&.call(env) + if @app + @app&.call(env) + else + [200, {}, "body"] + end ensure @called = true end