Skip to content

Extract cell_selector method in scaffold index view spec generator #2777

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

Merged
merged 2 commits into from
Aug 10, 2024
Merged

Extract cell_selector method in scaffold index view spec generator #2777

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5036997
Move conditional `cell_selector` out of generated view spec
mjankowski Aug 8, 2024
f785f7a
Add changelog entry for #2777
pirj Aug 10, 2024
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
1 change: 1 addition & 0 deletions Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ Enhancements:
* Verify ActiveJob arguments by comparing to the method signature. (Oli Peate, #2745)
* Add suggestion to rails_helper.rb to skip when not in test most. (Glauco Custódio, #2751)
* Add `at_priority` qualifier to `have_enqueued_job` set of matchers. (mbajur, #2759)
* Remove Rails version-specific conditional from index scaffold generation. (Matt Jankowski, #2777)

Bug Fixes:

Expand Down
2 changes: 1 addition & 1 deletion lib/generators/rspec/scaffold/templates/index_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
it "renders a list of <%= ns_table_name %>" do
render
cell_selector = Rails::VERSION::STRING >= '7' ? 'div>p' : 'tr>td'
cell_selector = <%= Rails::VERSION::STRING >= '7' ? "'div>p'" : "'tr>td'" %>
<% for attribute in output_attributes -%>
assert_select cell_selector, text: Regexp.new(<%= value_for(attribute) %>.to_s), count: 2
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I don't see why this needs to be a method, I would just do:

Suggested change
assert_select cell_selector, text: Regexp.new(<%= value_for(attribute) %>.to_s), count: 2
assert_select <%= Rails::VERSION::STRING >= '7' ? "'div>p'" : "'tr>td'" %>, text: Regexp.new(<%= value_for(attribute) %>.to_s), count: 2

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 may have misinterpreted your comment in the linked issue proposing we add a method (I assumed you meant in the generated view spec ... maybe you meant an internal method in the scaffold class that holds this logic and generates the string into the view?)

In either case, there are some other specs which reference cell_selector directly ... they keep passing if we turn the local var into a method in the generated view spec, but they'd fail if we change to direct string output here (could be solved by also having this conditional logic inline in the scaffold spec, and/or adding a method like this with duplicated logic to the spec).

I chose the smallest-diff path, but could do another pass to keep it as just a generated string in the view spec output, with conditional logic either in a shared method (template file and scaffold spec) or repeated in those spots.

Relatedly ... another thing which will wind up fixing this issue is whenever Rails 6.1 is EOL, there will presumably be a sweep/cleanup rspec-rails to remove a bunch of this logic, drop old ruby/rails support, etc?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mind this adjustment? It warrants an insta-merge.
Another alternative that's a minimal diff and an insta-merge would be:

- cell_selector = Rails::VERSION::STRING >= '7' ? 'div>p' : 'tr>td'
+ cell_selector = <%= Rails::VERSION::STRING >= '7' ? "'div>p'" : "'tr>td'" %>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use that latter approach (leave cell_selector as local var) to reduce the number of other specs that need changing.

Copy link
Member

Choose a reason for hiding this comment

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

For clarity / future travellers, I originally meant a method in the generator not the spec, but this is fine as is 👍

<% end -%>
Expand Down
6 changes: 6 additions & 0 deletions spec/generators/rspec/scaffold/scaffold_generator_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,6 +268,12 @@
.and(contain(/^RSpec.describe "(.*)\/index", #{type_metatag(:view)}/))
.and(contain(/assign\(:posts, /))
.and(contain(/it "renders a list of (.*)"/))

if ::Rails::VERSION::STRING >= '7.0.0'
expect(filename).to contain(/'div>p'/)
else
expect(filename).to contain(/'tr>td'/)
end
end
end

Expand Down