Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Rspec < 2.8 scaffold view specs create deprecations on rails 3.2.0.rc2 #485

Closed
petervandenabeele opened this Issue Jan 16, 2012 · 17 comments

Comments

Projects
None yet
5 participants

A project created with rails 3.1.3 and rspec 2.7.x and haml with e.g.

  rails generate scaffold Contact email:string

has created a large number of ../contact_app/spec/views/contacts/{new|edit|index|show}.html.haml_spec.rb
files that have the line

3    describe "contacts/new.html.haml" do

This problem is resolved for newly created scaffolds in

1ccb17c

but for existing projects which migrate to rails 3.2.x a large amount of deprecation warnings may
show up. The style is:

contact_app$ rspec
...............................................DEPRECATION WARNING: Passing a template handler in the template name is deprecated. You can simply remove the handler name or pass render :handlers => [:haml] instead. (called from block (2 levels) in <top (required)> at .../contact_app/spec/views/people/new.html.haml_spec.rb:15)
.DEPRECATION WARNING: Passing a template handler in the template name is deprecated. You can simply remove the handler name or pass render :handlers => [:haml] instead. (called from block (2 levels) in <top (required)> at .../contact_app/spec/views/people/index.html.haml_spec.rb:24)

The resolution seems clear (change line 3 in the affected view specs).

But, how could we document this so that existing rspec-rails users that migrate up to Rails 3.2
are informed about this issue and the solution. At the same time they will migrate to rspec-rails
2.8, so maybe there is an option there to have an "after gem upgrade message" ?

Owner

dchelimsky commented Jan 16, 2012

Thanks for the thorough explanation of the issue. Makes me wish we had language level support for deprecations that allowed upstream libs (like rspec-rails) to capture them when coming from downstream libs (like rails) and then modify them. That would be a great solution to this problem, but alas ...

At first glance, I'm at a loss for how to document this in a programatic way that is certain to be seen when it needs to be. Installation messages only happen once, and are oft ignored.

Maybe the solution should be to raise our own deprecation warning that explains how to solve the problem. You'd end up w/ twice the deprecation warnings at first, but at least you'd know what to do. WDYT?

Do I understand your proposal correctly that:

  • user runs rspec (2.8)
  • the output has a large number of the deprecation warnings (since rails 3.2.0.rc2)
    (I am not sure if those come through STDOUT or STDERROR)
  • rspec 2.8 detects that particular deprecation message
  • rspec 2.8 prints a message after all tests are finished that explains the issue
Owner

dchelimsky commented Jan 17, 2012

No - rspec would issue its deprecation warning when it sees the .format.handler (e.g. .html.erb) in the path passed to render.

Mange commented Jan 26, 2012

The problem runs a bit deeper.

  • rspec-rails documentation still show "path/view.format.handler" for the describe blocks
  • There's no easy way to scope examples to which template you are actually specing:
describe "posts/show" do # want to test show.html
  # ...
end

describe "posts/show" do # want to test show.xml
  # ...
end

That could be solved by adding some new methods to view examples

describe "posts/show" do
  describe "as HTML" do
    view_format :html
    # ...
  end

  describe "as XML" do
    view_format :xml
    # ...
  end
end

view_format would default to :html, and it would not set :format when set explicitly to a render call. This would be a bit bad, though, since it means that view specs need to be merged and the nice 1:1 relationship between a spec and a view would be lost.

Another option might be to let RSpec be a bit smarter about it all (a bit too smart in my taste): Parse the view name as "#{base}.#{format}.#{handler}" and split it off as a hash to render.

A work-around I applied for a /api interface that has both :html and :xml views is this:

describe "api/index" do
  it "renders a list of resources" do
    render(:template => 'api/index', :formats => [:xml])

Implementing the "as HTML" and "as XML" around block would be nice.

Mange commented Feb 2, 2012

I'm using the following solution right now. You can override in specific contexts, of course:

describe "some_path/some_view.format.handler" do
  def render
    super(:template => "some_path/some_view", :formats => [:format], :handlers => [:handler])
  end
end

It's a bit inflexible, yes, but it works for me since I rarely pass any arguments to render.

@Mange Interesting. If I see correctly, that is a manual implementation of your earlier proposal:

" ... Another option might be to let RSpec be a bit smarter about it all (a bit too smart in my taste): Parse the view name as "#{base}.#{format}.#{handler}" and split it off as a hash to render. ... "

But in reality, this might be workable as a more general solution. In that case, the default case would
still work (smart, maybe too smart indeed?), but one could still force the format and template to be
used by explicitly setting it in the parameter list that render is called with (the trick is to use the correct
merge_reverse in the implementation).

That may be a more fundamental solution than documenting the problem and asking developers to change all
the describe strings.

What do you think?

Mange commented Feb 7, 2012

I'm still not sure which approach would be most appropriate, but I'm sure that view specs are tightly coupled with the template files and should require almost no setup for that reason.

Here's an almost working implementation:

# module ViewExampleGroup

def render(options={}, local_assigns={}, &block)
  options = default_render_options if Hash === options and options.empty?
  super(options, local_assigns, &block)
end

private

def default_render_options
  view, format, handler = _default_file_to_render.split('.', 3)
  if view and format and handler
    {:template => view, :formats => [format], :handlers => [handler]}
  else
    {:template => _default_file_to_render}
  end
end
Owner

dchelimsky commented Feb 7, 2012

@Mange that seems pretty workable to me. Why is it "almost" working (i.e. what's not working about it yet)?

Mange commented Feb 7, 2012

@dchelimsky Oh, it's just not tested (in either sense). I wrote the code directly here in the comment box. See it as psuedo-code just to demonstrate the problem in a general sense. I would not be surprised it it did work, even when copied verbatim.

OK, that explains :-) (I thought the formats array needs to have a symbol, not a string).
Sorry, but I will not have time in the short run to test this ...

I've been upgrading to 3.2 recently and ran into this - @Mange's fix looks pretty good to me, although I did run into problems where some of our templates don't specify a format (we have a custom handler for rendering, eg "index.rbapi"). Not sure how feasible it would be to make it more flexible... does it need to handle template localizations as well?

I've been using this slightly modified version of @Mange's fix :

module Rails32DeprecationHelper
  # https://github.com/rspec/rspec-rails/issues/485
  def render(options={}, local_assigns={}, &block)
    options = default_render_options if Hash === options and options.empty?
    super(options, local_assigns, &block)
  end

  private

  def default_render_options
    # pluck the handler, format and locale out of, eg, posts/index.de.html.haml
    template, *components = _default_file_to_render.split('.')
    handler, format, locale = *components.reverse

    render_options = {:template => template}
    render_options[:handlers] = [handler] if handler
    render_options[:formats] = [format] if format
    render_options[:locales] = [locale] if locale

    render_options
  end
end

config.include(Rails32DeprecationHelper, :type => :view)

which seems to be working pretty well for our needs.

Owner

dchelimsky commented Apr 10, 2012

@jdelStrother care to submit that as a pull request with specs?

Sure. Might take a short while, today is turning out busier than expected...

Owner

dchelimsky commented Apr 10, 2012

Take your time. The earliest date for the 2.10 release is 4/21.

@justinko justinko closed this May 27, 2012

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