Incompatible with Rails 7.2

[ledermann]

The gem fails after upgrading an app to Rails 7.2, because ActiveRecord::ConnectionAdapters::ConnectionPool#lock_thread= does not exist anymore. There is a deprecation warning as well:

$ DISABLE_SPRING=1 RAILS_ENV=test bin/rake cypress:run

cypress-rails configuration:
============================
 CYPRESS_RAILS_DIR....................."/Users/ledermann/Projects/solectrus/solectrus"
 CYPRESS_RAILS_CYPRESS_DIR............."/Users/ledermann/Projects/solectrus/solectrus"
 CYPRESS_RAILS_HOST...................."127.0.0.1"
 CYPRESS_RAILS_PORT....................nil
 CYPRESS_RAILS_BASE_PATH..............."/"
 CYPRESS_RAILS_TRANSACTIONAL_SERVER....true
 CYPRESS_RAILS_CYPRESS_OPTS............""

DEPRECATION WARNING: ActiveRecord::ConnectionAdapters::ConnectionPool#connection is deprecated
and will be removed in Rails 8.0. Use #lease_connection instead.
 (called from map at /Users/ledermann/.rbenv/versions/3.3.4/lib/ruby/gems/3.3.0/gems/cypress-rails-0.7.0/lib/cypress-rails/manages_transactions.rb:61)
Coverage report generated for Cypress, RSpec to /Users/ledermann/Projects/solectrus/solectrus/coverage. 1905 / 2661 LOC (71.59%) covered.
Stopped processing SimpleCov as a previous error not related to SimpleCov has been detected
/Users/ledermann/.rbenv/versions/3.3.4/lib/ruby/gems/3.3.0/gems/cypress-rails-0.7.0/lib/cypress-rails/manages_transactions.rb:13:in `block in begin_transaction': undefined method `lock_thread=' for an instance of ActiveRecord::ConnectionAdapters::ConnectionPool (NoMethodError)

        connection.pool.lock_thread = true
                       ^^^^^^^^^^^^^^
    from /Users/ledermann/.rbenv/versions/3.3.4/lib/ruby/gems/3.3.0/gems/cypress-rails-0.7.0/lib/cypress-rails/manages_transactions.rb:11:in `each'
    from /Users/ledermann/.rbenv/versions/3.3.4/lib/ruby/gems/3.3.0/gems/cypress-rails-0.7.0/lib/cypress-rails/manages_transactions.rb:11:in `begin_transaction'
    from /Users/ledermann/.rbenv/versions/3.3.4/lib/ruby/gems/3.3.0/gems/cypress-rails-0.7.0/lib/cypress-rails/launches_cypress.rb:20:in `call'
    from /Users/ledermann/.rbenv/versions/3.3.4/lib/ruby/gems/3.3.0/gems/cypress-rails-0.7.0/lib/cypress-rails/run.rb:11:in `call'
    from /Users/ledermann/.rbenv/versions/3.3.4/lib/ruby/gems/3.3.0/gems/cypress-rails-0.7.0/exe/cypress-rails:15:in `<main>'

I haven't gone any deeper, but removing the two lines with lock_thread= fixes the problem in my application.

[ledermann]

I fixed this in #165 by using pin_connection!/ unpin_connection! when Rails 7.2 is present.

To use the fix before the PR gets merged, change this in your Gemfile:

gem 'cypress-rails', github: 'ledermann/cypress-rails', branch: 'rails-7-2'

[minter]

@ledermann Quick question: I'm using your branch for my Cypress tests with Rails 7.2, and they're all failing with what appear to be errors like this:

/Users/minter/.rvm/gems/ruby-3.3.3@suggestionox/gems/activerecord-7.2.0/lib/active_record/connection_adapters/abstract/connection_pool.rb:337:in `unpin_connection!': There isn't a pinned connection 29480 (RuntimeError)
    from /Users/minter/.rvm/gems/ruby-3.3.3@suggestionox/bundler/gems/cypress-rails-5429e5d71aad/lib/cypress-rails/manages_transactions.rb:57:in `block in rollback_transaction'
    from /Users/minter/.rvm/gems/ruby-3.3.3@suggestionox/bundler/gems/cypress-rails-5429e5d71aad/lib/cypress-rails/manages_transactions.rb:53:in `each'

I'm running the tests with: DATABASE_URL=postgres://minter@localhost:5432/sox_test rake cypress:run

The same tests work correctly on Rails 7.1.3.4 and cypress-rails 0.7.0.

Any ideas? Just something on my end?

[erikphansen]

Thanks for attempting to fix this, @ledermann! However I'm getting similar problems as @minter 🤔

[ledermann]

@minter, @erikphansen: Strange. My fix works fine for my Rails 7.2 application, the original only works for Rails 7.1. Sorry, I have no idea why it is failing for you.

[mhoofman]

I created a fork that comments out the calls to lock_thread and changes the deprecated call to connection to lease_connection: https://github.com/mhoofman/cypress-rails/tree/rails-7.2

This fixes the errors in my case and allows all the tests to run.

I haven't dug into exactly why lock_thread is needed or not needed but this "works on my machine".

[minter]

@mhoofman Your branch does seem to allow the tests to run. I'm running into an odd situation where my tests all run successfully on Rails. 7.1.3.4 and stock cypress-rails 0.7.0, but a portion of the tests fail on Rails 7.2.1 and your Rails 7.2 branch. Because the two are linked (can't only run one in isolation), I can't tell if it's a Rails 7.2 issue or a cypress-rails issue.

[rosston]

I've released 0.8.0.rc1 (which is just https://github.com/testdouble/cypress-rails/pull/167 packaged up) to hopefully fix Rails 7.2 compatibility. I removed the calls to lock_thread entirely since I don't think they're needed (similar to https://github.com/testdouble/cypress-rails/issues/164#issuecomment-2351142022). But I'd love to have a couple of you with real world applications use this RC version and let me know if things work.

[ledermann]

0.8.0.rc1 works fine for my app, thanks!

[mhoofman]

0.8.0.rc1 works for me. My app is running Rails 7.2.1. Thank you.

[erikphansen]

@rosston Are you expecting 0.8.0.rc1 to also work with a Rails 7.1 app? I'm getting lots of Cypress test failures when I upgrade from 0.7.1 to 0.8.0.rc1. I'm just starting to look into this, but it seems like my calls to cy.request('/cypress_rails_reset_state') are not actually reseting state. I make that call in a beforeEach hook in many of my Cypress test suites.

I just wanted to raise this as a potential problem before you decide to promote this RC version.

[rosston]

@rosston Are you expecting 0.8.0.rc1 to also work with a Rails 7.1 app?

Yeah, it should work with Rails 7.1 as well. https://github.com/testdouble/cypress-rails/pull/167 runs the example app tests against both Rails 7.1 and 7.2. And there's an existing test in there that makes use of cy.request('/cypress_rails_reset_state'). 🤔

Thanks for raising this! If there's any chance you can put together a minimal repo with that issue, or shed any light on what might be different between your setup and the example app's, I'd appreciate it. Throwing guesses out there with nothing to go on, maybe you're running Puma with more workers than the example app?

[erikphansen]

Thanks for the quick response! I am admittedly still fairly new to Rails, so this could definitely be something obvious on my end. But... since my test failures look exactly like what I'd expect to happen if I didn't reset the DB between tests, I did an experiment where I compared:

• the test results with 0.8.0.rc1
• the test results with 0.7.1 but _with all of my calls to cy.request('/cypress_rails_reset_state') commented out_

Results were the same. Meaning, 0.8.0.rc1 is operating as though the app state is never getting reset.

Regarding Puma workers: It looks like the test app doesn't specify how many workers Puma should use. My puma.rb config is pretty boilerplate but has the following line, I believe based on Heroku's recommendations: workers Integer(ENV["WEB_CONCURRENCY"] || 2). Commenting out that line does not change my results.

