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

Improve controller template when no action #2399

Merged
merged 4 commits into from
Oct 31, 2020
Merged

Improve controller template when no action #2399

merged 4 commits into from
Oct 31, 2020

Conversation

ThHareau
Copy link
Contributor

Fix for #2375

A generated file looks like this:

require 'rails_helper'

RSpec.describe "Posts", type: :request do
  describe "GET /index" do
    it "returns http success" do
      pending "add some scenarios (or delete) #{__FILE__}"
    end
  end
end

@ThHareau
Copy link
Contributor Author

Made with @tsleal29, @yann120, @fwuensche and @musait

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Amazing, thanks!
Just a few edits away from getting it done.

lib/generators/rspec/controller/controller_generator.rb Outdated Show resolved Hide resolved
lib/generators/rspec/controller/templates/request_spec.rb Outdated Show resolved Hide resolved
lib/generators/rspec/controller/templates/request_spec.rb Outdated Show resolved Hide resolved
<% namespaced_path = regular_class_path.join('/') -%>
<% if actions.empty? -%>
describe "GET /index" do
it "returns http success" do
Copy link
Member

Choose a reason for hiding this comment

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

Not specifically related to this pull request.

I'm slightly opposed to adding "returns http success" in request specs. From my point of view, they should test more than just http statuses, and providing this as a reference example in the template is promoting a dubious practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed. Maybe I can update the template for that as well? Do you have an idea regarding what we can provide?

I'm thinking to exists or is reachable (I have a preference for the second option)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe "returns a list of #{class_name.pluralize.downcase}"? At least this is what an "index" originally means.

Copy link
Member

Choose a reason for hiding this comment

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

update the template for that as well

That would be very helpful!

Copy link
Member

@benoittgt benoittgt left a comment

Choose a reason for hiding this comment

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

CI errors are on JRuby and unreleated. Your patch looks good to me. Thanks @pirj for the review :)

Thanks @ThHareau, @tsleal29, @yann120, @fwuensche and @musait

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thanks!

@pirj pirj merged commit ea09924 into rspec:main Oct 31, 2020
JonRowe added a commit that referenced this pull request Nov 1, 2020
benoittgt pushed a commit that referenced this pull request Nov 3, 2020
JonRowe pushed a commit that referenced this pull request Nov 26, 2020
Improve controller template when no action
JonRowe added a commit that referenced this pull request Nov 26, 2020
This was referenced Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants