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

Using the route matcher with 'format' can lead to failing specs, while everything is correct #809

Closed
aried3r opened this issue Oct 15, 2015 · 24 comments

Comments

@aried3r
Copy link

aried3r commented Oct 15, 2015

When having a route, or namespace like this:

namespace :api, defaults: { format: 'json' } do
  # some routes
end

and trying to assert this behavior using the route shoulda-matcher

it { is_expected.to route(:get, '/api/projects/1').to(action: :show, id: 1, format: :json) }

the spec will fail with something like:

The recognized options <{"controller"=>"projects", "action"=>"show", "id"=>"1", "format"=>"json"}> did not match <{"action"=>"show", "id"=>"1", "format"=>:json, "controller"=>"projects"}>, difference:.
--- expected
+++ actual
@@ -1 +1 @@
-{"action"=>"show", "id"=>"1", "format"=>:json, "controller"=>"projects"}
+{"controller"=>"projects", "action"=>"show", "id"=>"1", "format"=>"json"}

because the matcher will try to match the symbol (spec) against a string (route.rb).

See #693 for more details.

@thalesfp
Copy link

👍

@istana
Copy link

istana commented Oct 27, 2015

I have the same problem. I thought I can fix it with format: 'json' instead of format: :json in the spec, but no - it is still treated as symbol.

EDIT: actually you don't have to use defaults: { format: 'json' }, something like GET /articles/new(.:format) is enough.

and then it { is_expected.to route(:get, 'articles/new.json').to(action: :new, format: :json }

@dolzenko
Copy link

This seems to be introduced by 15359eb. I can't see where the format being a symbol requirement is documented, should we revert that commit? /cc @maurogeorge

@mcmire
Copy link
Collaborator

mcmire commented Nov 30, 2015

I think you're right, and I would agree that we should not be symbolizing the format -- in fact, we shouldn't even be changing it in any way.

@lfzawacki
Copy link

+1 I'm affected by the same problem. Tests are being skipped while waiting for a solution.

@dolzenko
Copy link

For those waiting for fix - just put this

Shoulda::Matchers::ActionController::RouteParams::PARAMS_TO_SYMBOLIZE = []

into spec_helper.rb. That worked for me.

@aried3r
Copy link
Author

aried3r commented Dec 17, 2015

If that works, would that be sufficient for a pull request?

@dolzenko
Copy link

@aried3r no, the whole thing is useless then

@aried3r
Copy link
Author

aried3r commented Dec 17, 2015

That's what I meant, can we remove it and be good?

@mcmire
Copy link
Collaborator

mcmire commented Dec 18, 2015

You might try:

Shoulda::Matchers::ActionController::RouteParams.class_eval do
  def symbolize_or_stringify(key, value)
    if key.in?(PARAMS_TO_SYMBOLIZE)
      value
    else
      stringify(value)
    end
  end
end

@zw963
Copy link

zw963 commented Nov 14, 2017

Hi, I work on a new team, this team still use rails 4.1.16.

I got follow error:

# config/routes
namespace :dingding_crm, defaults: { format: :json } do
        resources :specialists, only: [:show]
      end
end

Following spec is failed

require 'rails_helper'

RSpec.describe Api::DingdingCrm::SpecialistsController, type: :controller do
  it '' do
    should route(:get, 'api/dingding_crm/specialists/100')
      .to(action: :show, id: 100, format: :json)
  end
end

Error logs:

     Failure/Error: should route(:get, 'api/dingding_crm/specialists/100')
       The recognized options <{"format"=>:json, "action"=>"show", "controller"=>"api/dingding_crm/specialists", "id"=>"100"}> did not match <{"action"=>"show", "id"=>"100", "format"=>"json", "controller"=>"api/dingding_crm/specialists"}>, difference:.
       --- expected
       +++ actual
       @@ -1 +1 @@
       -{"action"=>"show", "id"=>"100", "format"=>"json", "controller"=>"api/dingding_crm/specialists"}
       +{"format"=>:json, "action"=>"show", "controller"=>"api/dingding_crm/specialists", "id"=>"100"}

I have to workaround this with change routes.rb to:

namespace :dingding_crm, defaults: { format: 'json' } do
        resources :specialists, only: [:show]
      end
end

This is worked again.

so, this issue still exists, right?

following is version:

 ╰─ $ 1  bundle list |grep shoulda
  * shoulda (3.5.0)
  * shoulda-context (1.2.2)
  * shoulda-matchers (2.8.0)

@mcmire
Copy link
Collaborator

mcmire commented Nov 15, 2017

@zw963 You're using an older version of shoulda-matchers, it looks like. What happens if you upgrade to 3.1.2?

@zw963
Copy link

zw963 commented Nov 15, 2017

@mcmire , hi, I have to development on a very old project (rails 4.1.16)

I only add

  gem 'shoulda'
  gem 'shoulda-matchers'

into Gemfile in recent days, those version seem like is the suitable version manage by bundler.

@mcmire
Copy link
Collaborator

mcmire commented Nov 15, 2017

@zw963 We do still support Rails 4.1, but I guess upgrading wouldn't help in this case, so I'm not sure why I said that :) I thought I had fixed this problem somewhere along the way but I guess I haven't yet. Sorry about that. Your "fix" to routes.rb seems like the right move.

@zw963
Copy link

zw963 commented Nov 16, 2017

@mcmire , so, there is no plan to try fix this for Rails 4.1, right ?

@mcmire
Copy link
Collaborator