My app is using postgresql in all environments, which is the first obvious difference between my app and the test app.

I'll keep digging!

[minter]

@rosston I'm seeing an odd situation on my Rails app (7.1.3.4). Testing on 7.1 in preparation for moving to 7.2.

Working case

Using the released version of cypress-rails

Wade-Minter~/git/Suggestion-Ox(testdouble-fix|✔) % bundle show cypress-rails
/Users/minter/.rvm/gems/ruby-3.3.3@suggestionox/gems/cypress-rails-0.7.1

Run my Cypress tests:

Wade-Minter~/git/Suggestion-Ox(testdouble-fix|✔) % DATABASE_URL=postgres://minter@localhost:5432/sox_test?pool=5 rake cypress:run

[...]

Launching Cypress…
$ CYPRESS_BASE_URL="http://127.0.0.1:56448/" "/Users/minter/git/Suggestion-Ox/node_modules/.bin/cypress" run --project "/Users/minter/git/Suggestion-Ox" 

[...]

    ✔  All specs passed!                        00:41       28       28        -        -        -  

All of the tests pass.

Failing Case

Change my Gemfile to the rc:

index c7cf7733..2f786fac 100644
--- a/Gemfile
+++ b/Gemfile
@@ -76,7 +76,7 @@ gem "rails-i18n"
 gem "i18n-js"

 group :development, :test do
-  gem "cypress-rails"
+  gem "cypress-rails", git: "https://github.com/testdouble/cypress-rails", branch: "rails-72-compat"
   gem "rails-perftest"
   gem "byebug"
   gem "i18n-tasks"
diff --git a/Gemfile.lock b/Gemfile.lock
index 15c11df7..23a5e75a 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -14,6 +14,15 @@ GIT
       actionpack (>= 3.2)
       railties (>= 3.2)

+GIT
+  remote: https://github.com/testdouble/cypress-rails
+  revision: bf27fcd2ca6ed93d0ab1c98b349c1d341d18f482
+  branch: rails-72-compat
+  specs:
+    cypress-rails (0.8.0.rc1)
+      puma (>= 3.8.0)
+      railties (>= 5.2.0)
+
 GEM
   remote: https://rails-assets.org/
   specs:
@@ -209,9 +218,6 @@ GEM
       rexml
     crass (1.0.6)
     csv (3.3.0)
-    cypress-rails (0.7.1)
-      puma (>= 3.8.0)
-      railties (>= 5.2.0)
     date (3.3.4)
     devise (4.9.4)
       bcrypt (~> 3.0)
@@ -665,7 +671,7 @@ DEPENDENCIES
   capistrano-yarn
   chargebee (~> 2)
   chroma
-  cypress-rails
+  cypress-rails!
   devise
   devise-async
   devise-encryptable

Run the specs again, the same way:

Wade-Minter~/git/Suggestion-Ox(testdouble-fix|✚2) % DATABASE_URL=postgres://minter@localhost:5432/sox_test?pool=5 rake cypress:run

[...]

Launching Cypress…
$ CYPRESS_BASE_URL="http://127.0.0.1:57903/" "/Users/minter/git/Suggestion-Ox/node_modules/.bin/cypress" run --project "/Users/minter/git/Suggestion-Ox" 

Some of the specs are now failing (this is consistent between multiple runs)

    ✖  5 of 7 failed (71%)                      01:09       28       21        7        -        -  

I'm glad to help provide any debug information you might need, but the issue does appear to be isolated to using the rc.

[rosston]

Thanks both @minter and @erikphansen! Sounds like I've got something to figure out yet. 😓 I'll poke around and see if I can reproduce any issues on my end with Rails 7.1.

For both of you, would you mind just letting me know which CYPRESS_RAILS_* env vars you have set, if any?

