Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code coverage not working with Puma 6.2 #600

Closed
ukdave opened this issue Aug 24, 2023 · 5 comments
Closed

Code coverage not working with Puma 6.2 #600

ukdave opened this issue Aug 24, 2023 · 5 comments

Comments

@ukdave
Copy link
Contributor

ukdave commented Aug 24, 2023

I just upgraded Puma from from 6.1 to 6.2 and suddenly all my teaspoon tests failed. After quite a bit of digging I found out it was to do with the code coverage - specifically how the assets are instrumented.

I added this monkey patch to my teaspoon_env.rb file which seems to fix it:

module Teaspoon
  class Instrumentation
    def instrumented_response
      status, headers, asset = @response
      headers, asset = [headers.clone, asset.clone]

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

I'm thinking the issue is due to PR #3072 in Puma and the original Teaspoon::Instrumentation#instrumented_response method returning an Asset object which has file-like methods (e.g. #filename and #bytesize). I'm wondering if Puma 6.2 is trying to read the original asset file, but is getting thrown off because we've updated the content-length. Either way changing the instrumented_response method to return the modified file contents as a string seems to fix it.

@mathieujobin
Copy link
Collaborator

nice ! I would have never thought.... this is really helpful.

would you mind opening a PR with the fix ? is it backward compatible with puma 6.1 and below or we need a conditional ?

@mathieujobin
Copy link
Collaborator

cc: @phenchaw

@ukdave
Copy link
Contributor Author

ukdave commented Sep 1, 2023

I've tested and confirmed that the fix works with Puma 6.3.1, 6.2.2, 6.1.1, 6.0.1, 5.6.7, and 5.0.4. I've opened PR #601

@mathieujobin
Copy link
Collaborator

truly awesome, thanks

mathieujobin pushed a commit that referenced this issue Sep 5, 2023
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.
@mathieujobin
Copy link
Collaborator

released in v1.4.0
https://rubygems.org/gems/teaspoon/versions/1.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants