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

cpruitt · 2019-03-15T15:47:51Z

Note: There may be a better way to approach this or ways to address some of my concerns below. I'd be very happy to get any feedback / suggestions on improving this.

An ActionView::Template's virtual_path is deprecated under Rails 6. This commit:

rails/rails@9a343d1#diff-40f2a5bcf9edfd65a8efe42a05c53736L437

causes the virtual path for templates rendered using file: to no longer have their virtual_path explicitly set to nil. rails-controller-testing uses the absence of the virtual_path attribute to indicate that the template was rendered as using file:. See:

https://github.com/rails/rails-controller-testing/blob/master/lib/rails/controller/testing/template_assertions.rb#L34

Because the attribute is never nil with Rails 6, the ability to test something like assert_template(file: "#{Rails.root}/public/404.html") is broken.

This commit changes the way rendered file: templates are tracked. Instead of paying attention to calls to ActionView::Template#render ( subscribing to"!render_template.action_view") this update instead focuses on ActionView::TemplateRenderer#determine_template. Because determine_tempalte is not already instrumented, we need to provide our own instrumentation by redefining the method, adding instrumentation, and then handing off to super.

Concerns

This probably is not ideal for at least a few reasons:

determine_template is a few steps removed from ActionView::Template#render but is the last place in which we have information about what template is being rendered and whether or not it was rendered as a file:.
This method overrides ActionView::TemplateRenderer#determine_template for all tests, regardless of whether or not file assertions are required. It would be better if the overriding module was prepended only when necessary and removed afterward.
The testing logic applied to file: templates is different than for other template types. I'm not extremely happy about that, but given that ActionView::Template#render is probably a better place to be watching, the change only applies to file templates, not partials, etc...

Testing:

All current tests continue to pass, but this is running against a Rails 5 dummy application so the Rails 6 issue isn't actually tested. The fix worked in our test suite running against a Rails 6 application, but it may be that a Rails 6 branch of rails_controller_testing with an upgraded dummy app is in order? (I'm not sure what's best there.)

An `ActionView::Template`'s `virtual_path` is deprecated under Rails 6. This commit: rails/rails@9a343d1#diff-40f2a5bcf9edfd65a8efe42a05c53736L437 causes the virtual path for templates rendered using `file:` to no longer have their `virtual_path` explicitly set to nil. `rails-controller-testing` uses the absence of the `virtual_path` attribute to indicate that the template was rendered as using `file:`. See: https://github.com/rails/rails-controller-testing/blob/master/lib/rails/controller/testing/template_assertions.rb#L34 Because the attribute is never nil with Rails 6, the ability to test something like `assert_template(file: "#{Rails.root}/public/404.html")` is broken. This commit changes the way rendered `file:` templates are tracked. Instead of paying attention calls to `ActionView::Template#render` ( subscribing to`"!render_template.action_view"`) this update instead focuses on `ActionView::TemplateRenderer#determine_template`. Because `determine_tempalte` is not already instrumented, we need to provide our own instrumentation by redefining the method, adding instrumentation, and then handing off to `super`.

windy · 2019-05-27T11:01:34Z

+1

windy · 2019-05-28T09:05:08Z

We use this branch call these error:

render plain: 'xxx'

causes

virual_path not found error

bbugh · 2019-08-23T20:02:50Z

This patch is working great for us, thank you.

rafaelfranca · 2019-09-19T15:34:31Z

Because the attribute is never nil with Rails 6

It seems it is still being set to nil in the FallbackFileSystemResolver. Am I looking in the wrong place?

rafaelfranca · 2019-09-19T15:39:21Z

What do you mean it is running against a Rails 5 dummy application? We are also running against the master branch. Does it make different which Rails version the application was generated? It should not.

rafaelfranca · 2019-09-19T16:52:39Z

OK. I know the problem now. I don't think virtual_path does anything here. The problem is that render file: doesn't trigger the !render_template notification anymore. I guess this fix is good enough and there is no problem on adding the method override to all tests since you are just triggering a notification. It would only have overhead if the subscriber is setup.

rafaelfranca · 2019-09-19T18:25:40Z

lib/rails/controller/testing.rb

@@ -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)


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.

cpruitt · 2019-09-20T14:34:11Z

@rafaelfranca Thanks for the comments/questions.

It seems it is still being set to nil in the FallbackFileSystemResolver. Am I looking in the wrong place?

It's been about six months since I've looked at this so I'm a little fuzzy but I believe rails/rails#35662 re-introduced a nil value for virtual_path that had previously been removed. I think what happened was that since this PR was opened, the original issue (virtual_path = nil) was fixed in Rails but a different issue (!render_template notification no longer being called) was introduced that just happens to be fixed by this PR. 😬 I may need to look into that a little deeper.

What do you mean it is running against a Rails 5 dummy application?

That's my confusion. I didn't realize at the time how testing was set up. When I wrote that, bundle/install resolved the Rails dependencies for ">= 5.0.1.rc1" to 5.2 and I erroneously assumed from some earlier commit comments that the app in test/dummy was somehow pinned to the 5.2 version. I also didn't realize the way CI was set up with multiple gemfiles. You can disregard that comment.

I don't think virtual_path does anything here. The problem is that render file: doesn't trigger the !render_template notification anymore.

I think this might be new behavior since I opened the PR. If this PR resolves that issue then it's probably a happy coincidence. It's not super fresh in my mind so I'll probably need to go back and take a look before I can comment for sure. I can go head and make the callback change you requested just to get that done and out of the way.

tconst · 2019-10-25T07:13:37Z

Alternative but similar solution based on the work in this PR that works in current Rails v6.0.0.
tconst@7c8f078

bbugh · 2019-11-21T21:23:42Z

Is there anything we can do to help get this or @tconst's patch merged?

bbugh · 2020-01-09T19:57:38Z

@cpruitt We recently changed a render to a send_data with type text, and this throws an error coming from this PR. Without rails-controller-testing this test passes fine (but all the legacy ones break of course).

Failure/Error: send_data result, type: 'text/csv', filename: filename

NoMethodError:
  undefined method `virtual_path' for text template:ActionView::Template::Text
# /Users/bbugh/.rvm/gems/ruby-2.6.5/bundler/gems/rails-controller-testing-f047f57530f1/lib/rails/controller/testing/template_instrumentation.rb:8:in `block in determine_template'
# /Users/bbugh/.rvm/gems/ruby-2.6.5/bundler/gems/rails-controller-testing-f047f57530f1/lib/rails/controller/testing/template_instrumentation.rb:6:in `tap'
# /Users/bbugh/.rvm/gems/ruby-2.6.5/bundler/gems/rails-controller-testing-f047f57530f1/lib/rails/controller/testing/template_instrumentation.rb:6:in `determine_template'
# ./app/controllers/files_controller.rb:27:in `render_csv'
# ./app/controllers/files_controller.rb:16:in `block (2 levels) in show'
# ./app/controllers/files_controller.rb:15:in `show'
# /Users/bbugh/.rvm/gems/ruby-2.6.5/bundler/gems/rails-controller-testing-f047f57530f1/lib/rails/controller/testing/integration.rb:13:in `block (2 levels) in <module:Integration>'
# ./spec/requests/files_controller_spec.rb:23:in `block (4 levels) in <top (required)>'

bbugh · 2020-05-06T15:54:12Z

lib/rails/controller/testing/template_instrumentation.rb

+        def determine_template(options)
+          super.tap do |found|
+            if found
+              instrument_payload = { template_identifier: found.identifier, virtual_path: found.virtual_path, options: options }


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)

@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). 🙂

cpruitt · 2020-05-07T19:52:02Z

I'd defer to tconst@7c8f078 for a fix. I'm currently out of date on my knowledge of this issue and don't think I have context or ability to reproduce and test this in a timely enough way to push this forward.

GeniusTimo · 2021-11-05T10:15:34Z

Full support for Rails 6 would be great, is there anything left that needs to be done?
tconst@7c8f078 would look like a good solution to me (Maybe it needs a new PR then?)

CC @rafaelfranca

tconst · 2022-01-24T05:41:26Z

I've been using tconst@7c8f078 now for some time and recently upgraded from rails 6.1 to 7.0.1 with no issues. I can open a PR if that would help move this along for others.

GeniusTimo · 2022-01-24T20:28:42Z

I would appreciate it 👍

* Brings back `!render_template.action_view` instrumentation when rendering with ActionView::Template::RawFile See rails#46 for discussion.

tconst · 2022-03-24T06:01:08Z

#76 contains my commit, thanks @airdrummingfool. I have been dealing with a newborn apologies for not getting back to this.

cpruitt · 2022-08-30T14:33:27Z

I'm closing this as there are better alternative PRs out there and it's, like... old. 🙂

cpruitt marked this pull request as ready for review March 15, 2019 17:02

jhawthorn mentioned this pull request Mar 18, 2019

Remove virtual_path from fallback templates rails/rails#35662

Merged

karljohansson85 mentioned this pull request Aug 27, 2019

Avoid deprecated dynamic controller/action segment #49

Closed

rafaelfranca requested changes Sep 19, 2019

View reviewed changes

bbugh reviewed May 6, 2020

View reviewed changes

airdrummingfool mentioned this pull request Mar 3, 2022

:identifiers in Rails 6 seem to fall into the wrong subscriber. #73

Open

airdrummingfool mentioned this pull request Mar 24, 2022

Fix assert_template(file:) with Rails 6 #76

Open

cpruitt closed this Aug 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cpruitt commented Mar 15, 2019 •

edited

Loading

windy commented May 27, 2019

windy commented May 28, 2019

bbugh commented Aug 23, 2019

rafaelfranca commented Sep 19, 2019

rafaelfranca commented Sep 19, 2019

rafaelfranca commented Sep 19, 2019

rafaelfranca Sep 19, 2019

cpruitt commented Sep 20, 2019

tconst commented Oct 25, 2019

bbugh commented Nov 21, 2019

bbugh commented Jan 9, 2020

bbugh May 6, 2020

cpruitt May 7, 2020

cpruitt commented May 7, 2020

GeniusTimo commented Nov 5, 2021

tconst commented Jan 24, 2022

GeniusTimo commented Jan 24, 2022 •

edited

Loading

tconst commented Mar 24, 2022

cpruitt commented Aug 30, 2022

	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 }

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

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

Conversation

cpruitt commented Mar 15, 2019 • edited Loading

windy commented May 27, 2019

windy commented May 28, 2019

bbugh commented Aug 23, 2019

rafaelfranca commented Sep 19, 2019

rafaelfranca commented Sep 19, 2019

rafaelfranca commented Sep 19, 2019

rafaelfranca Sep 19, 2019

Choose a reason for hiding this comment

cpruitt commented Sep 20, 2019

tconst commented Oct 25, 2019

bbugh commented Nov 21, 2019

bbugh commented Jan 9, 2020

bbugh May 6, 2020

Choose a reason for hiding this comment

cpruitt May 7, 2020

Choose a reason for hiding this comment

cpruitt commented May 7, 2020

GeniusTimo commented Nov 5, 2021

tconst commented Jan 24, 2022

GeniusTimo commented Jan 24, 2022 • edited Loading

tconst commented Mar 24, 2022

cpruitt commented Aug 30, 2022

cpruitt commented Mar 15, 2019 •

edited

Loading

GeniusTimo commented Jan 24, 2022 •

edited

Loading