Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UrlGenerationError when testing mocked controllers that use url_for just to add params #2506

Open
henrahmagix opened this issue Jun 8, 2021 · 15 comments

Comments

@henrahmagix
Copy link

henrahmagix commented Jun 8, 2021

What Ruby, Rails and RSpec versions are you using?

Ruby version: 2.6.7
Rails version: 6.0.3.7
RSpec version:

RSpec 3.10
  - rspec-core 3.10.1
  - rspec-expectations 3.10.1
  - rspec-mocks 3.10.2
  - rspec-rails 5.0.1 (and 4.0.2 before updating)
  - rspec-support 3.10.2

Observed behaviour

Using redirect_to url_for(my_extra_param: 'a value') in a Concern, and then testing that concern with a controller test using controller(ActionController::Base.include(MyConcern)) and requesting an action defined in the test controller and drawn with routes.draw, always fails with ActionController::UrlGenerationError as it cannot find the test routes.

Expected behaviour

Test routes added with routes.draw are available and url_for succeeds to build a url of the current route.

Can you provide an example app?

EDIT: here's a repo that reproduces this issue: https://github.com/henrahmagix/rails-bug-app/tree/c887222dc542572c4453a2c0f987afeefd730065

I will leave the example originally posted here intact below.


Unfortunately rubygems is down right now, so I can't bundle install on a new app to double-check my setup shows the bug correctly. So here's the test file that I would add to rails new that shows it:

Click to expand

require 'rspec/rails'

RSpec.configure do |config|
  # rspec-expectations config goes here. You can use an alternate
  # assertion/expectation library such as wrong or the stdlib/minitest
  # assertions if you prefer.
  config.expect_with :rspec
  config.mock_with :rspec
  config.filter_rails_from_backtrace!
end

module MockConcern
  extend ActiveSupport::Concern

  included do
    before_action :redirect_if_requested
  end

  private

    def redirect_if_requested
      redirect_to url_for(redirected_from_concern: true) if params[:redirect_me] == 'yes'
    end
end

describe 'url_for bug in concern tests', type: :controller do
  controller(ActionController::Base.include(MockConcern)) do
    def test
      render plain: 'testing'
    end
  end

  before do
    routes.draw do
      get "test" => "anonymous#test"
    end
  end

  it 'succeeds when url_for is not called in the concern' do
    get :test, params: { redirect_me: 'no' }, session: nil
    expect(response).not_to be_redirect
  end

  it 'fails when url_for is called in the concern' do
    get :test, params: { redirect_me: 'yes' }, session: nil
    expect(response).to be_redirect
  end
end

class MockController < ActionController::Base
  def in_real
    redirect_to url_for(extra_param: 'in_real')
  end
end

describe 'url_for bug in controller tests', type: :controller do
  controller(MockController) do
    def test
      render plain: 'testing'
    end

    def in_mock
      redirect_to url_for(extra_param: 'in_mock')
    end
  end

  before do
    routes.draw do
      get "test" => "mock#test"
      get "in_real" => "mock#in_real"
      get "in_mock" => "mock#in_mock"
    end
  end

  it 'succeeds when url_for is not called' do
    get :test, params: nil, session: nil
    expect(response).not_to be_redirect
    expect(response.body).to eql('testing')
  end

  it 'fails when url_for is called in the real controller' do
    get :in_real, params: nil, session: nil
    expect(response).to be_redirect
  end

  it 'fails when url_for is called in the mock controller' do
    get :in_mock, params: nil, session: nil
    expect(response).to be_redirect
  end
end

I will link to a rails new app that I've confirmed shows the same results once rubygems is back up =)

@pirj
Copy link
Member

pirj commented Jun 8, 2021

What if you

  controller do
    include MockConcern
    def test
      render plain: 'testing'
    end
  end

@henrahmagix
Copy link
Author

@pirj unfortunately it still fails =)

@JonRowe
Copy link
Member

JonRowe commented Jun 8, 2021

If it fails in the real controller as well as the mock controller then this is a Rails bug / a mistake in your configuration of the routes. Rails routing configuration is quite finicky.

@henrahmagix
Copy link
Author

henrahmagix commented Jun 8, 2021

@JonRowe Sorry, I think I've caused confusion with my poor naming choices in the example code I pasted. I've now made a sample repo, please refer to that =) https://github.com/henrahmagix/rails-bug-app/tree/c887222dc542572c4453a2c0f987afeefd730065

The sample repo shows that url_for works correctly in a concern under testing when included in a controller with a defined route. Then it shows what I am attempting to do, which is test the concern separately:

url_for is unable to match routes drawn for the test; routes defined in config/routes.rb are fine.

Hope that clears this up =)


Test output:

$ bundle exec rspec -fd

RedirectIfRequestedConcern
  is expected not to be redirect
  is expected to have attributes {:body => "Test controller, test method"}
  with redirect_me=yes
    example at ./spec/app/controllers/concerns/redirect_if_requested_concern_spec.rb:27 (FAILED - 1)

HomeController
  #show
    is expected not to be redirect
    is expected to have attributes {:body => "Home controller, show method"}
    with redirect_me=yes
      is expected to redirect to "http://test.host/home?redirected_from_concern=true"

Failures:

  1) RedirectIfRequestedConcern with redirect_me=yes 
     Failure/Error: redirect_to url_for(redirected_from_concern: true) if params[:redirect_me] == 'yes'
     
     ActionController::UrlGenerationError:
       No route matches {:action=>"test", :controller=>"anonymous", :redirected_from_concern=>true}
     # ./app/controllers/concerns/redirect_if_requested_concern.rb:11:in `redirect_if_requested'
     # /Users/henryblyth/.rbenv/versions/2.6.7/gemsets/rails-bug-app/gems/actiontext-6.1.3.2/lib/action_text/rendering.rb:20:in `with_renderer'
     # /Users/henryblyth/.rbenv/versions/2.6.7/gemsets/rails-bug-app/gems/actiontext-6.1.3.2/lib/action_text/engine.rb:55:in `block (4 levels) in <class:Engine>'
     # ./spec/app/controllers/concerns/redirect_if_requested_concern_spec.rb:16:in `block (2 levels) in <top (required)>'

Finished in 0.70503 seconds (files took 2.78 seconds to load)
6 examples, 1 failure

Failed examples:

rspec ./spec/app/controllers/concerns/redirect_if_requested_concern_spec.rb:27 # RedirectIfRequestedConcern with redirect_me=yes

@henrahmagix
Copy link
Author

henrahmagix commented Jun 8, 2021

I've just noticed something that might help pin-down the issue here:

An empty controller {} declaration in an otherwise-ok controller test (where described_class is a controller) causes the test's request call to fail to find a matching route, only if it comes before the request declaration. If it comes after, there's not change.

I've pushed a commit that shows this to the sample repo: see henrahmagix/rails-bug-app@cbd5cf7

(All sample repo links before this are canonical btw, so they point to the code at the time of submission.)


# bad
describe HomeController, type: :controller do
  controller {}

  before { get :show }
# good
describe HomeController, type: :controller do
  before { get :show }

  controller {}

@pirj
Copy link
Member

pirj commented Jun 9, 2021

Nice find.
The last example seems to be affected by the fact that we draw the default routes in before, and the order of hook execution comes into play. If get is executed before we draw, it fails. See lib/rspec/rails/example/controller_example_group.rb:

        def controller(base_class = nil, &body)
          before do
            self.routes = ActionDispatch::Routing::RouteSet.new.tap do |r|
              r.draw do

Yet another interesing thing is how routes.draw is implemented, see Rails' actionpack/lib/action_dispatch/routing/route_set.rb:

      def draw(&block)
        clear! unless @disable_clear_and_finalize
        eval_block(block)
        finalize! unless @disable_clear_and_finalize
        nil
      end

