Skip to content

Commit

Permalink
Add Rack::Lint to ActionDispatch::ServerTiming tests
Browse files Browse the repository at this point in the history
To ensure Rails is and remains compliant with [the Rack 3
spec](https://github.com/rack/rack/blob/6d16306192349e665e4ec820a9bfcc0967009b6a/UPGRADE-GUIDE.md)
we can add `Rack::Lint` to the Rails middleware tests.

This adds additional test coverage to `ActionDispatch::ServerTiming` to
validate that its input and output follow the Rack SPEC.

The `Server-Timing` header definition was moved to
`ActionDispatch::Constants` and is now downcased to match the Rack 3
SPEC.

The tests that rely on a `Concurrent::CyclicBarrier` ("events are
tracked by thread") were changed since passing the required proc in the
env is not compatible with the SPEC:

```
Rack::Lint::LintError: env variable proc has non-string value
```

The same can be achieved by invoking the proc as a child Rack app.
  • Loading branch information
nunosilva800 committed Jul 28, 2023
1 parent dda937f commit 04c6116
Show file tree
Hide file tree
Showing 3 changed files with 30 additions and 21 deletions.
2 changes: 2 additions & 0 deletions actionpack/lib/action_dispatch/constants.rb
Expand Up @@ -11,12 +11,14 @@ module Constants
LOCATION = "Location"
FEATURE_POLICY = "Feature-Policy"
X_REQUEST_ID = "X-Request-Id"
SERVER_TIMING = "Server-Timing"
else
VARY = "vary"
CONTENT_ENCODING = "content-encoding"
LOCATION = "location"
FEATURE_POLICY = "feature-policy"
X_REQUEST_ID = "x-request-id"
SERVER_TIMING = "server-timing"
end
end
end
8 changes: 4 additions & 4 deletions actionpack/lib/action_dispatch/middleware/server_timing.rb
Expand Up @@ -4,8 +4,6 @@

module ActionDispatch
class ServerTiming
SERVER_TIMING_HEADER = "Server-Timing"

class Subscriber # :nodoc:
include Singleton
KEY = :action_dispatch_server_timing_events
Expand Down Expand Up @@ -67,8 +65,10 @@ def call(env)
"%s;dur=%.2f" % [event_name, events_collection.sum(&:duration)]
end

header_info.prepend(headers[SERVER_TIMING_HEADER]) if headers[SERVER_TIMING_HEADER].present?
headers[SERVER_TIMING_HEADER] = header_info.join(", ")
if headers[ActionDispatch::Constants::SERVER_TIMING].present?
header_info.prepend(headers[ActionDispatch::Constants::SERVER_TIMING])
end
headers[ActionDispatch::Constants::SERVER_TIMING] = header_info.join(", ")

response
end
Expand Down
41 changes: 24 additions & 17 deletions actionpack/test/dispatch/server_timing_test.rb
Expand Up @@ -23,7 +23,8 @@ def create
end

setup do
@middlewares = [ActionDispatch::ServerTiming]
@middlewares = [Rack::Lint, ActionDispatch::ServerTiming, Rack::Lint]
@header_name = ActionDispatch::Constants::SERVER_TIMING
end

teardown do
Expand All @@ -36,49 +37,54 @@ def create
test "server timing header is included in the response" do
with_test_route_set do
get "/"
assert_match(/\w+/, @response.headers["Server-Timing"])
assert_match(/\w+/, @response.headers[@header_name])
end
end

test "includes default action controller events duration" do
with_test_route_set do
get "/"
assert_match(/start_processing.action_controller;dur=\w+/, @response.headers["Server-Timing"])
assert_match(/process_action.action_controller;dur=\w+/, @response.headers["Server-Timing"])
assert_match(/start_processing.action_controller;dur=\w+/, @response.headers[@header_name])
assert_match(/process_action.action_controller;dur=\w+/, @response.headers[@header_name])
end
end

test "includes custom active support events duration" do
with_test_route_set do
get "/id"
assert_match(/custom.event;dur=\w+/, @response.headers["Server-Timing"])
assert_match(/custom.event;dur=\w+/, @response.headers[@header_name])
end
end

test "events are tracked by thread" do
barrier = Concurrent::CyclicBarrier.new(2)

stub_app = -> (env) {
env["proc"].call
env["action_dispatch.test"].call
[200, {}, "ok"]
}
app = ActionDispatch::ServerTiming.new(stub_app)
app = Rack::Lint.new(
ActionDispatch::ServerTiming.new(Rack::Lint.new(stub_app))
)

t1 = Thread.new do
app.call({ "proc" => -> {
proc = -> {
barrier.wait
barrier.wait
} })
}
env = Rack::MockRequest.env_for("", { "action_dispatch.test" => proc })
app.call(env)
end

t2 = Thread.new do
barrier.wait

response = app.call({ "proc" => -> {
proc = -> {
ActiveSupport::Notifications.instrument("custom.event") do
true
end
} })
}
env = Rack::MockRequest.env_for("", { "action_dispatch.test" => proc })
response = app.call(env)

barrier.wait

Expand All @@ -88,8 +94,8 @@ def create
headers1 = t1.value[1]
headers2 = t2.value[1]

assert_match(/custom.event;dur=\w+/, headers2["Server-Timing"])
assert_no_match(/custom.event;dur=\w+/, headers1["Server-Timing"])
assert_match(/custom.event;dur=\w+/, headers2[@header_name])
assert_no_match(/custom.event;dur=\w+/, headers1[@header_name])
end

test "does not overwrite existing header values" do
Expand All @@ -100,15 +106,16 @@ def initialize(app)

def call(env)
status, headers, body = @app.call(env)
headers["Server-Timing"] = [headers["Server-Timing"], %(entry;desc="description")].compact.join(", ")
header_name = ActionDispatch::Constants::SERVER_TIMING
headers[header_name] = [headers[header_name], %(entry;desc="description")].compact.join(", ")
[ status, headers, body ]
end
end

with_test_route_set do
get "/"
assert_match(/entry;desc="description"/, @response.headers["Server-Timing"])
assert_match(/start_processing.action_controller;dur=\w+/, @response.headers["Server-Timing"])
assert_match(/entry;desc="description"/, @response.headers[@header_name])
assert_match(/start_processing.action_controller;dur=\w+/, @response.headers[@header_name])
end
end

Expand Down

0 comments on commit 04c6116

Please sign in to comment.