Add Rails 8

[StoneGod]

Hello guys!

I'm not sure that all tests are successful; locally a couple fail.
You can run it on CI; if something falls, I’ll figure out why and try to fix it.

Therefore, can I ask to run a CI tests?

[StoneGod]

@BuonOmo Hi, can you help me with test?

[JamesChevalier]

Yeah, I'm seeing 6 failed tests around these files:

• activerecord/test/cases/schema_dumper_test.rb:253 (main / v8.0.0)
• activerecord/test/cases/associations/has_many_associations_test.rb:3281 (main / v8.0.0)
• activerecord/test/cases/query_cache_test.rb:568 (main / v8.0.0)
• activerecord/test/cases/associations/belongs_to_associations_test.rb:1871 (main / v8.0.0)
• activerecord/test/cases/associations/has_one_associations_test.rb:999 (main / v8.0.0)
• activerecord/test/cases/connection_adapters/registration_test.rb:76 (main / v8.0.0)

Test result output

  1) Failure:
SchemaDumperTest#test_schema_dumps_unique_constraints [/Users/me/.asdf/installs/ruby/3.3.6/lib/ruby/gems/3.3.0/bundler/gems/rails-c694e575cf0f/activerecord/test/cases/schema_dumper_test.rb:253]:
Expected /t\.unique_constraint\ \["position_4"\],\ nulls_not_distinct:\ true,\ name:\ "test_unique_constraints_position_nulls_not_distinct"/ to match "# This file is auto-generated from the current state of the database. Instead\n# of editing this file, please use the migrations feature of Active Record to\n# incrementally modify your database, and then regenerate this schema definition.\n#\n# This file is the source Rails uses to define your schema when running `bin/rails\n# db:schema:load`. When creating a new database, `bin/rails db:schema:load` tends to\n# be faster and is potentially less error prone than running all of your\n# migrations from scratch. Old migrations may fail to apply correctly if those\n# migrations use external dependencies or application code.\n#\n# It's strongly recommended that you check this file into your version control system.\n\nActiveRecord::Schema[8.0].define(version: 0) do\n  # These are extensions that must be enabled in order to support this database\n  enable_extension \"ltree\"\n  enable_extension \"pg_catalog.plpgsql\"\n  enable_extension \"pgcrypto\"\n  enable_extension \"postgis\"\n  enable_extension \"uuid-ossp\"\n\n  create_table \"test_unique_constraints\", force: :cascade do |t|\n    t.integer \"position_1\"\n    t.integer \"position_2\"\n    t.integer \"position_3\"\n    t.integer \"position_4\"\n\n    t.unique_constraint [\"position_1\"], name: \"test_unique_constraints_position_deferrable_false\"\n    t.unique_constraint [\"position_2\"], deferrable: :immediate, name: \"test_unique_constraints_position_deferrable_immediate\"\n    t.unique_constraint [\"position_3\"], deferrable: :deferred, name: \"test_unique_constraints_position_deferrable_deferred\"\n    t.unique_constraint [\"position_4\"], name: \"test_unique_constraints_position_nulls_not_distinct\"\n  end\nend\n".

  2) Failure:
AsyncHasManyAssociationsTest#test_async_load_has_many [/Users/me/.asdf/installs/ruby/3.3.6/lib/ruby/gems/3.3.0/bundler/gems/rails-c694e575cf0f/activerecord/test/cases/associations/has_many_associations_test.rb:3281]:
Expected: 1
  Actual: 2

  3) Failure:
QueryCacheTest#test_cache_is_available_when_using_a_not_connected_connection [/Users/me/.asdf/installs/ruby/3.3.6/lib/ruby/gems/3.3.0/bundler/gems/rails-c694e575cf0f/activerecord/test/cases/query_cache_test.rb:568]:
2 instead of 1 queries were executed. Queries: SET search_path TO public

SELECT "tasks".* FROM "tasks" WHERE "tasks"."id" = $1 LIMIT $2.
Expected: 1
  Actual: 2

  4) Failure:
AsyncBelongsToAssociationsTest#test_async_load_belongs_to [/Users/me/.asdf/installs/ruby/3.3.6/lib/ruby/gems/3.3.0/bundler/gems/rails-c694e575cf0f/activerecord/test/cases/associations/belongs_to_associations_test.rb:1871]:
Expected: 1
  Actual: 2

  5) Failure:
AsyncHasOneAssociationsTest#test_async_load_has_one [/Users/me/.asdf/installs/ruby/3.3.6/lib/ruby/gems/3.3.0/bundler/gems/rails-c694e575cf0f/activerecord/test/cases/associations/has_one_associations_test.rb:999]:
Expected: 1
  Actual: 2

  6) Failure:
ActiveRecord::ConnectionAdapters::RegistrationIsolatedTest#test_#resolve_raises_if_the_adapter_is_using_the_pre_7.2_adapter_registration_API [/Users/me/.asdf/installs/ruby/3.3.6/lib/ruby/gems/3.3.0/bundler/gems/rails-c694e575cf0f/activerecord/test/cases/connection_adapters/registration_test.rb:76]:
--- 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."


9592 runs, 32501 assertions, 6 failures, 0 errors, 50 skips

