Skip to content

Loading…

`assert_template("")` is returning true even if a template has been rendered. #8302

Merged
merged 2 commits into from

3 participants

@roberto

After I got a false positive using a custom rspec matcher, I've figured out that assert_template("") was always returning true.

Example:

def index
  render :index
end
template = nil
get :index
assert_template("#{template}")

Change: assert_template("") fails if a template has been rendered, behaving like assert_template(nil).

@roberto roberto `assert_template` fails with empty string when a template has been re…
…ndered

For instance, it prevents false positive in this case:

    file = nil
    get :index
    assert_template("#{file}")
20723ca
@rafaelfranca rafaelfranca commented on an outdated diff
actionpack/lib/action_controller/test_case.rb
@@ -94,10 +94,14 @@ def assert_template(options = {}, message = nil)
matches_template =
case options
when String
- rendered.any? do |t, num|
- options_splited = options.split(File::SEPARATOR)
- t_splited = t.split(File::SEPARATOR)
- t_splited.last(options_splited.size) == options_splited
+ if options.empty?
+ rendered.blank?
@rafaelfranca Ruby on Rails member

Why need to check if the rendered is blank? Just return nil if options is blank

@carlosantoniodasilva Ruby on Rails member

!options.empty? && rendered.any? do ... end?

@roberto
roberto added a note

So, this way assert_template "" will fail in both cases:

  def test_with_empty_string_fails_when_template_rendered
    get :hello_world
    assert_raise(ActiveSupport::TestCase::Assertion) do
      assert_template ""
    end
  end

  def test_with_empty_string_passes_when_no_template_rendered
    get :nothing
    assert_raise(ActiveSupport::TestCase::Assertion) do
      assert_template ""
    end
  end

I'm not sure about what should be its behaviour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@carlosantoniodasilva
Ruby on Rails member

Looks fine I think.

@carlosantoniodasilva carlosantoniodasilva merged commit 4d0dc53 into rails:master
@carlosantoniodasilva
Ruby on Rails member

Ops, I didn't see your commits weren't squashed =(. Next time please squash them. Thanks.

@roberto

@carlosantoniodasilva, sorry about that. I didn't splash it before because I was checking another issue.

assert_template(foo: "bar") # it isn't raising anything and "passes"
assert_template(leiaute: "test") # misspelled option

assert_template is only handling the keys locals, partial or layout, anything else never fails or passes, giving false positives. We could call assert_valid_keys to check them:

when Hash
        options.assert_valid_keys(:layout, :partial, :locals)

If you are ok with that, then I'm gonna create another pull request.

Thanks.

@carlosantoniodasilva
Ruby on Rails member

I think it seems fine, go ahead with the pull request and we get more feedback there. I think :count is also an option, make sure you cover all of them from the implementation. Thanks!

@roberto

Alright, thank you again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 23, 2012
  1. @roberto

    `assert_template` fails with empty string when a template has been re…

    roberto committed
    …ndered
    
    For instance, it prevents false positive in this case:
    
        file = nil
        get :index
        assert_template("#{file}")
  2. @roberto
View
4 actionpack/CHANGELOG.md
@@ -1,5 +1,9 @@
## Rails 4.0.0 (unreleased) ##
+* `assert_template` is no more passing with empty string.
+
+ *Roberto Soares*
+
* Allow setting a symbol as path in scope on routes. This is now allowed:
scope :api do
View
2 actionpack/lib/action_controller/test_case.rb
@@ -94,7 +94,7 @@ def assert_template(options = {}, message = nil)
matches_template =
case options
when String
- rendered.any? do |t, num|
+ !options.empty? && rendered.any? do |t, num|
options_splited = options.split(File::SEPARATOR)
t_splited = t.split(File::SEPARATOR)
t_splited.last(options_splited.size) == options_splited
View
14 actionpack/test/controller/action_pack_assertions_test.rb
@@ -447,6 +447,20 @@ def test_with_nil_fails_when_template_rendered
end
end
+ def test_with_empty_string_fails_when_template_rendered
+ get :hello_world
+ assert_raise(ActiveSupport::TestCase::Assertion) do
+ assert_template ""
+ end
+ end
+
+ def test_with_empty_string_fails_when_no_template_rendered
+ get :nothing
+ assert_raise(ActiveSupport::TestCase::Assertion) do
+ assert_template ""
+ end
+ end
+
def test_passes_with_correct_string
get :hello_world
assert_template 'hello_world'
Something went wrong with that request. Please try again.