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.

Sign up for GitHub

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

Add Rails 8 #419

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+12 −9
Open

Add Rails 8 #419

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
d578a7a
Rails 8
Nov 8, 2024
97bf3f6
Fixed test
Nov 11, 2024
d16dac8
Update .gitignore
teeparham Dec 13, 2024
24bb5c0
Remove ruby 3.1 from CI test matrix
teeparham Dec 13, 2024
d373559
Merge branch 'rails8' into rails8-support
formigarafa Dec 30, 2024
038c0bf
skip false failure
formigarafa Dec 30, 2024
147fd43
Merge pull request #3 from formigarafa/rails8-support
StoneGod Dec 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions activerecord-postgis-adapter.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,9 @@ Gem::Specification.new do |spec|
spec.platform = Gem::Platform::RUBY

# ruby-lang.org/en/downloads/branches
spec.required_ruby_version = ">= 3.1.0"
spec.required_ruby_version = ">= 3.2.0"
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we keep Support for 3.1 until EOL ? (2025-03-31)

Copy link
Author

@StoneGod StoneGod Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Rails 8 required ruby_version >= 3.2.0
https://rubygems.org/gems/rails

Copy link
Contributor

Choose a reason for hiding this comment

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

Then, also need to remove ruby 3.1 from the test matrix.
here: https://github.com/rgeo/activerecord-postgis-adapter/blob/master/.github/workflows/tests.yml#L47C9-L47C36


spec.add_dependency "activerecord", "~> 7.2.0"
spec.add_dependency "activerecord", "~> 8.0.0"
spec.add_dependency "rgeo-activerecord", "~> 8.0.0"

spec.add_development_dependency "rake", "~> 13.0"
Expand Down
3 changes: 0 additions & 3 deletions test/database.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,13 @@ connections:
username: <%= ENV["PGUSER"] || "postgres" %>
password: <%= ENV["PGPASSWORD"] || "" %>
setup: default
schema_search_path: public
arunit2:
host: <%= ENV["PGHOST"] || "127.0.0.1" %>
port: <%= ENV["PGPORT"] || "5432" %>
database: <%= ENV["PGDATABASE"] || "postgis_adapter_test" %>
username: <%= ENV["PGUSER"] || "postgres" %>
password: <%= ENV["PGPASSWORD"] || "" %>
setup: default
schema_search_path: public
Copy link
Author

Choose a reason for hiding this comment

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

This helps with test like

  1) Failure:
AsyncBelongsToAssociationsTest#test_async_load_belongs_to [/Users/safin.rr/.asdf/installs/ruby/3.2.2/lib/ruby/gems/3.2.0/bundler/gems/rails-c694e575cf0f/activerecord/test/cases/associations/belongs_to_associations_test.rb:1871]:
Expected: 1
  Actual: 2

But I have no idea why

Copy link
Member

Choose a reason for hiding this comment

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

Not sure either. Looking at the original test it does some filtering of the events based on schema name, so maybe adding this search path adds another unexpected query.

https://github.com/rails/rails/blob/dd8f7185faeca6ee968a6e9367f6d8601a83b8db/activerecord/test/cases/associations/belongs_to_associations_test.rb#L1851-L1873

Copy link
Member

Choose a reason for hiding this comment

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

But I don't think this is a big deal to remove

Copy link
Contributor

Choose a reason for hiding this comment

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

Contextual note to pair with #419 (comment)
This change probably requires some changes in the README.md file since this section talks about it quite a bit.

arunit_without_prepared_statements:
min_messages: warning
prepared_statements: false
Expand All @@ -25,4 +23,3 @@ connections:
username: <%= ENV["PGUSER"] || "postgres" %>
password: <%= ENV["PGPASSWORD"] || "" %>
setup: default
schema_search_path: public
1 change: 1 addition & 0 deletions test/excludes/ActiveRecord/ConnectionAdapters/RegistrationIsolatedTest.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
exclude "test_#resolve_raises_if_the_adapter_is_using_the_pre_7.2_adapter_registration_API", TRIAGE_MSG
Copy link
Author

@StoneGod StoneGod Nov 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This test checked error message

--- expected
+++ actual
@@ -1 +1 @@
-"Database configuration specifies nonexistent 'fake_legacy' adapter. Available adapters are: abstract, fake, mysql2, postgresql, sqlite3, trilogy. Ensure that the adapter is spelled correctly in config/database.yml and that you've added the necessary adapter gem to your Gemfile if it's not in the list of available adapters."
+"Database configuration specifies nonexistent 'fake_legacy' adapter. Available adapters are: abstract, fake, mysql2, postgis, postgresql, sqlite3, trilogy. Ensure that the adapter is spelled correctly in config/database.yml and that you've added the necessary adapter gem to your Gemfile if it's not in the list of available adapters."

4 changes: 2 additions & 2 deletions test/excludes/CounterCacheTest.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
exclude "test_decrement_counter", TRIAGE_MSG
exclude "test_reset_counters_with_touch:_true", TRIAGE_MSG
exclude "test_reset_the_right_counter_if_two_have_the_same_foreign_key", TRIAGE_MSG
exclude "test_inactive_conter_cache", TRIAGE_MSG
exclude "test_inactive_counter_cache", TRIAGE_MSG
exclude "test_update_counters_of_multiple_records", TRIAGE_MSG
exclude "test_counters_are_updated_both_in_memory_and_in_the_database_on_create", TRIAGE_MSG
exclude "test_update_counter_with_initial_null_value", TRIAGE_MSG
Expand All @@ -47,7 +47,7 @@
exclude "test_reset_counters_with_touch:_:written_on", TRIAGE_MSG
exclude "test_reset_counters_for_cpk_model", TRIAGE_MSG
exclude "test_reset_the_right_counter_if_two_have_the_same_class_name", TRIAGE_MSG
exclude "test_active_conter_cache", TRIAGE_MSG
exclude "test_active_counter_cache", TRIAGE_MSG
exclude "test_increment_counters_with_touch:_:written_on", TRIAGE_MSG
exclude "test_update_multiple_counters_with_touch:_:written_on", TRIAGE_MSG
exclude "test_counter_cache_column?", TRIAGE_MSG
Expand Down
3 changes: 3 additions & 0 deletions test/excludes/SchemaDumperTest.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,2 +1,5 @@
exclude "test_schema_dump_with_timestamptz_datetime_format", TRIAGE_MSG
exclude "test_schema_dump_when_changing_datetime_type_for_an_existing_app", TRIAGE_MSG
if ActiveRecord::Base.lease_connection.pool.server_version(ActiveRecord::Base.lease_connection) < 15_00_00
exclude "test_schema_dumps_unique_constraints", TRIAGE_MSG
end
Comment on lines +3 to +5
Copy link
Member

Choose a reason for hiding this comment

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

@BuonOmo I feel like we may want to re-implement this one locally since it seems important.

@StoneGod do you know why this one was failing?

Copy link
Author

Choose a reason for hiding this comment

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

It's failing because my local version pg is 14, but I think originally test should check version

Copy link
Contributor

Choose a reason for hiding this comment

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

This test fails because nulls_not_distinct: true doesn't exist in the position_4 index in the schema dump. This happens because that is only supported in PostgreSQL 15+ as noted in rails/rails#48608 which added the capability.
I think it makes sense to skip this test if the PostgreSQL version in use happens to be lower than 15 even if we can set that version in CI, because there may be situations like @StoneGod and I experienced where we ran the tests locally in an environment that wasn't using version 15+.

Loading