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.

Sign up for GitHub

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

Updates shoulda gems #2665

Closed
wants to merge 3 commits into from
Closed

Updates shoulda gems #2665

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b4fd12a
Updates shoulda-matchers
jsmadis Oct 25, 2021
68d4cd0
MonkeyPatch for Route Params of shoulda-matchers
jsmadis Oct 26, 2021
803475d
Change should route to assert_routing because should route won't acce…
jsmadis Oct 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,9 +212,9 @@ group :test do
gem 'rspec_api_documentation'
gem 'rspec-html-matchers', github: '3scale/rspec-html-matchers', branch: 'fix/rspec-3-with-xml-document', require: false

gem 'shoulda', '~> 3.5.0', require: false
gem 'shoulda-context', '~> 1.2.2'
gem 'shoulda-matchers', '~> 2.8.0'
gem 'shoulda', '~> 4.0.0'
gem 'shoulda-context', '~> 2.0.0'
gem 'shoulda-matchers', '~> 4.5.1'
gem 'timecop', '~> 0.9'

gem 'ci_reporter_shell', github: '3scale/ci_reporter_shell', require: false
Expand Down
18 changes: 9 additions & 9 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1810,12 +1810,12 @@ GEM
selenium-webdriver (3.142.7)
childprocess (>= 0.5, < 4.0)
rubyzip (>= 1.2.2)
shoulda (3.5.0)
shoulda-context (~> 1.0, >= 1.0.1)
shoulda-matchers (>= 1.4.1, < 3.0)
shoulda-context (1.2.2)
shoulda-matchers (2.8.0)
activesupport (>= 3.0.0)
shoulda (4.0.0)
shoulda-context (~> 2.0)
shoulda-matchers (~> 4.0)
shoulda-context (2.0.0)
shoulda-matchers (4.5.1)
activesupport (>= 4.2.0)
sidekiq (5.2.9)
connection_pool (~> 2.2, >= 2.2.2)
rack (~> 2.0)
Expand Down Expand Up @@ -2108,9 +2108,9 @@ DEPENDENCIES
sass-rails (~> 5.0)
secure_headers (~> 6.3.0)
selenium-webdriver (~> 3.142)
shoulda (~> 3.5.0)
shoulda-context (~> 1.2.2)
shoulda-matchers (~> 2.8.0)
shoulda (~> 4.0.0)
shoulda-context (~> 2.0.0)
shoulda-matchers (~> 4.5.1)
sidekiq (< 6)
sidekiq-batch (~> 0.1.6)
sidekiq-cron
Expand Down
7 changes: 7 additions & 0 deletions spec/rails_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,3 +146,10 @@
config.curl_host = 'http://localhost:3000'
config.api_name = "Example App API"
end

Shoulda::Matchers.configure do |config|
config.integrate do |with|
with.test_framework :rspec
with.library :rails
end
end
7 changes: 5 additions & 2 deletions test/functional/partners/providers_controller_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

class Partners::ProvidersControllerTest < ActionController::TestCase

should route(:post, "http://#{master_account.domain}/partners/providers").to(action: 'create', format: :json)

def setup
@request.host = master_account.domain
@partner = FactoryBot.create(:partner, system_name: 'someone')
Expand All @@ -23,6 +21,11 @@ def provider_params
{subdomain: 'troloro', org_name: 'foo-org', email: 'foo@example.net', first_name: 'Tyler', last_name: 'Durden', api_key: @partner.api_key, open_id: "openid"}
end

test 'should route partners/providers to correct controller' do
assert_routing({ method: :post, path: "http://#{master_account.domain}/partners/providers" },
{ action: 'create', format: 'json', controller: 'partners/providers' })
end

test 'required api_key' do
post :create
assert_response :unauthorized
Expand Down
20 changes: 20 additions & 0 deletions test/monkey_patches/route_params.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# frozen_string_literal: true

# TODO: remove when you see similar error but actual format is: "format"=>:json
# PARAMS_TO_SYMBOLIZE causes errors in the tests regarding the format.
# It always converts format to symbol which causes a failure in the tests:
# Stats::Api::ApplicationsController should route GET /stats/api/applications/42/usage.json
# --- expected
# +++ actual
# @@ -1 +1 @@
# -{"application_id"=>"42", "action"=>"usage", "format"=>:json, "controller"=>"stats/api/applications"}
# +{"controller"=>"stats/api/applications", "action"=>"usage", "application_id"=>"42", "format"=>"json"}
module Shoulda
module Matchers
module ActionController
class RouteParams
PARAMS_TO_SYMBOLIZE = [].freeze
end
end
end
end
Comment on lines +1 to +20
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure how to fix it on the other side, somehow the tests fail because shoulda-matchers is symbolizing format values and it crashes our tests. Our controllers are returning format as a string then tests like https://github.com/3scale/porta/blob/master/test/functional/stats/api/applications_controller_test.rb#L7 fails.

I am not unable to debug it ( it will fail with other [IO] issues). There is no stack trace and also the test result is not helpful at all.

Example test result:

# Stats::Api::ApplicationsController should route GET /stats/api/applications/42/usage.json
# --- expected
# +++ actual
# @@ -1 +1 @@
# -{"application_id"=>"42", "action"=>"usage", "format"=>:json, "controller"=>"stats/api/applications"}
# +{"controller"=>"stats/api/applications", "action"=>"usage", "application_id"=>"42", "format"=>"json"}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this issue comment
thoughtbot/shoulda-matchers#809 (comment)

Perhaps we need to fix in routes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't do this in our case. We are using one route for multiple formats and default won't work.
One of the tests that are failing because of this: https://github.com/3scale/porta/blob/master/test/functional/stats/api/services_controller_test.rb#L7

On the other hand, when the tests are checking the controller they are parsing the path and the path contains the format which is parsed as string, not a symbol.
https://github.com/rails/rails/blob/v5.1.7/actionpack/lib/action_dispatch/routing/route_set.rb#L864

8 changes: 8 additions & 0 deletions test/test_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
WebMock.disable_net_connect!

require 'monkey_patches/active_job_test_helper'
require 'monkey_patches/route_params'
class ActiveSupport::TestCase
self.use_transactional_tests = true
self.use_instantiated_fixtures = false
Expand Down Expand Up @@ -86,3 +87,10 @@ def teardown
include TestHelpers::SectionsPermissions

ActiveJob::Uniqueness.test_mode!

Shoulda::Matchers.configure do |config|
config.integrate do |with|
with.test_framework :minitest
with.library :rails
end
end