Preload Selenium driver_path before parallelizing system tests #49908

mattbrictson · 2023-11-03T21:04:19Z

Motivation / Background

When the webdrivers gem is not present (which is the default scenario in Rails 7.1+), the Selenium driver_path starts out as nil. This means the driver is located lazily, and deferred until a system test is run.

If parallel testing is used, this leads to a race condition, where each worker process tries to resolve the driver simultaneously. The result is an error as described in #49906.

Detail

This commit fixes the race condition by changing the implementation of Browser#preload. The previous implementation worked when driver_path was set to a Proc by the webdrivers gem, but doesn't work when the webdrivers gem is not being used and the driver_path is nil.

If the driver_path is nil, Browser#preload will now use the DriverFinder utility provided by the selenium-webdriver gem to eagerly resolve the driver path. This ensures that driver_path is set before parallel test workers are forked.

Fixes #49906.

Additional information

This solution mimics the code within Selenium that determines the driver executable on demand:

service.executable_path ||= WebDriver::DriverFinder.path(options, service.class)

https://github.com/SeleniumHQ/selenium/blob/7680b7cf25579217cb62a0893c4b2b69c0186062/rb/lib/selenium/webdriver/common/local_driver.rb#L41

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
Tests are added or updated if you fix a bug or add a feature.
CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

actionpack/lib/action_dispatch/system_testing/browser.rb

jonathanhefner · 2023-11-06T17:21:19Z

actionpack/test/dispatch/system_testing/driver_test.rb

@@ -148,7 +148,7 @@ class DriverTest < ActiveSupport::TestCase
    end
  end

-  test "preloads browser's driver_path" do
+  test "preloads browser's driver_path if it is set to a Proc" do


If we drop service.driver_path.try(:call), I think we can drop this test too, because Rails would no longer be responsible for this behavior.

Thought it might be worth adding a test for when driver_path is set to an actual path (to verify that it is not overwritten).

Agreed. I'll drop the Proc test and add another test to verify an existing value is not overwritten.

Addressed in 916efae770 and 868599937a

jonathanhefner · 2023-11-06T17:21:51Z

actionpack/test/dispatch/system_testing/driver_test.rb

+    found_executable = RbConfig.ruby
+    ::Selenium::WebDriver::DriverFinder.stub(:path, found_executable) do
+      ActionDispatch::SystemTesting::Driver.new(:selenium, screen_size: [1400, 1400], using: :chrome)
+    end


I'm hesitant to stub DriverFinder.path since we're relying on it directly. Does this test blow up if we don't stub? Perhaps it would be slightly better to stub SeleniumManager.driver_path, which DriverFinder.path calls?

I used a stub to avoid a network call to download chromedriver, which would slow down the test and perhaps make it less reliable. However I agree that the test is more realistic and valuable without the stub. I'll remove it.

Addressed in be39f0c695

to avoid a network call to download chromedriver

Ah, right, in that case we should stub. (Sorry! 🙏) But I think stubbing SeleniumManager.driver_path might be a better choice because if the behavior of DriverFinder.path changes, then we will be more likely to catch it.

OK, addressed in 3f089b3aa1 ✅

mattbrictson · 2023-11-06T23:20:09Z

@jonathanhefner I've addressed your review comments and squashed down to 1 commit.

mattbrictson · 2023-11-07T04:58:34Z

@jonathanhefner thanks for your feedback and your patience. This is ready for another review.

When the webdrivers gem is not present (which is the default scenario in Rails 7.1+), the Selenium `driver_path` starts out as `nil`. This means the driver is located lazily, and deferred until a system test is run. If parallel testing is used, this leads to a race condition, where each worker process tries to resolve the driver simultaneously. The result is an error as described in rails#49906. This commit fixes the race condition by changing the implementation of `Browser#preload`. The previous implementation worked when `driver_path` was set to a Proc by the `webdrivers` gem, but doesn't work when the `webdrivers` gem is not being used and the `driver_path` is `nil`. `Browser#preload` now uses the `DriverFinder` utility provided by the `selenium-webdriver` gem to eagerly resolve the driver path if needed. This will ensures that `driver_path` is set before parallel test workers are forked. Fixes rails#49906. Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>

