Extract cell_selector method in scaffold index view spec generator
#2777
Conversation
| @@ -18,9 +18,12 @@ | |||
|
|
|||
| it "renders a list of <%= ns_table_name %>" do | |||
| render | |||
| 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 | |||
There was a problem hiding this comment.
Personally, I don't see why this needs to be a method, I would just do:
| 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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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'" %>There was a problem hiding this comment.
Updated to use that latter approach (leave cell_selector as local var) to reduce the number of other specs that need changing.
There was a problem hiding this comment.
For clarity / future travellers, I originally meant a method in the generator not the spec, but this is fine as is 👍
mjankowski
force-pushed
the
cell-selector-generate-values
branch
from
5ce7e31 to
84d504e
Compare
pirj
force-pushed
the
cell-selector-generate-values
branch
from
f478710 to
f785f7a
Compare
|
Thank you, @mjankowski ! |
…2777) Move conditional `cell_selector` out of generated view spec
|
Released in 6.1.4 |
Basically same change as described in #2619 - re-attempting only because of last comment - #2619 (comment) - which seemed to indicate a desire to get the logic into the generator and out of the generated spec file.
I read the linked discussion and it wasn't totally clear to me which portions involved people talking slightly past each other (about CI vs developer env) and which were genuine issues (that I may have not resolved here, if they exist?) so apologies if that's the case, and I can close and/or attempt to resolve. From my current understanding this does seem safe -- in CI environments or dev environments for working on rspec-rails itself, you'd presumably either have a fresh/clean run and/or clobber first and it should be ok; and for local rails-app development, if you had a prior-version generated spec+view they'd work in conjunction with each other, and this would continue to be true if you switched versions and started generating new ones, I think?
I agree with one of the comments in the linked discussion about how you would likely change this value pretty early on, but also that having the rails version conditional in the generated spec is slightly confusing when you first see it.