Modernize CI: Appraisal gem, Ruby 3.2-head, Rails 7.2-edge#147
Modernize CI: Appraisal gem, Ruby 3.2-head, Rails 7.2-edge#147flavorjones wants to merge 5 commits intomasterfrom
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rails main requires Ruby >= 3.3, so this combination would fail during dependency resolution.
There was a problem hiding this comment.
Pull request overview
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Updates the project’s Rails/Ruby compatibility and CI strategy by moving to Appraisal-managed Rails gemfiles and expanding the GitHub Actions matrix to cover newer Ruby/Rails combinations, while removing legacy compatibility code paths.
Changes:
- Replace
RAILS_VERSION-driven installs with Appraisal + committedgemfiles/*and CIBUNDLE_GEMFILE. - Raise minimum Rails runtime dependencies to
>= 7.2and simplify code/tests by removing older-version shims. - Expand CI matrix and harden DB services with container health checks.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/query_comments_test.rb | Removes legacy Rails/Minitest shims and simplifies controller rendering + socket config test path. |
| marginalia.gemspec | Raises runtime dependency floor to Rails components >= 7.2. |
| lib/marginalia/comment.rb | Drops pre-6.1 connection config branch; relies on db_config path only. |
| gemfiles/rails_7_2.gemfile | Adds Appraisal-generated gemfile for Rails 7.2 testing. |
| gemfiles/rails_8_0.gemfile | Adds Appraisal-generated gemfile for Rails 8.0 testing. |
| gemfiles/rails_8_1.gemfile | Adds Appraisal-generated gemfile for Rails 8.1 testing. |
| gemfiles/rails_edge.gemfile | Adds Appraisal-generated gemfile for Rails main/edge testing. |
| Gemfile | Moves base dependencies toward Appraisal-based workflows (adds appraisal, pins minitest). |
| Appraisals | Defines Rails version appraisals used to generate per-Rails gemfiles. |
| .gitignore | Ignores appraisal lockfiles under gemfiles/. |
| .github/workflows/ci.yml | Updates CI matrix, switches to BUNDLE_GEMFILE, adds DB container health checks, bumps checkout action. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -2,9 +2,5 @@ source "https://rubygems.org" | |||
|
|
|||
| gemspec | |||
|
|
|||
There was a problem hiding this comment.
The root Gemfile no longer declares rails (or railties), but the test suite requires rails/version and loads Rails components. Running bundle exec rake test:all locally with the default Gemfile will likely fail unless BUNDLE_GEMFILE is set to an appraisal gemfile. Consider adding a default Rails dependency back to Gemfile (e.g., the minimum supported Rails) or updating the test/dev workflow so the default bundle + rake path still works.
| gem "rails" |
| @@ -11,8 +11,8 @@ Gem::Specification.new do |gem| | |||
| gem.version = "1.11.1" | |||
| gem.license = "MIT" | |||
There was a problem hiding this comment.
This PR raises the minimum supported Rails/Ruby, but the gemspec doesn’t declare a required_ruby_version. Adding it helps Bundler fail fast on unsupported Rubies and keeps the published gem metadata accurate. Also consider aligning repo metadata/docs (e.g., .ruby-version and README support statements) with the new minimums so contributors don’t default to an unsupported Ruby/Rails.
| gem.license = "MIT" | |
| gem.license = "MIT" | |
| gem.required_ruby_version = ">= 3.1" |
| @@ -48,4 +55,3 @@ jobs: | |||
|
|
|||
| - name: Run tests | |||
| run: bundle exec rake db:reset test:all | |||
There was a problem hiding this comment.
db:reset calls rake tasks that shell out to mysql to create/drop the test DB. The mysql CLI defaults to a local UNIX socket when no host is provided, and it does not reliably honor MYSQL_HOST, so this can fail when the DB is provided by a GitHub Actions service container. Consider updating the DB reset/create logic to force TCP (e.g., pass -h 127.0.0.1/--protocol=tcp, and port if needed) or adjust the workflow to run DB setup inside the container network.
| run: bundle exec rake db:reset test:all | |
| run: MYSQL_HOST=127.0.0.1 MYSQL_TCP_PORT=3306 bundle exec rake db:reset test:all |
| pool.db_config.stubs(:configuration_hash).returns({:socket => "marginalia_socket"}) | ||
| Marginalia::Comment.components = [:socket] | ||
| API::V1::PostsController.action(:driver_only).call(@env) | ||
| assert_match %r{/\*socket:marginalia_socket}, @queries.first | ||
| pool.db_config.unstub(:configuration_hash) |
There was a problem hiding this comment.
This test stubs pool.db_config.configuration_hash but doesn’t ensure the stub is removed if an assertion raises. Other tests in this file use ensure for cleanup; consider wrapping the stub/unstub in a begin/ensure (or using Mocha’s block form) so stubs don’t leak across tests on failure.
| pool.db_config.stubs(:configuration_hash).returns({:socket => "marginalia_socket"}) | |
| Marginalia::Comment.components = [:socket] | |
| API::V1::PostsController.action(:driver_only).call(@env) | |
| assert_match %r{/\*socket:marginalia_socket}, @queries.first | |
| pool.db_config.unstub(:configuration_hash) | |
| begin | |
| pool.db_config.stubs(:configuration_hash).returns({ :socket => "marginalia_socket" }) | |
| Marginalia::Comment.components = [:socket] | |
| API::V1::PostsController.action(:driver_only).call(@env) | |
| assert_match %r{/\*socket:marginalia_socket}, @queries.first | |
| ensure | |
| pool.db_config.unstub(:configuration_hash) | |
| end |
Summary
RAILS_VERSIONenv var approach withBUNDLE_GEMFILEpointing at appraisal-generated gemfilesTest plan
🤖 Generated with Claude Code