fastruby / next_rails

A toolkit to upgrade your next Rails application
https://www.fastruby.io/blog/upgrade-rails/dual-boot/dual-boot-with-rails-6-0-beta.html?utm_source=github&utm_medium=description&utm_campaign=github&utm_term=next-rails
MIT License
466 stars 28 forks source link

[BUG] `KernelWarnTracker#warn` signature is incorrect #81

Closed mateusdeap closed 1 year ago

mateusdeap commented 1 year ago

Expected Behavior

When I configure the deprecation tracker in a rails_helper file using Ruby 2.7, and when the transform_message option attempts to call methods on the message variable not present in Hash, it shouldn't produce any errors.

Actual Behavior

When I configure the deprecation tracker in a rails_helper file using Ruby 2.7, and when the transform_message option attempts to call methods on the message variable not present in Hash, it produced a NoMethodError

Possible Fix

The issue is because the warn method, as of ruby 2.5, has the uplevel keyword arg. Since our signature only specifies *messages, that ends up becoming a hash with both the messages and the uplevel arg.

Hence, if your transform_message option attempts to manipulate what next_rails provides as a message, it will eventually be given a Hash with the uplevel key.

The most immediate fix is to change the signature of KernelWarnTracker#warn to match the current signature. We'll need to check ruby versions, since this was introduced in ruby 2.5. We'll also probably have to study how to properly deal with the uplevel arg.

To Reproduce

I was able to come up with a spec to reproduce the error. There might be some organizational changes needed concerning the use of contexts, but it catches the bug.

describe "bug when warning uses the uplevel keyword argument" do
  context "given I setup the DeprecationTracker::KernelWarnTracker with a callback that manipulates messages as Strings" do

    DeprecationTracker::KernelWarnTracker.callbacks << -> (message) { message.gsub("Rails.root/", "") }

    context "and given that I call code that emits a warning using the uplevel keyword arg" do
      it "throws a MissingMethod Error" do
        expect{ Kernel.warn("Oh no", uplevel: 1) }.not_to raise_error
      end
    end
  end
end

I will abide by the code of conduct

etagwerker commented 1 year ago

@mateusdeap Thanks for submitting this issue!

I think the first step would be to submit a PR that adds a simple test case for DeprecationTracker::KernelWarnTracker.callbacks

Considering our CI is configured to run with ruby 2.7, we should see a failure in CI.

Then you can probably write an adapter that uses the right signature depending on the ruby version that's being used.

mateusdeap commented 1 year ago

So, @etagwerker I've implemented the spec and the fix in #82. However, I decided to just check the ruby version and change the argument forwarding to super based on that. CI seems to be fine with it. However, I am a fan of learning how to implement new patterns, so if you think using the adapter patters would still be better, just let me know.

Also, I've added the spec under the DeprecationTracker::KernelWarnTracker describe block in a general manner, since I feel this is testing the warn method rather than the callbacks themselves....

Want me to tag you for review?

mateusdeap commented 1 year ago

Also, I think our CI should probably have a job for Ruby 2.5.0, since that's the version that introduced the change. Should I add it in the current PR or in another one?

etagwerker commented 1 year ago

@mateusdeap Please submit a different PR for this:

Also, I think our CI should probably have a job for Ruby 2.5.0

Thanks!

etagwerker commented 1 year ago

@mateusdeap I went ahead and submitted #85 to verify that it all works well with other versions of Ruby.