assert_template with string gives false positives #3849

Closed
freegenie opened this Issue Dec 4, 2011 · 7 comments

Projects

None yet

6 participants

@freegenie

I found that the current implementation of assert_template can lead to false positives. Say we two templates 'world.html.erb' and 'hello_world.html.erb', when I get :hello_world, an assertion like

assert_template 'world' 

will succeed.

@freegenie

Failing test case freegenie@32e127a

@rmehner
rmehner commented Apr 5, 2012

Just wanted to open up a pull request fixing the exact same issue only to find this one :)

Tested @freegenie's code, works for us, would be nice to have that merged.

@brettfishman

+1 on this. A spec will pass (IMHO, wrongly) if the name of the expected template to be rendered matches a substring of one of the rendered templates (e.g. a template "accounts/hello_world" is actually rendered and the test asserts that "accounts/hello" is rendered), even if the template does not exist. Here's an example spec: https://gist.github.com/3056288

@freegenie freegenie added a commit to freegenie/rails that referenced this issue Sep 28, 2012
@freegenie freegenie Fixes issue #3849 assert_template false positive 2b4e19e
@HugoLnx HugoLnx added a commit to HugoLnx/rails that referenced this issue Sep 30, 2012
@HugoLnx HugoLnx `assert_template` no more passing with what ever string that matches.
Given Im rendering an template `/layout/hello.html.erb`, assert_template was
passing with any string that matches. This behavior allowed false passing like:

	assert_template "layout"
	assert_template "out/hello"

Now the passing possibilities are:

	assert_template "layout/hello"
	assert_template "hello"

fixing assert_template bug when template matches expected, but not ends with

Cherry Pick Merge: Fixes issue #3849 assert_template false positive

taking redundant test off

prevening incorrect assert_template when rendering with repeated names in path

updating CHANGELOG with bugfix: assert_template false passing
19dff78
@rafaelfranca
Member

Closed by 7d17cd2

@rafaelfranca rafaelfranca added a commit that referenced this issue Oct 31, 2012
@rafaelfranca rafaelfranca Revert "Merge pull request #7659 from HugoLnx/template_error_no_match…
…es_rebased"

This reverts commit 7d17cd2.

Conflicts:
	actionpack/CHANGELOG.md

Reason: This added a regression since people were relying on this buggy behavior.
This will introduce back #3849 but we will be backward compatible in
stable release.

Fixes #8068.
d5b275d
@msan
msan commented Nov 27, 2012

I've also have the problem:

test "should get new" do
get :new, :room_id => @room.name
assert_response :success
assert_template 'ne'
assert_template 'res'
end

templates ne and res do not exists but I have no failures.
The very strange thing is that is I do
assert_template 'nene' then I have one failure.

@rafaelfranca
Member

@msan it is already fixed on master

@thoughtafter

I've run into this issue recently. I wish the patch had not been reverted. I wanted to add for anyone else that comes across this that one fix is to use a Rails version after the regexp patch was added:

#5375

Of course for one of my projects I cannot upgrade yet. I've found that using the full template name will eliminate at least 1 case of false positives. So instead of:

assert_template :show

You can use:

assert_template 'show.html.erb'

This fixes the case of not matching show_custom.html.erb. However it would still match the false positive case of custom_show.html.erb.

Another relatively easy solution is to monkey patch assert_template which is what I will probably do for all of my projects.

Also, I did not know if I should open a separate bug but this behavior should be documented in assert_template. It is not mentioned anywhere that the it uses a substring match and, thus, is prone to false positives and is being maintained for purposes of backward compatibility. This should be documented because I think it is pretty obviously not the expected behavior. Let me know if I should open a separate bug on this.

Thank you.

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