You may see that it clears routes on each call. Unless

There's also this rspec-rails's option config.infer_base_class_for_anonymous_controllers:

You can also disable base type inference, in which case anonymous controllers will inherit from ApplicationController instead of the described class by default

Where "described class" in the failing test is RedirectIfRequestedConcern, and you should get a:

TypeError:
  superclass must be an instance of Class (given an instance of Module)

if you don't change the test to use a quoted class name:

RSpec.describe "RedirectIfRequestedConcern", type: :controller do

or set infer_base_class_for_anonymous_controllers to false.

@pirj
Copy link
Member

pirj commented Jun 9, 2021

The simplest workaround is to use the following instead of url_for:

redirect_to '/test?redirected_from_concern=true' if params[:redirect_me] == 'yes'

A trick with

  before do
    routes.disable_clear_and_finalize = true
    routes.draw { get 'test' => 'anonymous#test' }
    routes.finalize!

didn't work for me.

After looking at lib/rspec/rails/example/controller_example_group.rb:

              r.draw do
                resources resource_name,
                          as: resource_as,
                          module: resource_module,
                          path: resource_path

where we draw a resourceful route, I was under the impression that switching to show/index would do the job.
And it partially does:

RSpec.describe RedirectIfRequestedConcern, type: :controller do
  controller do
    include RedirectIfRequestedConcern

    def index
      render plain: 'Test controller, test method'
    end
  end

  it do
    get :index, params: { redirect_me: 'yes' }, session: nil
    expect(response).to redirect_to('http://test.host/test?redirected_from_concern=true')
  end
end

this test gets into redirect_if_requested that fails with the same:

     ActionController::UrlGenerationError:
       No route matches {:action=>"index", :controller=>"anonymous", :redirected_from_concern=>true}

Do you have enough information to dig deeper why routes work fine in the spec, but not the controller itself?

@henrahmagix
Copy link
Author

henrahmagix commented Jun 10, 2021

@pirj Thanks for your full response =)

Do you have enough information to dig deeper why routes work fine in the spec, but not the controller itself?

So I've been trying to figure this out, but it doesn't seem possible to me. Maybe smarter minds than mine could confirm?

All I know is that if I print the routes available, they never include the routes drawn up in the concern test: see henrahmagix/rails-bug-app@5154e64

This makes sense to me. Like, how would it be possible for the application to know what routes RSpec has drawn to itself?

I can see how request methods in tests work with mocked controllers because RSpec calls the method directly on the controller, but once we're inside code that's not defined by the test (i.e. the concern being tested), then all we have is Rails.application.routes, which is finalized and so cannot be appended to.

Does this mean, for RSpec to work for my needs, Rails.application.routes needs to be rebuilt(/redrawn) with appended test routes? I'm not exactly sure how to do that myself, or even if it's worth doing.

In the end, is this kind of unit testing of concerns just not possible with RSpec? That's ok if so, but I would greatly appreciate it if smarter minds than mine could see if this kind of route manipulation is possible for these tests =)


The simplest workaround is to use the following instead of url_for:

redirect_to '/test?redirected_from_concern=true' if params[:redirect_me] == 'yes'

Unfortunately I can't use that: my real-life usage is in a concern that's shared amongst many controllers, so I need to be able to use url_for to add/overwrite a path param without knowing what the current route is.

Specifically, we're using RouteTranslator's localized to automatically duplicate all routes with the locale as the first path part, and we have a shared concern that needs to set the locale route param. As far as I can tell, url_for(locale: new_locale) is the only way (or rather, the correct way) to do this for every route without having to know what the route is.

In the very least, in our concern code I'm certain that we need to stick to using Rails url generation helpers, and not string manipulation, to ensure that we are generating correct urls for routes that exist.