You have skipped tests. Run with --verbose for details.
rake aborted!
Command failed with status (1)
/Users/me/Documents/activerecord-postgis-adapter/Rakefile:43:in `block (2 levels) in <top (required)>'
/Users/me/.asdf/installs/ruby/3.3.6/bin/bundle:25:in `load'
/Users/me/.asdf/installs/ruby/3.3.6/bin/bundle:25:in `<main>'
Tasks: TOP => test_all
(See full trace by running task with --trace)

If I add them to the excludes folder, then all tests pass successfully 🙈 🤣

Seriously, though, I'd like to try and help out. This is what I've been able to put together so far:

• Failure 1 is happening because nulls_not_distinct: true doesn't exist in the position_4 index in the schema dump.
• Failures 2, 4, and 5 all look closely related.
• Failure 3 looks like the SET search_path TO public query is being counted in an unexpected way (which could related it to 2, 4 and 5 since those are failing on query counts).
• Failure 6 suggests we're using "pre 7.2 adapter registration API"

---

Failure 1 is resolved by updating my local PostgreSQL server to 17 (I was using 14). The nulls_not_distinct test was likely failing because that is only supported in PostgreSQL 15+ as noted in rails/rails#48608 which added the capability.
So this failure can be resolved by adding something like this to the test/excludes/SchemaDumperTest.rb file:
exclude "test_schema_dumps_unique_constraints", "Skipping due to PostgreSQL lower than 15" if ActiveRecord::Base.connection.execute("SHOW server_version").first['server_version'].to_f < 15.0

Failures 2-5 are all resolved by removing schema_search_path: public from the test/database.yml file 🤔 🤷

Failure 6 is probably bogus and should be excluded. The difference between expected and actual response is only the addition of "postgis" in the list of available adapters, which is what we fully want/expect.
I'd recommend adding this to a new test/excludes/ActiveRecord/ConnectionAdapters/RegistrationIsolatedTest.rb file:
exclude "test_#resolve_raises_if_the_adapter_is_using_the_pre_7.2_adapter_registration_API", "We are purposefully adding postgis as an available adapter"

[StoneGod]

@JamesChevalier
Do you have this?

Method CounterCacheTest#test_inactive_conter_cache is not defined
Method CounterCacheTest#test_active_conter_cache is not defined

[JamesChevalier]

Yes, I do see those two warnings emitted during the test run as well. There's also this warning prior to the test run:

/Users/me/.asdf/installs/ruby/3.3.6/lib/ruby/gems/3.3.0/bundler/gems/rails-c694e575cf0f/activesupport/lib/active_support/core_ext/object/blank.rb:153: warning: method redefined; discarding old blank?
/Users/me/.asdf/installs/ruby/3.3.6/lib/ruby/gems/3.3.0/gems/rubocop-1.68.0/lib/rubocop/core_ext/string.rb:15: warning: previous definition of blank? was here

I'm not completely certain, but I don't think we need to concern ourselves with these particular bits of output.

[StoneGod]

Thank you!

I made a commit with fix test.
Locally all checks passed.

I don’t know if it’s necessary to edit rubocop in this MR or if it would be better to open a new one

[JamesChevalier]

I was joking about excluding all the failing tests 😬
I suspect these should actually be fixed in the code somehow

[StoneGod]

I thought this was normal.

Need a vacation

[JamesChevalier]

@StoneGod I think I have some fixes that you can include. I updated my #419 (comment) with extra details, but here's the short version:

Add this to the test/excludes/SchemaDumperTest.rb file:
exclude "test_schema_dumps_unique_constraints", "Skipping due to PostgreSQL lower than 15" if ActiveRecord::ConnectionAdapters::PostGIS::VERSION.to_f < 15.0

Create a new test/excludes/AsyncHasOneAssociationsTest.rb file and add this to it:
exclude "test_async_load_has_one", "We are purposefully adding postgis as an available adapter"

Also, you were totally right with your "conter_cache" fixes in test/excludes/CounterCacheTest.rb (lines 33 and 50) so you can re-add those as well. Those two line changes will remove the two mid-test warnings we were seeing. 👍

[StoneGod]

ActiveRecord::ConnectionAdapters::PostGIS::VERSION.to_f < 15.0
We should check db version, not gem version.

Can check like this

::ActiveRecord::Base.connection_pool.server_version(ActiveRecord::ConnectionAdapters::PostGISAdapter.new(conn_hash.merge(adapter: "postgis")).raw_connection) >= 15_00_00

[JamesChevalier]

🤦 Thanks for correcting me on how my suggestion gets the gem version, not the PostgreSQL version

I tried your example, and got this error:

undefined method `server_version' for an instance of ActiveRecord::ConnectionAdapters::ConnectionPool

I was able to get ActiveRecord::Base.connection.execute("SHOW server_version").first['server_version'] to return a float as a string e.g. "14.13" and on a Mac using Postgres.app it returns "17.0 (Postgres.app)" ... both respond well to to_f, resulting in 14.13 and 17.0 respectively.

[StoneGod]

I removed schema_search_path: public from test/database.yml, when
AsyncHasOneAssociationsTest.rb passed

I don't understand why it help.

[StoneGod]

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

[keithdoggett]

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

[keithdoggett]

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

[StoneGod]

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

[StoneGod]

@keithdoggett Hi! Can you please show my MR?

[BuonOmo]

Hi @StoneGod @JamesChevalier thanks for working on the PR. I just approved the test workflow, lets see if tests are running here. I didn't and won't have time soon to review this fully. I guess people that want rails-9 now can point to your branch at first, and contribute if they see failures.

Maybe @keithdoggett will have more time even though I doubt it.

If it is critical for any of your companies, talk to your bosses and send me an email to discuss support.

[BuonOmo]

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

[StoneGod]

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

[formigarafa]

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

[igorkasyanchuk]

Is anyone using this branch already in production?

[januszm]

Yes, I used it for the past couple of days and it works just fine.

[keithdoggett]

Thanks for the work on this everyone. Only concern is about that schema dumper test but everything else looks good.

Also we will need to remove old rubies from the CI.

[keithdoggett]

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

[keithdoggett]

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

[keithdoggett]

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

[StoneGod]

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

[JamesChevalier]

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

[januszm]

@StoneGod do you need a help with this to finish?

[StoneGod]

@januszm Yes, I need help.