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

Jump to bottom

Favor request specs over controller specs when generating a controller #2222

Merged
merged 4 commits into from Dec 20, 2019
Merged

Favor request specs over controller specs when generating a controller #2222

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7b337f4
Favor request specs over controller specs for rails g controller
klyonrad Dec 7, 2019
b9203c8
Make content of automatic request spec almost empty
klyonrad Dec 7, 2019
8107697
Remove rails version checks for controller generator
klyonrad Dec 15, 2019
5614581
Generate request specs for the provided actions
klyonrad Dec 15, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/generators/rspec/controller/controller_generator.rb
View file
Open in desktop
Expand Up @@ -7,10 +7,18 @@ class ControllerGenerator < Base
argument :actions, type: :array, default: [], banner: "action action"

class_option :template_engine, desc: "Template engine to generate view files"
class_option :controller_specs, type: :boolean, default: true, desc: "Generate controller specs"
class_option :request_specs, type: :boolean, default: true, desc: "Generate request specs"
class_option :controller_specs, type: :boolean, default: false, desc: "Generate controller specs"
class_option :view_specs, type: :boolean, default: true, desc: "Generate view specs"
class_option :routing_specs, type: :boolean, default: false, desc: "Generate routing specs"

def generate_request_spec
return unless options[:request_specs]

template 'request_spec.rb',
File.join('spec/requests', class_path, "#{file_name}_request_spec.rb")
end

def generate_controller_spec
return unless options[:controller_specs]

Expand Down
14 changes: 14 additions & 0 deletions lib/generators/rspec/controller/templates/request_spec.rb
View file
Open in desktop
@@ -0,0 +1,14 @@
require 'rails_helper'

RSpec.describe "<%= class_name.pluralize %>", <%= type_metatag(:request) %> do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copied over from lib/generators/rspec/integration/templates/request_spec.rb

I didn't try "very hard" to use the same template file because I am not sure if that is even wished for

Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend writing a spec for get name etc that returns success or something similarly basic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, figuring the details for the templates out was/is a bit difficult for me. I am sure, I would figure it out - but can you clarify the specs a bit maybe? I mean, what does the rspec-rails team think should be the convention for request specs that test a controller

Copy link
Member

Choose a reason for hiding this comment

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

Does the request spec guide help you? https://relishapp.com/rspec/rspec-rails/docs/request-specs/request-spec

Copy link
Member

Choose a reason for hiding this comment

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

I mean, what does the rspec-rails team think should be the convention for request specs that test a controller

I don't use generators myself as I find that the similar lines tend to just be the naming only. There isn't that much of a semantic difference between a controller spec and a request spec, its the level of stack integration that differs. Personally I would be happy with:

describe "GET /<%= name.underscore.pluralize %>" do
  it "works! (now write some real specs)" do
    get "/<%= name.underscore.pluralize %>"
    expect(response).to be_success
  end
end

However finesse wise I think that name.underscore.pluralize is wrong here, because it needs to be name spaced (or not) appropriately.

Copy link
Member

Choose a reason for hiding this comment

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

Let me know if you want any more feedback

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 went with doing the same thing like the existing controller spec templates

If I understand the rails controller generator code correctly
it does note generate any routes unless we gave actions as a parameter. So the lib/generators/rspec/controller/templates/controller_spec.rb template makes a very empty controller spec

require 'rails_helper'

RSpec.describe Admin::CustomersController, type: :controller do

end

So when the generator is generated without action names it should not have a test. Otherwise that test would not be green

<% namespaced_path = regular_class_path.join('/') %>
<% for action in actions -%>
describe "GET /<%= action %>" do
it "returns http success" do
get "<%= "/#{namespaced_path}" if namespaced_path != '' %>/<%= file_name %>/<%= action %>"
expect(response).to have_http_status(:success)
end
end

<% end -%>
end
63 changes: 58 additions & 5 deletions spec/generators/rspec/controller/controller_generator_spec.rb
View file
Open in desktop
Expand Up @@ -5,25 +5,51 @@
RSpec.describe Rspec::Generators::ControllerGenerator, :type => :generator do
setup_default_destination

describe 'controller specs' do
subject { file('spec/controllers/posts_controller_spec.rb') }
describe 'request specs' do
subject { file('spec/requests/posts_request_spec.rb') }

describe 'generated by default' do
before do
run_generator %w(posts)
run_generator %w[posts]
end

describe 'the spec' do
it { is_expected.to exist }
it { is_expected.to contain(/require 'rails_helper'/) }
it { is_expected.to contain(/^RSpec.describe PostsController, #{type_metatag(:controller)}/) }
it { is_expected.to contain(/^RSpec.describe "Posts", #{type_metatag(:request)}/) }
end
end

describe 'skipped with a flag' do
before do
run_generator %w(posts --no-controller_specs)
run_generator %w[posts --no-request_specs]
end
it { is_expected.not_to exist }
end


describe 'with actions' do
before do
run_generator %w[posts index custom_action]
end

it { is_expected.to exist }
it { is_expected.to contain('get "/posts/index"') }
it { is_expected.to contain('get "/posts/custom_action"') }
end

describe 'with namespace and actions' do
subject { file('spec/requests/admin/external/users_request_spec.rb') }

before do
run_generator %w[admin::external::users index custom_action]
end

it { is_expected.to exist }
it { is_expected.to contain(/^RSpec.describe "Admin::External::Users", #{type_metatag(:request)}/) }
it { is_expected.to contain('get "/admin/external/users/index"') }
it { is_expected.to contain('get "/admin/external/users/custom_action"') }
end
end

describe 'view specs' do
Expand Down Expand Up @@ -127,4 +153,31 @@
it { is_expected.not_to exist }
end
end

describe 'controller specs' do
subject { file('spec/controllers/posts_controller_spec.rb') }

describe 'are not generated' do
it { is_expected.not_to exist }
end

describe 'with --controller-specs flag' do
before do
run_generator %w[posts --controller-specs]
end

describe 'the spec' do
it { is_expected.to exist }
it { is_expected.to contain(/require 'rails_helper'/) }
it { is_expected.to contain(/^RSpec.describe PostsController, #{type_metatag(:controller)}/) }
end
end

describe 'with --no-controller_specs flag' do
before do
run_generator %w[posts --no-controller-specs]
end
it { is_expected.not_to exist }
end
end
end