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

Add Rails 7.1 builds #2692

Merged
merged 3 commits into from
Oct 20, 2023
Merged

Add Rails 7.1 builds #2692

merged 3 commits into from
Oct 20, 2023

Conversation

JonRowe
Copy link
Member

@JonRowe JonRowe commented Oct 6, 2023

No description provided.

@pirj
Copy link
Member

pirj commented Oct 7, 2023

For Rails 6.1 builds:

webdrivers 5.3.1 is installed for Ruby 2.6/2.7 and the build is green, but webdrivers 5.2.0 for Ruby 3.0/3.1 and those builds fail.

Problems are due to this:

If you are using an older version of webdrivers gem, and cannot upgrade, you can set the required version of chromedriver to v114 (Webdrivers::Chromedriver.required_version = '114.0.5735.90')

Where "older version" is 5.2.0, and "cannot upgrade" only affects Ruby 2.5, where webdrivers 4.x is installed.

At a glance, I can't see why 5.3.1 is not installed for all 2.6+ Ruby versions.

@taketo1113
Copy link
Contributor

At a glance, I can't see why 5.3.1 is not installed for all 2.6+ Ruby versions.

It is dependency of a selenium-webdriver gem.

For Ruby 3.0+, it installs selenium-webdriver v4.13.1.
It can not install webdriver v5.3.1 and selenium-webdriver v4.13.1, because webdriver v5.3.1 has a dependency of 'selenium-webdriver', '~> 4.0', '< 4.11'.
Thus it is installed webdriver v5.2.0 and selenium-webdriver v4.13.1.

https://github.com/SeleniumHQ/selenium/blob/selenium-4.13.1-ruby/rb/selenium-webdriver.gemspec#L33
https://github.com/titusfortner/webdrivers/blob/v5.3.1/webdrivers.gemspec#L41

For Ruby 2.6/2.7, it installs selenium-webdriver v4.9.0, because selenium-webdriver v4.10.0+ required ruby version 3.0+.
It can install webdriver v5.3.1 with selenium-webdriver v4.9.0.

https://github.com/SeleniumHQ/selenium/blob/selenium-4.9.0/rb/selenium-webdriver.gemspec#L33

@taketo1113
Copy link
Contributor

I think it's better to remove the webdrivers gem dependency in Ruby 3.0+.

# https://github.com/rspec/rspec-rails/blob/main/example_app_generator/generate_app.rb#L34
-  gsub_file "Gemfile", /.*chromedriver-helper.*/, "gem 'webdrivers'"
+  if RUBY_VERSION < "3.0"
+    gsub_file "Gemfile", /.*chromedriver-helper.*/, "gem 'webdrivers'"
+  end

Because Rails omit webdrivers gem from Gemfile template in Ruby 3.0+.
https://github.com/rails/rails/blob/main/railties/lib/rails/generators/rails/app/templates/Gemfile.tt#L64-L66
rails/rails#48847

@JonRowe JonRowe force-pushed the add-rails-71-directly branch 4 times, most recently from 5809669 to 2d06e09 Compare October 12, 2023 20:30
@pirj pirj mentioned this pull request Oct 13, 2023
@pirj
Copy link
Member

pirj commented Oct 14, 2023

I suggest skipping features on Ruby 2.5

@pirj
Copy link
Member

pirj commented Oct 14, 2023

For 7.1:
‘’’
Failure/Error: require 'generators/rspec'
3204

3205
NameError:
3206
uninitialized constant Rails::Generators::Actions
‘’’

@JonRowe
Copy link
Member Author

JonRowe commented Oct 14, 2023

Only Rails 7 dropped webdriver, we still need it on 6.1 @pirj, also would you mind making your changes on your own branch / PR either to this branch or seperate.

@JonRowe JonRowe force-pushed the add-rails-71-directly branch 3 times, most recently from d0e3089 to 1a8cc68 Compare October 14, 2023 09:44
@JonRowe
Copy link
Member Author

JonRowe commented Oct 14, 2023

@jcoyne 's webdriver removal actually weirdly works on 6.1 so I've picked that across with the skips for the broken specs which I'll work on seperately later to see if we can get closer to passing hre.

@JonRowe
Copy link
Member Author

JonRowe commented Oct 14, 2023

@chaadow This PR is about getting our CI up and running, please use the mailing list if you have problems you want to discuss or open a bug report if you think you've found an issue but remember that rspec-rails is a thin wrapper around Rails test helpers, if they don't recognise a route anymore you'll need to change.

@chaadow
Copy link

chaadow commented Oct 14, 2023

@JonRowe yes, when i updated from rspec-rails 6.0.0 to 6.0.3, the issue was solved thanks!

@chaadow
Copy link

chaadow commented Oct 14, 2023

It was fixed here bd2bb24

Also I thought since the builds were failing here and i'm in the process of upgrading to rails 7.1 and had issues with rspec, that maybe the issue I have might be related to rails 7.1 👍🏼

@pirj
Copy link
Member

pirj commented Oct 14, 2023

Only Rails 7 dropped webdriver

7.1 did. rails/rails@9a53234
I didn’t look if we have failures of Rails 7.0 on 3.0/3.1, it’s probably because of this.

It’s not that Rails 7.1 dropped webdriver and we are forced to use webdrivers for rails 6.1/7.0.
Existing projects running on Rails 6.1 face the same issue, and they either have to drop webdrivers gem (if they are on Ruby 3+), or use webdrivers 5.3.1 if on Ruby 2.6/2.7.

I’m unaware of a reliable solution for Rails 6.1 running on Ruby 2.5 to run browser specs.

We have to follow the same practical approach in our test suite.

I believe that my commit fixed features for Rails 6.1/7.0 running on Ruby 2.6+. I’ll be happy to send a separate PR, or @jcoyne you probably could accomodate those changes? I’m from my phone this weekend.

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.

LGTM!
Please disregard my prior message.
Let’s deal with 7.1 failures and broken mailer specs separately?
Thanks!

@JonRowe
Copy link
Member Author

JonRowe commented Oct 14, 2023

The fixes for other builds has been merged as #2700 I'm continuing work on the 7.1 build here

@JonRowe JonRowe force-pushed the add-rails-71-directly branch 3 times, most recently from 2dde28c to cf7d3a2 Compare October 15, 2023 08:26
@JonRowe JonRowe force-pushed the add-rails-71-directly branch 6 times, most recently from 5b5f9aa to 39d9fa0 Compare October 20, 2023 09:11
@JonRowe
Copy link
Member Author

JonRowe commented Oct 20, 2023

Final failure was a zeitwerk exception with our generator, I've surpressed this by ignoring the file after much faffing around trying to fix the issue, it doesn't happen locally for me (but did show up in zeitwerks check) so this was a pain to debug (and is hidden in the builds by the frozen array non error).

@JonRowe JonRowe merged commit 9681266 into main Oct 20, 2023
19 checks passed
@JonRowe JonRowe deleted the add-rails-71-directly branch October 20, 2023 09:43
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.

4 participants