Apologies for not producing a sample repo more accurate to my real-life usage =)


As a "silver lining", I'm very glad for my increased knowledge of how RSpec Controller tests work now, so I'm less likely in the future to get frustrated by "No route matches" errors because I understand how everything is constructed now =)

@JonRowe
Copy link
Member

JonRowe commented Jun 10, 2021

Routing wise, your example app is the correct behaviour, anonymous controllers only have anonymous routes

@pirj
Copy link
Member

pirj commented Jun 10, 2021

Wondering if stubbing url_for to return some generic path, e.g.

allow(controller).to receive(:url_for).with(locale: 'fr').and_return('/test?locale=fr')
expect(controller).to receive(:url_for).with(locale: 'fr')
get :test, ...
expect(response).to redirect_to(...)

Actually, when you describe the use case, it seems like such a natural thing to do to test controller concerns in such a way. And redirecting from controller concerns is not a rare thing either. Wondering how wasn't this reported earlier. Maybe there's some cool trick I lack knowledge of how to cover such concerns with tests.

@henrahmagix
Copy link
Author

henrahmagix commented Jun 11, 2021

Does anyone know of a way to save existing Rails.application.routes, start Rails.application.draw (so they're all cleared and we can add custom routes) and then reinsert them?

Essentially, is it possible to insert already-compiled routes, or must every route be compiled from source? Because if it's the latter, then most uses of config/routes.rb won't allow the redrawing of the routes.

Like, to be able to do this would be great:

# config/routes.rb
Rails.application.routes.draw do
  # For details on the DSL available within this file, see https://guides.rubyonrails.org/routing.html

  get 'home' => 'home#show'
end

# spec/my_new_test_spec.rb
before do
  @orig_routes = Rails.application.routes
  Rails.application.routes.draw do
    insert @orig_routes

    get 'my_new_test' => 'my_new_test#show'
  end
end

after do
  Rails.application.routes.draw do
    insert @orig_routes
  end
end

Perhaps there's a way to decompile a route back to its get 'path' => 'controller#action' definition 🤔

(It's not possible to do Rails.application.routes = unfortunately)

@henrahmagix
Copy link
Author

henrahmagix commented Jun 11, 2021

I've managed to get these example tests passing with changes to RSpec::Rails::ControllerExampleGroup (see henrahmagix/rails-bug-app@09e82ca) but with a massive caveat and it doesn't seem like a viable solution for this library.

But I feel that the gist of it is what I would love this library to be able to do: add to existing routes before each test and revert the changes afterwards. That way, all routes drawn up in tests are then available in app code when it's being tested =) (or just specific routes if we want to make this an additional option instead of a fundamental change)

EDIT: I just force-amended the commit with extra comment info, and have replaced the commit link here

@henrahmagix
Copy link
Author

I haven't had any more luck with this :'<

@genehsu
Copy link

genehsu commented Oct 25, 2021

@henrahmagix I've tried to reproduce this with your bug repo and a linux vm and was able to run your test successfully. Here's a gist of the output: https://gist.github.com/genehsu/d484317924aa9358ab887f7967ea5505

@RichardsonWTR
Copy link

RichardsonWTR commented Feb 24, 2022

I'm not using url_for, but this thread helped me to fix my problem, so I'll share it with everyone.
I got this working using rspec 3.9.0 and rspec-rails 3.9.1

require 'rails_helper'

class MyConcernFakeController < ApplicationController
  include MyAuthConcern

  def show_status
    render json: {status: 'ok'}
  end
end

RSpec.describe MyConcernFakeController, type: :controller do

  context 'without authentication header' do
    before do
      routes.disable_clear_and_finalize = true
      routes.draw { get 'show_status' => 'my_concern_fake#show_status' }
      routes.finalize!
    end

    it 'raises error' do
      get :show_status
      expect(response).to have_http_status(401)  
    end
  end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants