Routing specs can set their own routes

[alindeman]

(For example, to test Rails engines)

Prior to 2.12.1, routing specs could override the routeset by setting
the ivar @routes. However, after putting a delegation layer between us
and Rails, this no longer worked correctly. I do not think that setting
@routes was necessarily supposed to be a public interface, so I do not
think we have a duty to support it.

This pull request exposes a routes setter that can correctly override
the routeset, and adds it explicitly as a public interface.

I'm still mulling over the solution and the aesthetic of the setter, but
I figured I'd get something out there for feedback.

@myronmarston, @dchelimsky, what do you think?

Related: #668

[myronmarston]

It's a little surprising to me that you set @routes both on self and the assertion instance. Is it needed on both?

[alindeman]

Yes, I think so. I set it on self so the attr_reader works correctly (this has been in the public interface for a while) and I set it on assertion_instance because that is the place where the URL helpers are expected to be delegated to.

It's not ideal, I agree, but I feel like there are limited ways we can isolate ourselves ... and the Rails test module expects this instance variable to exist.

[myronmarston]

A setter is pretty natural here, but one concern is that it's prone to users making this mistake:

before { routes = my_routes }

...which assigns my_routes to a local variable named routes. self. must be used to use the setter method, but this is pretty non-obvious.

[alindeman]

A setter is pretty natural here, but one concern is that it's prone to users making this mistake:

before { routes = my_routes }

...which assigns my_routes to a local variable named routes. self. must be used to use the setter method, but this is pretty non-obvious.

I had a similar thought. Are there other options? A use_routes method or similar, maybe?

[dchelimsky]

What about exposing a wrapper for the Rails routes API at the class level, e.g.

describe ThingsController do
  routes.draw do
    map.resources :things
  end

  it "does something" do
    # ...
  end
end

The draw method would init a new Routes instance and call draw on it with the block, and then assign it to both @routes and assertion_instance.instance_variable_set(:@routes, @routes). That way it's explicit and uses a familiar API.

[alindeman]

What about exposing a wrapper for the Rails routes API at the class level, e.g.

The use case I'm really trying to nail is engines (#668) and this type of routing spec.

That is, we need to be able to set the routeset to a completely new one, not just modify it.

Maybe?

describe "things routing" do
  routes { MyEngine::Engine.routes }
end

[dchelimsky]

I like routes { ... }.

[alindeman]

I like routes { ... }.

Cool. I'll try to get this out before 12, but not sure it'll make it.

[gemyago]

Hello everyone. After upgrading to Rails 3.2.12 I've faced an issue with engine routes specs, basically engine routes are not recognized without explicitly setting routes instance variable (in before hook) like this:

assertion_instance.instance_variable_set(:@routes, @routes)

So I believe the approach proposed by @alindeman will solve the issue in a more acceptable way.

[alindeman]

Closing this to focus on #668. I think the best answer will be to pull in mounted_helpers somehow.

[incorvia]

I'm glad this was pulled in and I'm glad it works but I must say this is certainly testing me. For sake of clarity and future developers...

Routing Tests

This works inside of routing tests:

before(:each) do
  self.routes = MyEngine::Engine.routes
end

And again this succeeds

routes { MyEngine::Engine.routes }

But this fails

before(:each) do
  @routes = MyEngine::Engine.routes
end

The reason the first works is because it uses the setter routes= which also sets @routes inside the assertion_instance:

def routes=(routes)
  @routes = routes
  assertion_instance.instance_variable_set(:@routes, @routes)
end

The second example works because it defines a before block which actually implements the first solution that uses the routes= setter thus also setting the instance variable inside the assertion_instance. The last one fails because it never sets @routes inside the assertion_instance.

My understanding from the ticket conversation seems to indicate that when the test is run, url_helpers execute in the context of the assertion_instance so the @routes instance variable needs to be set there for everything to work properly. This is a little confusing because from what I can gather about controller tests the DSL is almost the exact opposite.

Controller Tests

This allows routes to work even though in routing tests it fails:

before(:each) do
  @routes = MyEngine::Engine.routes
end

But this causes them to fail even though in routing tests it succeeds:

before(:each) do
  self.routes = MyEngine::Engine.routes
end

And the other option of using the class method 'routes' fails where it succeeded before:

routes { MyEngine::Engine.routes }

The second fails because routes= is not a defined method.. and neither is the class level 'routes' method so the third fails as well. I think its a bit confusing that everything that works in the context of routing fails in terms of controllers and vice-versa. Just my thoughts. Thanks for all the work guys.

[alindeman]

I'm about to push a fix for controller specs to unify the syntaxes for routing and controller specs. I did not initially realize that anyone would be overriding routes in a controller spec for the purposes of using routing matchers. But since we do allow it, we will unify them for rspec-rails 2.13.1.

I might edit your comment, @incorvia, after I push the change to allay any confusion.

Great writeup. I appreciate you pushing me to make it better.

[alindeman]

As an aside, while this feels like a breaking change, I'm not sure we ever meant to allow folks to override @routes from the outside. We are between a rock and a hard place because we have to insulate ourselves from Rails 4 more than we needed to previously, and we can't receive notifications that someone changed an instance variable.

So, we're introducing a new syntax routes { ... } to allow it publicly.

[incorvia]

@alindeman wonderful and please do! Thank you so much.