For @minter, is there any indication that your test failures are related to app resetting like https://github.com/testdouble/cypress-rails/issues/164#issuecomment-2375222244? Based on your issue in https://github.com/testdouble/cypress-rails/issues/164#issuecomment-2353284863, it sounds like rc1 will have issues on certain kinds of Rails 7.2 apps too.

[minter]

@rosston No env variables being manually set:

Wade-Minter~/git/Suggestion-Ox(testdouble-fix|✚2) % env | grep -i cypress
Wade-Minter~/git/Suggestion-Ox(testdouble-fix|✚2) % 

As far as the app resetting, I do think that it could be a related problem, yes!

[erikphansen]

I had CYPRESS_RAILS_CYPRESS_OPTS=--e2e --browser chrome set, but even with that cleared out I am getting the same results.

[rosston]

I didn't expect anything useful, but just FYI I tried switching the example app over to postgres instead of sqlite, and all the tests still pass with rc1.

I'm gonna have to puzzle on this some more. In the meantime, I guess it's something that there's a version on RubyGems that works for some people on Rails 7.2? 😅

---

The way I see it, what we know right now is:

• lock_thread or something equivalent is required for some apps' tests to pass, but we don't know why and can't yet reproduce it in the example app
• pin_connection and unpin_connection also don't work for some cases (this can be shown by applying the diff from https://github.com/testdouble/cypress-rails/pull/165 onto the diff from https://github.com/testdouble/cypress-rails/pull/167 and running the example app tests in Rails 7.2)

What I really want right now is a way to prove the usefulness of lock_thread on Rails 7.1 in the example app tests (i.e., a failing test in Rails 7.1 that is solved by adding lock_thread back in). Then from there, we could work out a proper change that should actually work for all these different apps and Rails versions.

[minter]

@rosston In case it's a useful datapoint, I see the same failures with Rails 7.2.1 and the rc (so it's not just happening with Rails 7.1)

[erikphansen]

I did some more looking into this. Maybe, hopefully, this is helpful?

In summary, starting with some background for the sake of clarity:

• In my application, cypress-rails 0.8.0.rc1 was not resetting application state between tests the way previous versions did.
• The tests for the (very simple) example application bundled with cypress-rails worked fine and app state was getting reset correctly.
• So I created a sample app to show that resetting app state with cy.request('/cypress_rails_reset_state') is flaky with cypress-rails 0.8.0.rc1
• My theory was that something about my app having many more models and/or using Postgres instead of SQLite was causing state restoration to fail.
• I created a stripped down version of a section of my app, hoping I could get cypress-rails to trip up when trying to reset state.
• The structure of my example app is pretty simple: A Ticket belongs to a User and a Committee. And a Committee belongs to a Community.
• And the Cypress test suite is two tests: First, add a new Ticket through the UI. Second, confirm that new Ticket is gone after resetting application state.
• This is where things get really annoying. That second test does fail, if you are using Postgres. At least most of the time. Sometimes I'd have to run cypress:run a few times before it starts failing. And things are strange in interactive mode with: cypress:open. Sometimes state gets restored between tests and the test passes. Usually state is not restored and the second test fails. And further, if you get rerunning the suite in the Cypress UI, sometimes the list of Tickets keeps growing and growing as the first test keeps adding a new Ticket.
• I tried expanding the example app in the cypress-rails repo, adding in the new features and tests of my standalone example app. However, that new functionality of the app failed to work correctly when running in Cypress (even though it worked fine when firing up the app locally with the Rails server command). This is when I decided to throw in the towel 😅

The flakiness I'm seeing certainly feels like a race condition of some kind. Which maybe makes sense because the removed called to connection.pool.lock_thread were probably there to prevent race conditions.

You can find my example app here if you want to play around with it locally or try to fold its functionality into the cypress-rails repo.

[azyzio]

Hi,

I'm here with some extra findings.

In our case, the main issue was that puma suddenly stopped responding in many tests and the request timed out.