jonathanhefner · 2023-11-07T17:41:16Z

Looks good! Though I pushed a commit to change With help from @jonathanhefner in the commit message. When you put someone's GitHub username in a commit message, GitHub notifies them every time that commit message is pushed, e.g. when rebasing, cherry picking, etc. And there's no way to disable those notifications. (It is a major failing on GitHub's part.) So if you want to credit a co-author, you can add Co-authored-by: Name <email@example.com> to the end of the commit message. An easy way to get the appropriate name / email is to visit a PR or commit on GitHub that the person has authored and append ".patch" to the URL. For example, https://github.com/rails/rails/pull/49908.patch shows Matt Brictson <matt@mattbrictson.com>.

Thank you, @mattbrictson! 🏎️

Preload Selenium driver_path before parallelizing system tests

lazyatom · 2023-12-06T12:16:24Z

I think this might break system tests where the Chrome binary is not in the default path, e.g. where selenium-webdriver has downloaded a version of "Chrome for Testing" into a local cache. I'm investigating more and will report back when I have something concrete.

lazyatom · 2023-12-06T14:10:47Z

OK, here's what I think is happening. Deep in selenium-webdriver, the call to Selenium::WebDriver::SeleniumManager.driver_path has a side-effect of setting binary in the option Hash it has been passed as an argument. This Hash is eventually used to determine the "capabilities" to send to chromedriver, in particular the capabilities["goog:chromeOptions"]["binary"] value, which tells chromedriver to launch that binary instead of whatever's available in PATH.

With this change, we are setting Selenium::WebDriver::Chrome::Driver.driver_path in advance, which then prevents us from calling SeleniumManager.driver_path(options) later, resulting in the binary never being sent to chromedriver as part of the capabilities hash.

Is this a bug in Rails? Or an issue with selenium-webdriver relying on this side-effect to populate the binary option? Who is to say.

jonathanhefner · 2023-12-06T17:14:52Z

@lazyatom Thank you for investigating, but I'm not sure I follow. Why does SeleniumManager.driver_path produce a different result later? Could you provide a reproduction script or an example test setup?

lazyatom · 2023-12-06T20:36:24Z

@jonathanhefner thanks for replying. I've created a new issue (#50287) with a replication script and a hopefully-clearer description/explanation. The only thing I can't achieve with the script is making sure that you don't already have Chrome installed -- that's key to demonstrating this issue.

Follow-up to rails#49908. When Selenium resolves the driver path to a copy of Chrome that is has downloaded / cached, it mutates the `Selenium::WebDriver::Chrome::Options` object it receives, and relies on those changes later when the options are used. If `Selenium::WebDriver::Chrome::Service.driver_path` is set but a different options object is used, Selenium will raise "cannot find Chrome binary". Therefore, this commit ensures that the options object passed to `Selenium::WebDriver::DriverFinder.path` is the same options object used by the driver later.

Follow-up to rails#49908. When Selenium resolves the driver path to a copy of Chrome that it has downloaded / cached, it mutates the `Selenium::WebDriver::Chrome::Options` object it receives, and relies on those changes later when the options are used. If `Selenium::WebDriver::Chrome::Service.driver_path` is set but a different options object is used, Selenium will raise "cannot find Chrome binary". Therefore, this commit ensures that the options object passed to `Selenium::WebDriver::DriverFinder.path` is the same options object used by the driver later.

majkelcc · 2023-12-12T14:04:39Z

Hi @mattbrictson, it looks like the resolve_driver_path method is always trying to look for a local binary, which breaks containerized setups using remote browsers. Can you confirm it was missed, should I create a bug report? Everything was working fine in Rails 7.1.1.

Code example that's failing:

class ApplicationSystemTestCase < ActionDispatch::SystemTestCase
  driven_by :selenium,
    using: :headless_chrome,
    options: {
      url: "http://chrome:4444",
      browser: :remote
    }
  end
end

With this ::Selenium::WebDriver::DriverFinder.path inside resolve_driver_path is downloading a local binary after not finding one.

mattbrictson · 2023-12-12T16:16:29Z

@majkelcc that sounds like it should be a new bug report.

I am not verify familiar with how remote browsers work with Selenium, but apparently Rails does not consult options[:browser] when preloading. As you can see in the following code, only :using is considered, and other options (like browser: :remote) are ignored.

rails/actionpack/lib/action_dispatch/system_testing/driver.rb

Lines 18 to 19 in 1eda300

    
           @browser = Browser.new(options[:using]) 
        
           @browser.preload

I suspect Rails 7.1.1 was working for you because prior to 7.1.2, the preload mechanism only worked if driver_path was set to a Proc, which was being done by the webdrivers gem. So if you used 7.1.1 without webdrivers, essentially preload was a no-op. In 7.1.2 (i.e. as a result of this PR), the preload mechanism now calls Selenium::WebDriver::DriverFinder explicitly, hence the local download that you are seeing.

rails-bot bot added the actionpack label Nov 3, 2023

mattbrictson force-pushed the bugs/chromedriver-race-cond branch 2 times, most recently from f60821b to 1c1da2c Compare November 3, 2023 21:23

jonathanhefner reviewed Nov 6, 2023

View reviewed changes

mattbrictson force-pushed the bugs/chromedriver-race-cond branch from 8685999 to deaf466 Compare November 6, 2023 23:19

mattbrictson force-pushed the bugs/chromedriver-race-cond branch from 3f089b3 to 378991a Compare November 7, 2023 04:57

jonathanhefner force-pushed the bugs/chromedriver-race-cond branch from 378991a to e7d743b Compare November 7, 2023 17:26

jonathanhefner merged commit 3162afe into rails:main Nov 7, 2023
4 checks passed

mattbrictson deleted the bugs/chromedriver-race-cond branch November 7, 2023 19:02

rafaelfranca pushed a commit that referenced this pull request Nov 8, 2023

Merge pull request #49908 from mattbrictson/bugs/chromedriver-race-cond

eaee120

Preload Selenium driver_path before parallelizing system tests

lazyatom mentioned this pull request Dec 6, 2023

Rails 7.1.2 system tests won't find Chrome downloaded by selenium-webdriver #50287

Closed

jonathanhefner mentioned this pull request Dec 7, 2023

Fix system tests with Chrome cached by Selenium #50296

Merged

srabuini mentioned this pull request Jan 21, 2024

System tests not working on Rails 7.1.3 running on a remote machine/docker container. #50827

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preload Selenium driver_path before parallelizing system tests #49908

Preload Selenium driver_path before parallelizing system tests #49908

mattbrictson commented Nov 3, 2023

jonathanhefner Nov 6, 2023

mattbrictson Nov 6, 2023

mattbrictson Nov 6, 2023

jonathanhefner Nov 6, 2023

mattbrictson Nov 6, 2023

mattbrictson Nov 6, 2023

jonathanhefner Nov 6, 2023

mattbrictson Nov 7, 2023

mattbrictson commented Nov 6, 2023

mattbrictson commented Nov 7, 2023

jonathanhefner commented Nov 7, 2023

lazyatom commented Dec 6, 2023

lazyatom commented Dec 6, 2023

jonathanhefner commented Dec 6, 2023

lazyatom commented Dec 6, 2023

majkelcc commented Dec 12, 2023

mattbrictson commented Dec 12, 2023

Preload Selenium driver_path before parallelizing system tests #49908

Preload Selenium driver_path before parallelizing system tests #49908

Conversation

mattbrictson commented Nov 3, 2023

Motivation / Background

Detail

Additional information

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattbrictson commented Nov 6, 2023

mattbrictson commented Nov 7, 2023

jonathanhefner commented Nov 7, 2023

lazyatom commented Dec 6, 2023

lazyatom commented Dec 6, 2023

jonathanhefner commented Dec 6, 2023

lazyatom commented Dec 6, 2023

majkelcc commented Dec 12, 2023

mattbrictson commented Dec 12, 2023