Skip to content

Commit

Permalink
Revamp the Rails integration's tests
Browse files Browse the repository at this point in the history
* Remove extraneous spans that were being generated due to the
  ActiveSupport::Notifications subscriptions. There's no need to
  actually configure these from the tests, since we already have the
  active_support_spec.rb, and it's not like the config in the
  rails_spec.rb even matched the one from the Rails generator or
  anything. These just get in the way of testing the Rails integration,
  since the custom fields we'd want to make assertions about only wind
  up on the root span anyway.

* Factor out assertions into a set of shared examples, and actually
  start making assertions about the `request.{action,controller,route}`
  fields that the integration adds (which weren't even being tested for
  previously 😱).

* Rework the "bad request" tests that reproduce honeycombio#49. These tests will
  now break against the old implementation, since they assert that
  routing information is still present (whereas honeycombio#50 wipes the fields out
  with error messages). There are ways to get the routing information
  without ever trying to parse the GET/POST parameters, so we can still
  save this use case. Furthermore, the only Rails 5+ specific changes
  are to whether or not Rails responds with an HTTP 400, so we needn't
  wipe out the entire `describe` block for Rails < 5.

* Add tests to reproduce the issue described in honeycombio#62, where unrecognized
  routes may cause the old implementation to erroneously fall back to
  the GET/POST parameters for routing information.

* Add tests that reproduce honeycombio#31. The bug is actually in the twirp gem,
  whose middleware fails to rewind the `rack.input` after reading it.
  I've verified these regression tests against the old version of the
  Rails integration that didn't check `if request.raw_post`, and they
  fail appropriately. I wanted to have this test in here to have
  confidence in changing the implementation of the Rails integration.
  The `raw_post` workaround of honeycombio#39 is unnecessary if we actually never
  look at the GET/POST parameters in the first place (no parsing, no
  problem).
  • Loading branch information
ajvondrak committed Feb 22, 2020
1 parent 865358e commit 69a02e7
Showing 1 changed file with 98 additions and 26 deletions.
124 changes: 98 additions & 26 deletions spec/honeycomb/integrations/rails_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,10 +14,6 @@
let(:configuration) do
Honeycomb::Configuration.new.tap do |config|
config.client = libhoney_client
config.notification_events = %w[
render_template.action_view
process_action.action_controller
].freeze
end
end
let(:client) { Honeycomb::Client.new(configuration: configuration) }
Expand All @@ -43,51 +39,127 @@

class TestController < ActionController::Base
def hello
render plain: "Hello World!"
render plain: "Hello #{params[:name]}!"
end
end

shared_examples_for "the rails integration" do
let(:event_data) { libhoney_client.events.map(&:data) }

it_behaves_like "event data"

it "sends the right number of events" do
expect(libhoney_client.events.size).to eq 1
end

let(:event) { event_data.first }

it "sends the right request.controller" do
expect(event["request.controller"]).to eq controller
end

it "sends the right request.action" do
expect(event["request.action"]).to eq action
end

it "sends the right request.route" do
expect(event["request.route"]).to eq route
end
end

describe "a standard request" do
before do
header("Http-Version", "HTTP/1.0")
header("User-Agent", "RackSpec")

get "/hello/martin"
get "/hello/world"
end

it "returns ok" do
expect(last_response).to be_ok
end

it "sends the right number of events" do
expect(libhoney_client.events.size).to eq 3
include_examples "the rails integration" do
let(:controller) { "test" }
let(:action) { "hello" }
let(:route) { "GET /hello/:name(.:format)" }
end
end

let(:event_data) { libhoney_client.events.map(&:data) }
describe "a request with invalid parameter encoding" do
before do
get "/hello/world?via=%c1"
end

it_behaves_like "event data"
if VERSION >= Gem::Version.new("5")
it "returns bad request" do
expect(last_response).to be_bad_request
end
else
it "returns ok" do
expect(last_response).to be_ok
end
end

include_examples "the rails integration" do
let(:controller) { "test" }
let(:action) { "hello" }
let(:route) { "GET /hello/:name(.:format)" }
end
end

if VERSION >= Gem::Version.new("5")
describe "a bad request" do
before do
header("Http-Version", "HTTP/1.0")
header("User-Agent", "RackSpec")
describe "an unrecognized request" do
before do
get "/unrecognized", action: "action", controller: "controller"
end

get "/hello/martin?via=%c1"
end
it "returns not found" do
expect(last_response).to be_not_found
end

it "returns bad request" do
expect(last_response).to be_bad_request
include_examples "the rails integration" do
let(:controller) { nil }
let(:action) { nil }
let(:route) { nil }
end
end

describe "twirp bug" do
before do
app.routes.draw do
mount TwirpBug, at: "/twirp", via: :post
end
end

it "sends the right number of events" do
expect(libhoney_client.events.size).to eq 1
# Emulates a bug in the twirp gem.
#
# The twirp middleware calls IO#read on the rack.input without rewinding.
# Later, when Rails tries to parse the request parameters by calling
# IO#read(n) with the content length, we get back nil, which blows up the
# default JSON MIME type handler.
#
# @see https://github.com/honeycombio/beeline-ruby/issues/31
# @see https://github.com/honeycombio/beeline-ruby/pull/39
# @see https://github.com/twitchtv/twirp-ruby/blob/c8520030b3e4eb584042b0a8db9ae606a3b6c6f4/lib/twirp/service.rb#L138
module TwirpBug
def self.call(env)
env["rack.input"].read # don't rewind
::Rack::Response.new("OK").finish
end
end

let(:event_data) { libhoney_client.events.map(&:data) }
before do
env "CONTENT_TYPE", "application/json"
env "CONTENT_LENGTH", 17
env "rack.input", StringIO.new('{"json":"object"}')
post "/twirp"
end

it "returns ok" do
expect(last_response).to be_ok
end

it_behaves_like "event data"
include_examples "the rails integration" do
let(:controller) { nil }
let(:action) { nil }
let(:route) { "POST /twirp" }
end
end
end
Expand Down

0 comments on commit 69a02e7

Please sign in to comment.