First I tried to see if the problems are caused by Rails upgrade to 7.2 or cypress-rails upgrade do rc1. So I tested rc1 with Rails 7.1 and the same test failures occured.

rc1 is introducing two main changes: getting rid of setting lock_thread and switching from connection to lease_connection. I wasn't sure which of those two is causing problems and so I removed setting of lock_threads from the stable 0.7.1 version. The tests started failing in exactly those same places.

After reading your description @rosston here, I realized that my set-up is slightly different than what you're describing.

When a system test starts Puma spins up one thread and Capybara spins up another thread.

In my case 4 threads were spun, all for Puma:

before_server_start
Starting Puma...
* Version 6.4.3 , codename: The Eagle of Durango
* Min threads: 0, max threads: 4
* Listening on http://127.0.0.1:3003

This config is hardcoded in puma.rb as: default_options = {Host: host, Port: port, Threads: "0:4", workers: 0, daemon: false}

After changing Thread to be "0:1", the tests started to work again.

I retested on Rails 7.2 with rc1 introducing this change to maximum threads number and it fixed the problem.

I checked the example app here in this repo and it's also using 0..4 threads. Why is it then working and mine isn't? I'm not sure, but I assume that the complexity of my app is much higher and so it manages to make use of the extra threads available. When they're not returned to the pool - puma runs out of available threads and stop responding.

If the assumption is that cypress tests should run on one thread, then I think we could enforce it. I prepered a branch limiting the number of threads if anyone else wants to test it on their app.

gem 'cypress-rails', github: 'azyzio/cypress-rails', branch: 'rails-72-compat'

Best Adam

[minter]

@azyzio Your branch allowed my tests to pass on both Rails 7.1 and 7.2! 🎉

[azyzio]

I'm not really confident about dropping the threads count for all cypress-rails users, so I decided to dig a little deeper. The previous solution could work as a hack for now, but I don't think this should be the final solution.

I realised that the original version of ManagesTransaction is an almost copy-paste of TestFixtures from the time that cypress-rails was created from Rails repo. There are some naming changes, but the code is really similar:

• gather_connections = enlist_fixture_connections
• begin_transaction = bottom half of setup_fixtures
• rollback_transaction = teardown_fixtures
• setup_shared_connection_pool = setup_shared_connection_pool

The not so hacky approach that still doesn't require a total understanding of this connection pooling, is to find the part of the code that has the same function, but in new TestFixtures.

So I did just that. I managed to recreate most of the changes and put them in a new ManagesTransaction class. As the whole approach is very different, I decided to put the old logic in a separate class dedicated to versions before Rails 7.2. The legacy version can only be deleted when cypress-rails decides to stop supporting older versions.

I prefer this approach to my previous one as it still allows us to use several threads and is fully utilizing the new approach in Rails.

My tests are working on this new branch. Please check yours too :)

gem 'cypress-rails', github: 'azyzio/cypress-rails', branch: 'rails72-readiness'

I also opened a PR #169 as an alternative to the current rc1.

[minter]

@azyzio My tests also pass with this branch

[erikphansen]

@azyzio Your branch is looking good for me, too. Things work exactly as expected with Rails 7.1. I'm getting test failures when I upgrade to Rails 7.2.1 but that appears to be related to something that changed in Rails 7.2. Thanks for your work on this!

[erikphansen]

