Skip to content

Allow assert_template(file:) to work using Rails 6 #46

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.

Sign up for GitHub

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

Closed
cpruitt wants to merge 1 commit into from
Closed

Allow assert_template(file:) to work using Rails 6 #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/rails/controller/testing.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
require 'rails/controller/testing/test_process'
require 'rails/controller/testing/integration'
require 'rails/controller/testing/template_assertions'
require 'rails/controller/testing/template_instrumentation'

module Rails
module Controller
Expand All @@ -10,16 +11,19 @@ def self.install
ActiveSupport.on_load(:action_controller_test_case) do
include Rails::Controller::Testing::TestProcess
include Rails::Controller::Testing::TemplateAssertions
ActionView::TemplateRenderer.prepend(InstrumentDetermineTemplate)
Copy link
Member

@rafaelfranca rafaelfranca Sep 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should include this inside an on_load(:action_view) otherwise we could prepend this same module three times to the same class when running tests.

end

ActiveSupport.on_load(:action_dispatch_integration_test) do
include Rails::Controller::Testing::TemplateAssertions
include Rails::Controller::Testing::Integration
include Rails::Controller::Testing::TestProcess
ActionView::TemplateRenderer.prepend(InstrumentDetermineTemplate)
end

ActiveSupport.on_load(:action_view_test_case) do
include Rails::Controller::Testing::TemplateAssertions
ActionView::TemplateRenderer.prepend(InstrumentDetermineTemplate)
end
end
end
Expand Down
16 changes: 10 additions & 6 deletions lib/rails/controller/testing/template_assertions.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,16 @@ def setup_subscriptions
end
end

@_subscribers << ActiveSupport::Notifications.subscribe("determine_template.rails_controller_testing") do |_name, _start, _finish, _id, payload|
if payload[:options][:file]
path = payload[:template_identifier]
if path
@_files[path] += 1
@_files[path.split("/").last] += 1
end
end
end

@_subscribers << ActiveSupport::Notifications.subscribe("!render_template.action_view") do |_name, _start, _finish, _id, payload|
if virtual_path = payload[:virtual_path]
partial = virtual_path =~ /^.*\/_[^\/]*$/
Expand All @@ -40,12 +50,6 @@ def setup_subscriptions
end

@_templates[virtual_path] += 1
else
path = payload[:identifier]
if path
@_files[path] += 1
@_files[path.split("/").last] += 1
end
end
end
end
Expand Down
16 changes: 16 additions & 0 deletions lib/rails/controller/testing/template_instrumentation.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
module Rails
module Controller
module Testing
module InstrumentDetermineTemplate
def determine_template(options)
super.tap do |found|
if found
instrument_payload = { template_identifier: found.identifier, virtual_path: found.virtual_path, options: options }
Copy link

@bbugh bbugh May 6, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
instrument_payload = { template_identifier: found.identifier, virtual_path: found.virtual_path, options: options }
instrument_payload = { template_identifier: found.identifier, virtual_path: found.try(:virtual_path), options: options }

@cpruitt virtual_path isn't always available, like if you're rendering a string rather than a template. This should try to call virtual_path so it doesn't fail if the method is missing.

I reported this error here: #46 (comment)

Copy link
Author

@cpruitt cpruitt May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bbugh Thanks for the suggested change. Our test implementation changed such that I'm not in a great position to reproduce this problem or test this fix. I believe the original issue that this PR was intended to resolve changed and this PR just happened to still fix a similar problem. I'm not currently able to allocate time to get up to speed to verify that and test this out. If you have available time, I'd wholeheartedly endorse you taking this and running with it in a new PR. I'm happy to help answer any questions I can (thought I don't know how much help that would be). 🙂

ActiveSupport::Notifications.instrument("determine_template.rails_controller_testing", instrument_payload)
end
end
end
end
end
end
end