rspec / rspec-rails

RSpec for Rails 7+
https://rspec.info
MIT License
5.19k stars 1.04k forks source link

Use headless driver for next Rails release #2746

Closed stevepolitodesign closed 8 months ago

stevepolitodesign commented 8 months ago

In the next release of Rails, the default driver was switched from :chrome to :headless_chrome as see in: https://github.com/rails/rails/pull/50512 This is to ensure the new ci template will "work out of the box".

However, this will not work with applications using rspec-rails, since it still defaults to :selenium. Instead, GitHub actions will fail with the following error:

Selenium::WebDriver::Error::SessionNotCreatedError:
            session not created: Chrome failed to start: exited normally.
              (session not created: DevToolsActivePort file doesn't exist)
              (The process started from chrome location /usr/bin/google-chrome is no longer running, so ChromeDriver is assuming that Chrome has crashed.)

I've created a demo application to highlight the problem, and test the proposed solution. The commit history aims to break everything out change by change, but I will highlight the important parts below.

  1. This commit replicates the bug. Here's the corresponding failure.
  2. Without changing the test, I simply update the Gemfile to use this branch, resulting in a passing test.
stevepolitodesign commented 8 months ago

Just pointing out that https://github.com/rails/rails/pull/51289 will remove the test job altogether. However, I think it's still important we keep parity with Rails.

JonRowe commented 8 months ago

:wave: thanks!, can you update the spec to match?

stevepolitodesign commented 8 months ago

@JonRowe I made a change in f122744 in an attempt to fix CI, but Rails main is currently set to 7.2.0.alpha. However, ci.yml uses 'main' for the RAILS_VERSION.

stevepolitodesign commented 8 months ago

I think my imlpementation is correct, since CI is passing with my demo application. Does ci.yml need to change instead? Something like:

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index f54e46d7..92453045 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -39,10 +39,10 @@ jobs:
          # Edge Rails (?) builds >= 2.7
          - ruby: 3.2
            env:
-             RAILS_VERSION: 'main'
+             RAILS_VERSION: '~> 7.2.0'
          - ruby: 3.1
            env:
-             RAILS_VERSION: 'main'
+             RAILS_VERSION: '~> 7.2.0'

          # Rails 7.1 builds >= 2.7
          - ruby: 3.2
JonRowe commented 8 months ago

The version specified in our ci.yml triggers fetching Rails from git, it doesn't affect the version string of Rails exposed to rspec, your original check was correct but it changed the default and theres a spec that checks it: spec/rspec/rails/example/system_example_group_spec.rb:33 what I meant was to conditionally define that and add the second spec like you have.

stevepolitodesign commented 8 months ago

@JonRowe thank you for the additional context. I was surprised by the original failure since I figured the default would not change because of the conditional in the code. Do I need to do something like this?

Is there a way for me to test this locally?

diff --git a/spec/rspec/rails/example/system_example_group_spec.rb b/spec/rspec/rails/example/system_example_group_spec.rb
index a9b9ff83..1a19d932 100644
--- a/spec/rspec/rails/example/system_example_group_spec.rb
+++ b/spec/rspec/rails/example/system_example_group_spec.rb
@@ -29,7 +29,7 @@ module RSpec::Rails
       end
     end

-    describe '#driver' do
+    describe '#driver', if: ::Rails::VERSION::STRING.to_f < 7.2 do
       it 'uses :selenium driver by default' do
         group = RSpec::Core::ExampleGroup.describe do
           include SystemExampleGroup
JonRowe commented 8 months ago

I was surprised by the original failure since I figured the default would not change because of the conditional in the code.

It failed our Rails main build because we check for the specific value which you changed

stevepolitodesign commented 8 months ago

@JonRowe let me know if my latest change is what you expected. My confusion lies in the fact that I would have expected there to be two specs:

However, using #take_screenshot as an example, it looks like it's only tested once, and with the conditional.

JonRowe commented 8 months ago

Thanks!

JonRowe commented 2 months ago

Released in 7.0.0