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

Test against rails 7.1 #995

Merged
merged 1 commit into from
Dec 15, 2023
Merged

Conversation

segiddins
Copy link
Contributor

No description provided.

@segiddins
Copy link
Contributor Author

I.... don't know why CI is failing this way only on 7.1, it looks like the db reset isn't creating tables?

@sej3506
Copy link
Contributor

sej3506 commented Dec 2, 2023

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, dummy:db:reset isn't doing what it should be doing. When we ran the commands separately, dummy:db:drop dummy:db:create dummy:db:migrate, we had more success.

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. :)

@sej3506
Copy link
Contributor

sej3506 commented Dec 8, 2023

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

@segiddins segiddins marked this pull request as ready for review December 9, 2023 00:04
@segiddins
Copy link
Contributor Author

Amazing, thanks so much for the help @sej3506! Super excited to get this into RubyGems.org

sej3506 added a commit that referenced this pull request Dec 12, 2023
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>
@segiddins
Copy link
Contributor Author

@sej3506 i think you might've force-pushed over my last commit or two

@sej3506
Copy link
Contributor

sej3506 commented Dec 12, 2023

@sej3506 i think you might've force-pushed over my last commit or two

Oh crap!! 🤦🏻‍♀️

@sej3506
Copy link
Contributor

sej3506 commented Dec 12, 2023

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

@segiddins
Copy link
Contributor Author

All good! I added it back

@segiddins
Copy link
Contributor Author

@sej3506 I think this should be ready to merge & release whenever y'all are ready!

Copy link
Contributor

@sej3506 sej3506 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 Thank you!

@sej3506 sej3506 merged commit 0724372 into thoughtbot:main Dec 15, 2023
18 checks passed
@segiddins
Copy link
Contributor Author

if it's not too much of a hassle, a release with this would be 💯 🙇🏻‍♂️

@simi
Copy link

simi commented Jan 15, 2024

@sej3506 anything I can help with to get release done?

@sej3506
Copy link
Contributor

sej3506 commented Jan 15, 2024

@segiddins @simi Sorry for the delay on this release. It's out now!

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

3 participants