improved `assert_template` when matching :locals #8520

Merged
merged 2 commits into from Feb 4, 2013

Projects

None yet

2 participants

@senny
Owner
senny commented Dec 15, 2012

This PR improves two aspects of assert_template when used with the :locals option.

1. descriptive error message when the partial wasn't rendered

When assert_template is used with the :locals option, and the partial was not rendered, a method_missing error was raised. This changes first checks, if the partial actually was rendered and raises a descriptive error.

2. recognize partial inside a directory

previously when a partial was placed inside a directory (eg. '/dir/partial'), assert_template did not replace the '' prefix when looking through rendered tempaltes, which resulted in an error.

I modified it to replace both, the leading '' and the last '' after a '/'.

This is a fix for #8516

Owner
senny commented Dec 15, 2012

I created a separate commit for each change. Let me know I they need to be squashed. Also I think we should backport this because it makes assert_template with :locals much more usable.

I've noticed that some code for assert_template is getting deeply nested. Would you mind if I refactor https://github.com/senny/rails/blob/b0364a20fcd89a495542505823840f7d0eb6aa27/actionpack/lib/action_controller/test_case.rb#L83-L159 in a separate PR?

@carlosantoniodasilva @rafaelfranca can you take a look?

Owner
senny commented Jan 23, 2013

@carlosantoniodasilva I revisited this PR and made some changes to the CHANGELOG and the tests to reflect the intent better. Could you review it?

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Feb 4, 2013
actionpack/test/template/test_case_test.rb
+ controller.controller_path = "test"
+ render(template: "test/hello_world_with_partial")
+ e = assert_raise ActiveSupport::TestCase::Assertion do
+ assert_template partial: 'i_was_never_rendered', locals: { 'did_not' => 'happen' }
+ end
+ assert_match "i_was_never_rendered to be rendered but it wasn't", e.message
+ assert_match 'Expected ["/test/partial"] to include "i_was_never_rendered"', e.message
+ end
+
+ test 'specifying locals works when the partial is inside a directory with prefix' do
+ controller.controller_path = "test"
+ render(template: 'test/render_partial_inside_directory')
+ assert_template partial: 'test/_directory/_partial_with_locales', locals: { 'name' => 'Jane' }
+ end
+
+ test 'specifying locals works when the partial is inside a directory without prefix' do
carlosantoniodasilva
carlosantoniodasilva Feb 4, 2013 Owner

How about using with underline prefix and without underline prefix? I had to look at the assertion to remember that 😄.

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Feb 4, 2013
actionpack/lib/action_controller/test_case.rb
@@ -126,7 +126,11 @@ def assert_template(options = {}, message = nil)
if expected_partial = options[:partial]
if expected_locals = options[:locals]
if defined?(@_rendered_views)
- view = expected_partial.to_s.sub(/^_/,'')
+ view = expected_partial.to_s.sub(/^_/,'').sub(/\/_(?=[^\/]+\z)/,'/')
carlosantoniodasilva
carlosantoniodasilva Feb 4, 2013 Owner

Mind adding spaces after ,?

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Feb 4, 2013
actionpack/lib/action_controller/test_case.rb
@@ -126,7 +126,11 @@ def assert_template(options = {}, message = nil)
if expected_partial = options[:partial]
if expected_locals = options[:locals]
if defined?(@_rendered_views)
- view = expected_partial.to_s.sub(/^_/,'')
+ view = expected_partial.to_s.sub(/^_/,'').sub(/\/_(?=[^\/]+\z)/,'/')
+
+ partial_was_not_rendered_msg = "expected %s to be rendered but it wasn't" % view
carlosantoniodasilva
carlosantoniodasilva Feb 4, 2013 Owner

Is it common to use the contracted version like wasn't in messages? Just wondering :)

@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Feb 4, 2013
actionpack/CHANGELOG.md
@@ -1,5 +1,17 @@
## Rails 4.0.0 (unreleased) ##
+* `assert_template` can be used to verify the locals of partials,
+ which live inside a directory.
+ Fix #8516
+
+ # Prefixed partials inside directories worked and still work.
+ assert_template partial: 'directory/_partial', locals: {name: 'John'}
@carlosantoniodasilva carlosantoniodasilva commented on an outdated diff Feb 4, 2013
actionpack/CHANGELOG.md
@@ -1,5 +1,17 @@
## Rails 4.0.0 (unreleased) ##
+* `assert_template` can be used to verify the locals of partials,
+ which live inside a directory.
+ Fix #8516

Very cool 👍, just minor comments and we can merge. Thanks @senny.

senny added some commits Dec 15, 2012
@senny senny descriptive `assert_template` error when partial wasn't rendered
When `assert_template` is used with the :locals option, and the
partial was not rendered, a method_missing error was raised.
This changes first checks, if the partial actually was rendered
and raises a descriptive error.
c21ab33
@senny senny partials inside directory work with `assert_template`
previously when a partial was placed inside a directory
(eg. '/dir/_partial'), `assert_template` did not replace
the '_' prefix when looking through rendered tempaltes,
which resulted in an error.

I modified it to replace both, the leading '_' and the last '_'
after a '/'.
cce94e7
Owner
senny commented Feb 4, 2013

@carlosantoniodasilva thanks for the review. I incorporated your changes.

I think the problem persists on 3-2-stable. Should we consider a backport (let me know and I'll submit a PR)?

@carlosantoniodasilva carlosantoniodasilva merged commit 65113fd into rails:master Feb 4, 2013
@senny senny deleted the senny:8516_assert_template_with_locals branch Feb 4, 2013

I think it's fine to backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment