-
-
Notifications
You must be signed in to change notification settings - Fork 468
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
Test against rails 7.1 #995
Conversation
I.... don't know why CI is failing this way only on 7.1, it looks like the db reset isn't creating tables? |
6190c92
to
2cf5a8a
Compare
Hey, @segiddins! I spent some time digging into the CI failures with some co-workers. The solution is not totally nailed down yet, but we're on the right track. It appears there may be an issue with how the table(s) are being created. Like you noticed, There's still some issue with the migration generation, but again, feeling like the solution is on its way. Hope to unblock you within the next week. :) |
f96e632
to
aacd52b
Compare
@segiddins I need to clean up my own PR's commit message and all before I merge it (#998) but I've rebased your changes on top here so now we can see those pretty pretty green lights. |
Amazing, thanks so much for the help @sej3506! Super excited to get this into RubyGems.org |
Context: In #995, a PR to add support for Rails 7.1, CI was failing. The thing causing this first failure was issue number 1 below. Each additional fix here was yet another thing CI failed on. Things in this commit: 1. Split dummy:db:reset to dummy:db:drop and dummy:set:up 2. Replace outdated use of "MiniTest" with "Minitest" 3. Update session_spec.rb `.serialize_cookies` to handle old and new versions of Rack's handling of headers 4. Remove deprecated active record handling in application.rb 5. Update request_with_remember_token.rb `remember_token_cookies` handling of headers. --- 1. Setup & Github Workflow - Split dummy:db:reset to dummy:db:drop and dummy:set:up This issue was discovered in #995. I'm still looking into why exactly this is happening, but solution has been consistent for me. To reproduce the issue for yourself: When CI runs for each version, it does so using the specific Gemfile for the version. Running `BUNDLE_GEMFILE=gemfiles/rails_7.1.gemfile RAILS_ENV=test bundle exec rake dummy:db:reset` and then `bundle exec appraisal rake` the Users table will not be found, because it is not created from this command. This issue only exists with the 7.1 gemfile that I've found. Splitting the command dummy:db:reset to its component parts of dummy:db:drop and dummy:db:setup on separate lines works - for all version. Using them on a single line does not work, though. 2. Replace outdated use of "MiniTest" with "Minitest" Use of "MiniTest" (vs modern "Minitest") was removed in version 5.19.0. CI runs using version 5.20 of Minitest and was failing. https://my.diffend.io/gems/minitest/5.18.1/5.19.0 3. Update session_spec.rb `.serialize_cookies` to handle old and new versions of Rack's handling of headers The way `Rack::Utils.set_cookie_header!` works in rack 3.0.8 is different than previous versions. Rack downcases keys at the class level now. This update supports both versions of the method. rack/rack@95d2f64 Also updating spec/support/cookies.rb and spec/support/request_with_remember_token.rb for this. 4. Remove deprecated active record handling in application.rb Rails 6.0 no longer uses `config.activerecord.sqlite3.represent_boolean_as_integer` as it is always true now. rails/rails@f59b081 As well, `config.active_record.legacy_connection_handling` has been deprecated since Rails 7.0 and throws errors if used. rails/rails#45835 5. Update request_with_remember_token.rb `remember_token_cookies` handling of headers. The value might be a string or an array. Co-authored-by: Samuel Giddins <segiddins@segiddins.me>
985780f
to
c3bb668
Compare
@sej3506 i think you might've force-pushed over my last commit or two |
Oh crap!! 🤦🏻♀️ |
@segiddins I sincerely apologize. I definitely should not have been force pushing (especially on someone else's branch). If you have the commits locally still, you can add them to the branch using this method. If you do not have them locally anymore, I can add them back manually using the information here and here. |
c3bb668
to
dd5a645
Compare
All good! I added it back |
@sej3506 I think this should be ready to merge & release whenever y'all are ready! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻 Thank you!
if it's not too much of a hassle, a release with this would be 💯 🙇🏻♂️ |
@sej3506 anything I can help with to get release done? |
@segiddins @simi Sorry for the delay on this release. It's out now! |
No description provided.