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

Fix tests on Ruby 3 #87

Open
dylanahsmith opened this issue Oct 20, 2021 · 5 comments
Open

Fix tests on Ruby 3 #87

dylanahsmith opened this issue Oct 20, 2021 · 5 comments

Comments

@dylanahsmith
Copy link
Contributor

See CI test failures (https://github.com/Shopify/rotoscope/runs/3954967334?check_suite_focus=true) from draft PR #86 changing CI to test on ruby 3

@dylanahsmith
Copy link
Contributor Author

It looks like the primary difference is that ruby 3 is actually exposing the C call frames in rb_profile_frames now. E.g. we are now seeing that calls to initialize are coming from new instead of the code that calls new. That is why you see differences like this in the test failures

-  :caller_method_name=>"test_start_trace_and_stop_trace",
+  :caller_method_name=>"new",

These C frames are missing a file path. Since the normalization in the tests tries to expand the path as part of turning it into a relative path, it ends up expanding the empty path to a path to the current directory, resulting in differences like

-  :filepath=>"/rotoscope_test.rb",
+  :filepath=>"/home/runner/work/rotoscope/rotoscope",

@dylanahsmith
Copy link
Contributor Author

I also noticed that the calls to initialize seem to be coming from a "new" with class_method_level "instance", which seemed strange. The full_label on that profile frame shows the call is actually from Class#new, which makes more sense, but it doesn't make sense in the context of the receiver class being a concrete class. For example, we would expect a call to Example#initialize to come from the class method Example.new, not the non-existent instance method Example#new.

@jahfer
Copy link
Contributor

jahfer commented Oct 25, 2021

We handle C frames different than Ruby ones (link). If Ruby 3 is now including the C frame in #rb_profile_frames, our logic makes sense to be broken, and our frame_index is probably looking one level too high in the stack. That might explain Class#new instead of Example.new, though I haven't looked closely at the data beyond what you've mentioned here.

@dylanahsmith
Copy link
Contributor Author

There is still a difference in that the C call has the caller at the top of the stack when the event hook is called. Perhaps that difference can be removed upstream, but it probably got introduced because it didn't matter in the context of rb_profile_frames primary purpose of supporting sample based profiling.

Class#new is the defined method that gets called when it doesn't get overridden, so it is right from that perspective. It just is just wrong from the perspective of knowing that method is called on the receiver class (e.g. Example)

@dylanahsmith
Copy link
Contributor Author

Class#new is the defined method that gets called when it doesn't get overridden, so it is right from that perspective. It just is just wrong from the perspective of knowing that method is called on the receiver class (e.g. Example)

This is also related to the fact that we were pulling together information from rb_profile_frames and rb_tracearg_* from the parent call for the caller, since the former didn't expose the receiver or class.

For example, when I run the script

require 'rotoscope'

class Example
end

def format_call(class_name, is_singleton_method, method_name)
  class_name ||= "<UNKNOWN>"
  method_name ||= "<UNKNOWN>"
  singleton_indicator = case is_singleton_method
  when nil then "?"
  when false then "#"
  when true then "."
  else
    raise
  end
  "#{class_name}#{singleton_indicator}#{method_name}"
end

rs = Rotoscope.new do |call|
  caller_method = format_call(call.caller_class_name, call.caller_singleton_method?, call.caller_method_name)
  receiver_method = format_call(call.receiver_class_name, call.singleton_method?, call.method_name)
  puts "#{caller_method} called #{receiver_method}"
end
rs.trace do
  Example.new
end

it outputs

<UNKNOWN>#<UNKNOWN> called Example.new
Example#new called Example#initialize

where you can see the inconsistency between it logging the Example.new class method being called, but logging it as an instance method when it is the caller.

jahfer added a commit that referenced this issue Oct 28, 2021
Until #87 is fixed, this gem produces unreliable results under Ruby 3.
@jahfer jahfer mentioned this issue Oct 28, 2021
1 task
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