@azyzio Three days ago I said that your branch seemed to be working and the test failures I was seeing were due to a change in Rails 7.2. Well, turns out I was wrong :( I addressed the issues that were introduced by Rails 7.2. But now I'm running into issues that appear to be either race conditions and/or state not getting properly restored. I updated my sample app to use your branch and it's certainly flaky when running in interactive mode.

If I use your earlier approach, editing puma.rb to limit the threads to 1 instead of 4, things are rock solid.

[azyzio]

Thanks for the failing sample app @erikphansen!

I can see that the transaction doesn't get rolled back correctly between the tests. The rollback is a part of the ConnectionPool.unpin_connection! method. In my branch solution, it is only run if there are any pools with active_connection?. It seems that between the tests, none of the connections are actually considered active.

I started messing around with the setup_shared_connection_pool which is very different than the one in ActiveRecord::TestFixtures, but couldn't make it work now.

I guess a potential workaround is to specify something like calling a DatabaseCleaner in CypressRails.hooks.after_state_reset

[erikphansen]

I should have said this in my last message: The 0.8.0.rc1 branch that @rosston put up a while ago also works fine for me if I adjust its puma.rb to limit threads to 1 instead of 4.

[rosston]

Thanks all for staying active on this while I've been neglecting it.

@azyzio The approach you took of porting the "same" code from Rails 7.2 and then branching on using those two different ManagesTransactions is privately what I had resigned myself to as well. I say "resigned" because I figured the only proof we'd have that it works is if folks' test suites pass. It's great that folks are so helpful and active about this! But that's also less ideal than a failing test in the in-repo sample app that we can make pass. Anyway, that's not your problem, and I appreciate your work here! I'll take a closer look at your PR soon. 🙇

@erikphansen Thank you for your sample app! I'll take a closer look at that as well and see if we can strengthen our in-repo sample app and tests.

As for the way forward, it sounds like a re-ported ManagesTransactions isn't (yet) a slam dunk. But limiting Puma to a single thread is. Even though our in-repo sample app is using more than one thread for Puma. 🤔 The bit I had in my commit message elsewhere was from the original message for the commit that added lock_thread to Rails. It does make me wonder if Rails system tests spin up the Rails server differently than how many of us are doing it with cypress-rails? If Rails system tests limited the Rails process to a single thread, I'd be somewhat inclined to make that the "fix" for this as well. So that's another thing for me to look into — how does Rails start up the necessary processes in system tests?

[rosston]

It does make me wonder if Rails system tests spin up the Rails server differently than how many of us are doing it with cypress-rails? If Rails system tests limited the Rails process to a single thread, I'd be somewhat inclined to make that the "fix" for this as well. So that's another thing for me to look into — how does Rails start up the necessary processes in system tests?

I've lost the receipts on this, and I honestly don't feel like looking them up right now. But I did look into the system test runner code within Rails, and it just does run Rails.application like cypress-rails, nothing special.

• So I created a sample app to show that resetting app state with cy.request('/cypress_rails_reset_state') is flaky with cypress-rails 0.8.0.rc1

Thanks for the sample app! This has been really helpful to dig into things. I have very confusingly found that removing javascript_importmap_tags from the layout consistently makes the sample app tests pass. Even removing the importmap pins and removing the content of application.js but leaving the javascript_importmap_tags in place leads to intermittently breaking tests. (I can push branches for this if that's helpful.)

This still leaves me stumped, but I thought it might be interesting enough to share with others.

[rosston]

Here are the branches I mentioned above, both of which use the RC for some consistency (though I believe they exhibit the same behavior with @azyzio's branch):

• no-importmap
  • just removes javascript_importmap_tags from the layout
  • for me, this passes consistently with bin/rails cypress:open
• empty-js
  • empties out the JS in the application
  • for me, this fails very often (thought not always?) with bin/rails cypress:open

[erikphansen]

Thanks for revisiting this, @rosston. What you've found is... odd 🤣

FWIW, for the past month I've been using 0.8.0.rc1 with its puma.rb modified so threads are limited to 1. It's been working great with Rails 7.2.2. Maybe I'll try upgrading to Rails 8 to see what happens. Honestly, I think it's less flaky than it was with previous versions of this gem.

For the past 13 months I've had Cypress set to retry failing tests twice. I've needed that because when running my Cypress suite in non-interactive mode I'd always get at least one or two tests that would randomly fail and need to retry once or twice before they passed. This is rarely an issue now. I attribute it to limiting threads to 1. I can't (or don't want to take the time to) prove that definitively. Suffice it to say, I've been perfectly happy with this solution!