[rspec-users] Possible improvements to routing spec API

Wincent Colaiuta win at wincent.com
Wed Jul 7 02:24:09 EDT 2010


El 07/07/2010, a las 01:16, Lalish-Menagh, Trevor escribió:

> Hi David,
> 
> You make a good point. I was talking with a coworker about this
> problem, and he suggested a simpler format, that I think will coincide
> some with Wincent's thoughts. Here is my next stab
> (http://gist.github.com/466064):
> describe "routing" do
>  it "recognizes and generates #index" do
>    get("/days").should have_routing('days#index')
>  end

Very nice. I definitely like this better than the "map()" terminology that I used.

>  it "generates #index" do
>    get("/days").should generate('days#index')
>  end

Reads well, _but_ I think you've got it back-to-front. assert_generates, and therefore the corresponding matcher in RSpec, asserts that a _path_ can be generated from a set of _params_ (controller, action, additional params), but here you're asserting the opposite.

(One thing to note is that assert_generates really only does assert about the _path_, not the method + path, but it's still nice to write it as "get(path)" just for symmetry with the reverse version of the spec.)

>  it "recognizes #index" do
>    ('days#index').should recognize get("/days")
>  end

This one is back-to-front too; assert_recognizes is an assertion that a path (this time method + path) is recognized as a particular route (action, controller, additional params) but here you're asserting the opposite.

If we're really serious about using the same verbs as the Rails assertions, and we want to forward and reverse versions of the spec to read in the same way, we could use something like:

  specify { get('/days').should recognize_as('days#index') }
  specify { get('/days').should generate_from('days#index') }

If we don't mind swapping the order around

  specify { get('/days').should recognize_as('days#index') }
  specify { 'days#index'.should generate('/days') }

Note that in the "generate" spec here I drop the "get()" seeing as Rails isn't actually looking at the method, and it is just verbiage IMO to start nesting other method calls inside the generate() matcher.

Weighing up the two orders, I prefer the first pair of specs, I think, because:

- the repetition of the pattern makes it easier to remember and apply.

- if you have to supply additional parameters (as you do in your later example), using the first format you can write ('days#index', :student_id => '1') whereas with the second format you are obliged to involve another method like with() in order to express it as "('days#index').with(:student_id => '1')".

>  describe "nested in students" do
>    it "recognizes #index" do
>      ('days#index').with(:student_id => "1").should recognize
> get("/students/1/days")
>    end
>  end
> end

This is another back-to-front one, I think. You're not recognizing here, but generating.

> Notes:
> - I am using have_routing, recognize, and generate because those are
> the verbs used in Rails for the functions we are wrapping.

Seems like a good idea that we should definitely try to do, although with one reservation; if we feel that the language is confusing or suboptimal then we should feel free to change it.

I personally have lingering doubts about the Rails language because I think it _is_ confusing in a way. Look at the way you used recognize and generate back-to-front in this message. And I know that every time I want to make a comment about "assert_recognizes" and "assert_generates" I end up double-checking the API docs just to make sure that I'm about to use them in the right way.

> - I like using the word "with" to represent "extras," in the Rails API
> (see http://edgeapi.rubyonrails.org/classes/ActionDispatch/Assertions/RoutingAssertions.html),
> since I think it fits here, and we already use with in RSpec in other
> places (like stubs, etc.).

I don't really like it; I think it adds unnecessary verbiage to the specs.

> - I like using get('path') since it is similar to the routing calls in
> the Rails 3 route file, and I think it is easy to intuit.
> - We can use the hash notation to conform to Rails 3, with an option
> to provide a full hash as well (Rails 2 style).

Yes, the example I posted at http://gist.github.com/464081 does that. 

> - The format still reads like English, and using "have_routing"
> instead of "routes_to" avoids the "_to" and "_from" problem that we
> have been talking about, PLUS it makes it easier to draw a
> relationship between the RSpec command and the Test::Unit command
> (assert_routing).

Yeah, I think "have_routing" is a definite improvement over "map".

I am less convinced that "recognize_as" is an improvement over the highly readable "map_to".

I am less attached to "map_from", though, and think "generate_from" might be an improvement.

Like I said above, I think the Rails terminology does have the potential to be confusing, so I don't necessarily feel "wedded" to it.

> - Using these verbs still allow "should_not" to make sense.

Yes, I think that's important.

One thing I wanted to add is that I discovered in my testing that the existing "be_routable" matcher isn't very useful. This is because it relies on "recognize_path", which I am not sure is public API or not, and "recognize_path" sometimes recognizes the unrecognizable... For example:

  in config/routes.rb:
    resource :issues, :except => :index

  visit issues#index (/issues) in your browser:
    get a RoutingError (expected)

  in your spec:
    recognize_path('/issues', :method => :get) # recognizes!

So this means you can't do stuff like get('issues#index').should_not be_routable, because the Rails recognize_path() method will tell you that it _is_ routable. Seems that it is only useful for a subset of unroutable routes (like completely non-existent resources, for example).

If you look in the Gist you'll see that I work around this limitation by adding a "be_recognized" matcher which doesn't use "recognize_path()" under the hood. Instead it uses "assert_recognizes" and decides what to do based on what kind of exception, if any, gets thrown by it. So you can write:

  specify { get('/issues').should_not be_recognized }

(Funnily enough, assert_recognizes() _does_ use recognize_path() under the hood, but it's evidently doing some additional set-up and tear-down to make it work. It seems like wrapping assert_recognizes rather than replicating its logic, though, is the right thing to do.)

Cheers,
Wincent



More information about the rspec-users mailing list