Skip to content

use the tag itself instead of the variable #2619

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

Closed
wants to merge 2 commits into from

Conversation

JuanVqz
Copy link

@JuanVqz JuanVqz commented Sep 15, 2022

while testing rails 7 I did notice the following test has a variable called cell_selector then I asked my self why do we need to know what rails version are we working on. could we generate only the tag without telling everybody how to do it?
and here we are, trying to implement it. if you think this is not useful please let me know.

  # before
  it "renders a list of orders" do
    render
    cell_selector = Rails::VERSION::STRING >= "7" ? "div>p" : "tr>td"
    assert_select cell_selector, text: Regexp.new("MyText".to_s), count: 2
  end
  
  # after, >= rails 7
  it "renders a list of orders" do
      render

      assert_select "div>p", text: Regexp.new("MyText"), count: 2
  end

  # after <= rails 6 
  it "renders a list of orders" do
    render

    assert_select "tr>td", text: Regexp.new("MyText".to_s), count: 2
  end

@@ -18,9 +18,10 @@

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.to_f >= 7.0 ? 'div>p' : 'tr>td' -%>
Copy link
Author

Choose a reason for hiding this comment

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

I was wondering if we need the parent Rails version

Suggested change
<% cell_selector = Rails::VERSION::STRING.to_f >= 7.0 ? 'div>p' : 'tr>td' -%>
<% cell_selector = ::Rails::VERSION::STRING.to_f >= 7.0 ? 'div>p' : 'tr>td' -%>

@pirj
Copy link
Member

pirj commented Sep 15, 2022

why do we need to know what rails version are we working on

There's a lengthy discussion here.

An alternative approach like you suggest is to bake this in into the generator.
There's a case when you've generated specs on Rails 6, and then switched to Rails 7, and a some other scenarios.
Which approach is more appealing to you, and why?

@JuanVqz
Copy link
Author

JuanVqz commented Sep 16, 2022

why do we need to know what rails version are we working on

There's a lengthy discussion here.

An alternative approach like you suggest is to bake this in into the generator.

There's a case when you've generated specs on Rails 6, and then switched to Rails 7, and a some other scenarios.

Which approach is more appealing to you, and why?

To be honest I don't think somebody will keep the scaffold as it is, you will ended up removing it.
I'll close the PR, thank you for sharing the info. 👍

@JuanVqz JuanVqz closed this Sep 16, 2022
@JonRowe
Copy link
Member

JonRowe commented Sep 18, 2022

I think this is reasonable tbh, or maybe this information should some how be baked into a method which the test uses, I didn't realise we were generating if rails_version in specs, that information is definietly intended to be invisible to developers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants