-
Notifications
You must be signed in to change notification settings - Fork 48
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
Incompatible with Rails 7.2 #164
Comments
I fixed this in #165 by using To use the fix before the PR gets merged, change this in your gem 'cypress-rails', github: 'ledermann/cypress-rails', branch: 'rails-7-2' |
@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:
I'm running the tests with: The same tests work correctly on Rails 7.1.3.4 and cypress-rails 0.7.0. Any ideas? Just something on my end? |
Thanks for attempting to fix this, @ledermann! However I'm getting similar problems as @minter 🤔 |
@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. |
I created a fork that comments out the calls to This fixes the errors in my case and allows all the tests to run. I haven't dug into exactly why |
@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. |
I've released 0.8.0.rc1 (which is just #167 packaged up) to hopefully fix Rails 7.2 compatibility. I removed the calls to |
|
|
@rosston Are you expecting I just wanted to raise this as a potential problem before you decide to promote this RC version. |
Yeah, it should work with Rails 7.1 as well. #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 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? |
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:
Results were the same. Meaning, Regarding Puma workers: It looks like the test app doesn't specify how many workers Puma should use. My 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! |
@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 caseUsing the released version of
Run my Cypress tests:
All of the tests pass. Failing CaseChange 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:
Some of the specs are now failing (this is consistent between multiple runs)
I'm glad to help provide any debug information you might need, but the issue does appear to be isolated to using the rc. |
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 For @minter, is there any indication that your test failures are related to app resetting like #164 (comment)? Based on your issue in #164 (comment), it sounds like |
@rosston No env variables being manually set:
As far as the app resetting, I do think that it could be a related problem, yes! |
I had |
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 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:
What I really want right now is a way to prove the usefulness of |
@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) |
I did some more looking into this. Maybe, hopefully, this is helpful? In summary, starting with some background for the sake of clarity:
The flakiness I'm seeing certainly feels like a race condition of some kind. Which maybe makes sense because the removed called to You can find my example app here if you want to play around with it locally or try to fold its functionality into the |
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 After reading your description @rosston here, I realized that my set-up is slightly different than what you're describing.
In my case 4 threads were spun, all for Puma:
This config is hardcoded in After changing Thread to be 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.
Best |
@azyzio Your branch allowed my tests to pass on both Rails 7.1 and 7.2! 🎉 |
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:
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 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 :)
I also opened a PR #169 as an alternative to the current |
@azyzio My tests also pass with this branch |
@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! |
@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 |
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 I started messing around with the I guess a potential workaround is to specify something like calling a DatabaseCleaner in |
I should have said this in my last message: The |
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 @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 |
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
Thanks for the sample app! This has been really helpful to dig into things. I have very confusingly found that removing This still leaves me stumped, but I thought it might be interesting enough to share with others. |
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):
|
Thanks for revisiting this, @rosston. What you've found is... odd 🤣 FWIW, for the past month I've been using 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! |
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:I haven't gone any deeper, but removing the two lines with
lock_thread=
fixes the problem in my application.The text was updated successfully, but these errors were encountered: