Skip to content

Commit

Permalink
fix instrumentation to support puma >= 6.2 (#600) (#601)
Browse files Browse the repository at this point in the history
Tests are failing on Puma 6.2 (and newer) when instrumentation is
enabled. This seems to be caused by PR #3072 in Puma which changes how
it handles responses. If the body (3rd argument) is a file-like object
(which Asset is) then it appears to be getting the file path and reading
the original contents which then doesn't match the modified
Content-Length header for the instrumented version.

This commit fixes the issue by returning the instrumented source as a
string rather then the Asset object.
  • Loading branch information
ukdave committed Sep 5, 2023
1 parent 8f062d3 commit 4d7cfbf
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 8 deletions.
6 changes: 2 additions & 4 deletions lib/teaspoon/instrumentation.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,8 @@ def instrumented_response

result = add_instrumentation(asset)

asset.instance_variable_set(:@source, result)
asset.instance_variable_set(:@length, headers["Content-Length"] = result.bytesize.to_s)

[status, headers, asset]
headers["Content-Length"] = result.bytesize.to_s
[status, headers, [result]]
end

protected
Expand Down
9 changes: 5 additions & 4 deletions spec/teaspoon/instrumentation_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

let(:asset) { double(source: source, filename: "path/to/instrument.js") }
let(:source) { "function add(a, b) { return a + b } // ☃ " }
let(:instrumented_source) { source + " // instrumented" }
let(:response) { [200, { "Content-Type" => "application/javascript" }, asset] }
let(:env) { { "QUERY_STRING" => "instrument=true" } }

Expand All @@ -21,7 +22,7 @@
expect(asset).to receive(:clone).and_return(asset)

allow(File).to receive(:open)
allow_any_instance_of(subject).to receive(:instrument).and_return(source + " // instrumented")
allow_any_instance_of(subject).to receive(:instrument).and_return(instrumented_source)

path = nil
Dir.mktmpdir { |p| path = p }
Expand All @@ -43,7 +44,7 @@

it "replaces the response array with the appropriate information" do
response = [666, { "Content-Type" => "application/javascript" }, asset]
expected = [666, { "Content-Type" => "application/javascript", "Content-Length" => "59" }, asset]
expected = [666, { "Content-Type" => "application/javascript", "Content-Length" => "59" }, [instrumented_source]]

expect(subject.add_to(response, env)).to eq(expected)
end
Expand Down Expand Up @@ -124,10 +125,10 @@ def ignore(paths)

it "instruments a file" do
pending("needs istanbul to be installed") unless Teaspoon::Instrumentation.executable
status, headers, asset = subject.add_to(response, "QUERY_STRING" => "instrument=true")
status, headers, body = subject.add_to(response, "QUERY_STRING" => "instrument=true")
expect(status).to eq(200)
expect(headers).to include("Content-Type" => "application/javascript")
expect(asset.source).to match(/var __cov_.+ = \(Function\('return this'\)\)\(\);/)
expect(body.first).to match(/var __cov_.+ = \(Function\('return this'\)\)\(\);/)
end

it "hooks into sprockets" do
Expand Down

0 comments on commit 4d7cfbf

Please sign in to comment.