route_to Fails with Composing Matchers

[vizjerai]

This worked in RSpec 2.14.1 not sure what changed leading up to RSpec 3.0.2. but it doesn't look like route_to changed much between the two versions.

describe ProductsController do
  describe 'routing' do
    describe 'GET /products/*permalink' do
      it do
        expect(get: '/products/some/path').to route_to 'products#show', permalink: an_instance_of(String)
      end
    end
  end
end

I get an error basically saying 2 hashes are not equal. Most likely the culprit is assert_recognizes so I made a replacement that bypasses it and use recognize_path and do my own assert.

module RSpec::Rails::Matchers
  module RoutingMatchers

    class RouteToWithComposingMatcher < RouteToMatcher
      def matches?(verb_to_path_map)
        @actual = @verb_to_path_map = verb_to_path_map
        # assert_recognizes does not consider ActionController::RoutingError an
        # assertion failure, so we have to capture that and Assertion here.
        match_unless_raises ActiveSupport::TestCase::Assertion, ActionController::RoutingError do
          path, query = *verb_to_path_map.values.first.split('?')
          options = {method: verb_to_path_map.keys.first, extras: Rack::Utils::parse_nested_query(query)}
          route = @scope.routes.recognize_path(path, options)
          @scope.expect(route).to @scope.match(@expected)
        end
      end
    end

    def route_to(*expected)
      RouteToWithComposingMatcher.new(self, *expected)
    end

  end
end

[JonRowe]

Can you post the error? It might be you're misinterpreting it.

[JonRowe]

I'm not familar with this,

@scope.expect(route).to @scope.match(@expected)

unless @scope.match(@expected) returns an RSpec matcher it won't behave as expected...

[cupakromer]

I believe @scope is supposed to be the controller instance under test. I'd need to dig into the code though to be certain. In general, this isn't where routing checks should be made. We have route specs for this exact purpose.

@vizjerai could you move this to proper routing specs? Also, did you happen to update rails at the same time updated RSpec?

[myronmarston]

In general, matchers have to be changed to make them support receiving other matchers as arguments, and when I added the composable matchers feature to rspec-expectations I didn't think to look at the matchers here in rspec-rails. @cupakromer -- it's probably worth doing an audit of the matchers provided by rspec-rails and see which ones need to be updated. Let me know if you have questions about how to make matchers composable.

[vizjerai]

@JonRowe @scope is a reference the controller instance.

@cupakromer the code I put in was just a quick replacement of the matches? method in /lib/rspec/rails/matchers/routing_matchers.rb

I did not update rails between the upgrade from RSpec 2.14 to RSpec 3.0.2. I'm currently using rails 4.0.1 although I did run transpec when I was using rspec-rails 2.99.0. So it may have changed my routing specs. Either way running the spec I have above will fail using RSpec 3.0.2 and will pass if you change the permalink's value to 'some/path'.

Also looking at the current routing specs none of them use composable matchers.

[cupakromer]

@vizjerai are you saying your modification gets the error or the original matcher?

[cupakromer]

I think Myron is correct, we need to audit all of the matchers to make sure they are composable.

[vizjerai]

@cupakromer the original matcher gets the error. The modification allows it to pass but it may not pass all of the tests. I haven't tried modifying rspec-rails directly and run the tests.

[cupakromer]

Starting the work on this one. Well auditing matchers for proper composable implementations.

[fables-tales]

App that reproduces this: https://github.com/samphippen/demo_app_for_rspec_1108

[jtxyz]

The RouteToMatcher calls assert_recognizes which is defined in ActionDispatch's RoutingAssertions module. These use minitest assertions so can't accept RSpec matchers.

Rewriting the RoutingMatchers to do the comparison directly (@scope is the ExampleGroup) as in @vizjerai's initial comment would allow support for composable matchers, but that would also mean losing some of the functionality from the recognized_request_for like support for full URIs. The BeRoutableMatcher already uses @scope.routes.recognize_path so it would be consistent with that at least.

It seems like using expect within a matcher isn't the correct pattern, so we'd have to handle the case where recognize_path fails with an ActionController::RoutingError separately and use values_match? to compare the result in the case that it succeeds. Would something like this work?