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

Use headless driver for next Rails release #2746

Merged
merged 6 commits into from Mar 19, 2024

Conversation

stevepolitodesign
Copy link
Contributor

In the next release of Rails, the default driver was switched from :chrome to :headless_chrome as see in: rails/rails#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.

In the next release of Rails, the default driver was switched from
`:chrome` to `:headless_chrome` as see in: rails/rails#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.)
```

[ci template]: https://github.com/rails/rails/blob/main/railties/lib/rails/generators/rails/app/templates/github/ci.yml.tt
@stevepolitodesign
Copy link
Contributor Author

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

@JonRowe
Copy link
Member

JonRowe commented Mar 18, 2024

👋 thanks!, can you update the spec to match?

Rails `main` is [currently][] set to `7.2.0.alpha`. However, `ci.yml`
uses `'main'` for the `RAILS_VERSION`.

[currently]: https://github.com/rails/rails/blob/5ab13c5a7798c2a9f96bebb1c68285dfb842d4f9/RAILS_VERSION
@stevepolitodesign
Copy link
Contributor Author

@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
Copy link
Contributor Author

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
Copy link
Member

JonRowe commented Mar 19, 2024

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
Copy link
Contributor Author

stevepolitodesign commented Mar 19, 2024

@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
Copy link
Member

JonRowe commented Mar 19, 2024

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
Copy link
Contributor Author

@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:

  • The existing #driver spec the ensures itss set to :selenium
  • A new #driver spec ensuring it's set to :selenium_chrome_headless if ::Rails::VERSION::STRING.to_f >= 7.2

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

@JonRowe JonRowe merged commit c2a7b82 into rspec:main Mar 19, 2024
17 checks passed
JonRowe added a commit that referenced this pull request Mar 19, 2024
@JonRowe
Copy link
Member

JonRowe commented Mar 19, 2024

Thanks!

@stevepolitodesign stevepolitodesign deleted the update-default-driver branch March 19, 2024 23:33
stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request Mar 21, 2024
Follow-up to #1156

Creates parity with Rails' decision to [use a headless driver by
default][headless].

This will be fixed in an [upcoming release][rspec] of rspec-rails, but I
felt it was important to capture here. Additionally, it ensures the
`screen_size` is the same as what is set in Rails.

Removes `webdrivers` dependency in favor of `selenium-webdriver`. This
generator assumes the app was generated with the `--skip-test`, which
means we need to add the `selenium-webdriver` and `capybara` gems.

Updates `action_dispatch-testing-integration-capybara` dependency to the
most recent tagged release in an effort to suppress Dependabot
notifications.

[headless]:drive://github.com/rails/rails/pull/50512
[rspec]: rspec/rspec-rails#2746
stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request Mar 21, 2024
Follow-up to #1156

Creates parity with Rails' decision to [use a headless driver by
default][headless].

This will be fixed in an [upcoming release][rspec] of rspec-rails, but I
felt it was important to capture here. Additionally, it ensures the
`screen_size` is the same as what is set in Rails.

Removes `webdrivers` dependency in favor of `selenium-webdriver`. This
generator assumes the app was generated with the `--skip-test`, which
means we need to add the `selenium-webdriver` and `capybara` gems.

Updates `action_dispatch-testing-integration-capybara` dependency to the
most recent tagged release in an effort to suppress Dependabot
notifications.

Ensure all files under `spec/support` are loaded by uncommenting a line
generated by the RSpec installation script.

[headless]:drive://github.com/rails/rails/pull/50512
[rspec]: rspec/rspec-rails#2746
stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request Mar 21, 2024
Follow-up to #1156

Creates parity with Rails' decision to [use a headless driver by
default][headless].

This will be fixed in an [upcoming release][rspec] of rspec-rails, but I
felt it was important to capture here. Additionally, it ensures the
`screen_size` is the same as what is set in Rails.

Removes `webdrivers` dependency in favor of `selenium-webdriver`. This
generator assumes the app was generated with the `--skip-test`, which
means we need to add the `selenium-webdriver` and `capybara` gems.

Updates `action_dispatch-testing-integration-capybara` dependency to the
most recent tagged release in an effort to suppress Dependabot
notifications.

Ensure all files under `spec/support` are loaded by uncommenting a line
generated by the RSpec installation script.

[headless]: drive://github.com/rails/rails/pull/50512
[rspec]: rspec/rspec-rails#2746
stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request Mar 21, 2024
Follow-up to #1156

Creates parity with Rails' decision to [use a headless driver by
default][headless].

This will be fixed in an [upcoming release][rspec] of rspec-rails, but I
felt it was important to capture here. Additionally, it ensures the
`screen_size` is the same as what is set in Rails.

Removes `webdrivers` dependency in favor of `selenium-webdriver`. This
generator assumes the app was generated with the `--skip-test`, which
means we need to add the `selenium-webdriver` and `capybara` gems.

Updates `action_dispatch-testing-integration-capybara` dependency to the
most recent tagged release in an effort to suppress Dependabot
notifications.

Ensure all files under `spec/support` are loaded by uncommenting a line
generated by the RSpec installation script.

[headless]: rails/rails#50512
[rspec]: rspec/rspec-rails#2746
stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request Mar 21, 2024
Follow-up to #1156

Creates parity with Rails' decision to [use a headless driver by
default][headless].

This will be fixed in an [upcoming release][rspec] of rspec-rails, but I
felt it was important to capture here. Additionally, it ensures the
`screen_size` is the same as what is set in Rails.

Removes `webdrivers` dependency in favor of `selenium-webdriver`. This
generator assumes the app was generated with the `--skip-test` flag,
which means we need to add the `selenium-webdriver` and `capybara` gems.

Updates `action_dispatch-testing-integration-capybara` dependency to the
most recent tagged release in an effort to suppress Dependabot
notifications.

Ensure all files under `spec/support` are loaded by uncommenting a line
generated by the RSpec installation script.

[headless]: rails/rails#50512
[rspec]: rspec/rspec-rails#2746
stevepolitodesign added a commit to thoughtbot/suspenders that referenced this pull request Mar 21, 2024
Follow-up to #1156

Creates parity with Rails' decision to [use a headless driver by
default][headless].

This will be fixed in an [upcoming release][rspec] of rspec-rails, but I
felt it was important to capture here. Additionally, it ensures the
`screen_size` is the same as what is set in Rails.

Removes `webdrivers` dependency in favor of `selenium-webdriver`. This
generator assumes the app was generated with the `--skip-test` flag,
which means we need to add the `selenium-webdriver` and `capybara` gems.

Updates `action_dispatch-testing-integration-capybara` dependency to the
most recent tagged release in an effort to suppress Dependabot
notifications.

Ensure all files under `spec/support` are loaded by uncommenting a line
generated by the RSpec installation script.

[headless]: rails/rails#50512
[rspec]: rspec/rspec-rails#2746
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.

None yet

2 participants