mcmire commented Nov 16, 2017

@zw963 If it only shows up under Rails 4.1, then we won't be addressing it, but, I'm thinking that this problem may exist under other Rails versions, and if that's the case, then you would get this fix for free.

@istana
Copy link

istana commented Nov 16, 2017

AFAIK it hasn't been fixed yet. And probably never will be as there is a ton of PRs and no activity in repository.

@mcmire
Copy link
Collaborator

mcmire commented Nov 16, 2017

@istana Hey, I don't really appreciate your comments. Let's try to keep it civil around here. Thanks.

@zw963
Copy link

zw963 commented Nov 17, 2017

@mcmire , I tried in my another project, which is rails 5.1.1

Following is my route:

namespace :wx_mini_api, path: '' do
  namespace :v1, defaults: {format: :json} do
    resources :items, only: [:show, :index]
  end
end

Following is my test: (minitest)

class WxMiniApi::V1::ItemsControllerTest < ActionDispatch::IntegrationTest
  should route(get: 'items').to(action: :index, format: :json)
end

But I got following error:

 ╰─ $ 1  rails test /home/zw963/Xthink/ershou_web/test/controllers/wx_mini_api/v1/items_controller_test.rb
Running via Spring preloader in process 27342
/home/zw963/Xthink/ershou_web/test/controllers/wx_mini_api/v1/items_controller_test.rb:4:in `<class:ItemsControllerTest>': undefined method `route' for WxMiniApi::V1::ItemsControllerTest:Class (NoMethodError)
	from /home/zw963/Xthink/ershou_web/test/controllers/wx_mini_api/v1/items_controller_test.rb:3:in `<top (required)>'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/activesupport-5.1.1/lib/active_support/dependencies.rb:292:in `require'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/activesupport-5.1.1/lib/active_support/dependencies.rb:292:in `block in require'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/activesupport-5.1.1/lib/active_support/dependencies.rb:258:in `load_dependency'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/activesupport-5.1.1/lib/active_support/dependencies.rb:292:in `require'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/railties-5.1.1/lib/rails/test_unit/test_requirer.rb:14:in `block in require_files'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/railties-5.1.1/lib/rails/test_unit/test_requirer.rb:13:in `each'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/railties-5.1.1/lib/rails/test_unit/test_requirer.rb:13:in `require_files'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/railties-5.1.1/lib/rails/test_unit/minitest_plugin.rb:96:in `plugin_rails_init'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/minitest-5.10.3/lib/minitest.rb:81:in `block in init_plugins'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/minitest-5.10.3/lib/minitest.rb:79:in `each'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/minitest-5.10.3/lib/minitest.rb:79:in `init_plugins'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/minitest-5.10.3/lib/minitest.rb:130:in `run'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/railties-5.1.1/lib/rails/test_unit/minitest_plugin.rb:77:in `run'
	from /home/zw963/.rvm/gems/ruby-2.3.3@ershou_web/gems/minitest-5.10.3/lib/minitest.rb:63:in `block in autorun'
	from /home/zw963/.rvm/rubies/ruby-2.3.3/lib/ruby/site_ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
	from /home/zw963/.rvm/rubies/ruby-2.3.3/lib/ruby/site_ruby/2.3.0/rubygems/core_ext/kernel_require.rb:55:in `require'
	from -e:1:in `<main>'

Following is shoulda version.

 ╰─ $ bundle list |grep shoulda
  * shoulda (3.5.0)
  * shoulda-context (1.2.2)
  * shoulda-matchers (2.8.0)

@mcmire
Copy link
Collaborator

mcmire commented Nov 18, 2017

@zw963 Sigh... I'm sorry. shoulda has a dependency on shoulda-matchers 2.8.0 right now so that's what version it will use. I really need to address this. Does your project really use Minitest or can you use RSpec?

@zw963
Copy link

zw963 commented Nov 18, 2017

@mcmire,I don't understood what you means,this new rails 5.1.1 i use minitest.

@istana
Copy link

istana commented Nov 20, 2017

@mcmire I'm sorry for my reaction. I really appreciate the work you do here.

In my case it didn't work in Rails 4.2 either, but with shoulda matchers 2.8.0. I could try to trigger the bug with latest Rails 5.1 and shoulda matchers.

@mcmire mcmire added this to the v4.next milestone Feb 18, 2019
@philihp
Copy link

philihp commented Apr 23, 2019

Fixed this by just changing my routes.

  namespace :api, defaults: { format: 'json' } do

to

  namespace :api, defaults: { format: :json } do

@mcmire mcmire modified the milestones: v4.next, v4.2.0 Jun 9, 2019
aried3r added a commit to aried3r/shoulda-matchers that referenced this issue Oct 11, 2019
@guialbuk guialbuk modified the milestones: v4.2.0, v4.next Feb 18, 2020
@mcmire
Copy link
Collaborator

mcmire commented May 6, 2020

Hey folks. In an effort to lighten our load as maintainers and be able to serve you better in the future, the shoulda-matchers team is working on cleaning out the cobwebs in this repo by pruning the backlog. As there are few of us, there are a lot of items that will simply never earn our attention in a reasonable time frame, and rather than giving you an empty promise, we think it makes more sense to focus on more recent issues. That means, unfortunately, that we must close this issue.

Don't take this the wrong way: our aim is not to diminish the effort people have made or dismiss problems that have been raised. If you feel that we should reopen this issue, then please let us know so that we can reprioritize it. Thanks!

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

No branches or pull requests

9 participants