feat: rails 7.1 support #300

BuonOmo · 2024-01-07T20:57:21Z

Remove deprecated configure_connection method override. It is now unnecessary as CockroachDB supports SET TIMEZONE <...>. This fixes the unset type_map bug

Fix changes induced by rails/rails@896d359

lib/active_record/connection_adapters/cockroachdb/referential_integrity.rb

lib/active_record/connection_adapters/cockroachdb/schema_statements.rb

lib/active_record/connection_adapters/cockroachdb_adapter.rb

BuonOmo · 2024-01-18T16:09:54Z

test/cases/adapters/postgresql/postgresql_adapter_test.rb

+          # TODO: It seems like in CRDB this tests won't create the index at all.
+          #   It is created in PG with `valid: false` set. Is this correct?


So here there is a behaviour difference between CRDB and PG:

PG creates the index anyway, but invalid. CRDB doesn't create it. Should I dig into it to see if this is a CRDB bug, or something we want ?

this is an area where CRDB diverges from PG. see: cockroachdb/cockroach#65929

So I should modify the test to look for a constraint then ? And add a comment referencing this issue maybe

FYI @rafiss I've looked at what is generated and I see neither and index nor a constraint

ah i see; i read the test more closely now. yes, that's another difference in CRDB. if you try to add the unique index but it fails, then neither the index nor constraint will exist.

test/cases/adapters/postgresql/virtual_column_test.rb

BuonOmo · 2024-01-18T16:19:42Z

test/cases/migration_test.rb

+    %i(
+      test_adding_indexes
+      test_removing_index
+      test_adding_multiple_columns
+      test_changing_index
+    ).each do |method_name|
+      file, line = ::BulkAlterTableMigrationsTest.instance_method(method_name).source_location
+      iter = File.foreach(file)
+      (line - 1).times { iter.next }
+      indent = iter.next[/\A\s*/]
+      content = +""
+      content << iter.next until iter.peek == indent + "end\n"
+      content['"PostgreSQLAdapter" =>'] = '"CockroachDBAdapter" =>'
+      eval(<<-RUBY, binding, __FILE__, __LINE__ + 1)
+        def #{method_name}
+          #{content}


I think we should go more in that direction to avoid having micro changes in the tests making our test suite fail. WDYT?

i don't quite understand this change, could you explain more?

Yeah definitely sorry. Basically I'm advocating for a bit of meta-programmation in tests.

now

We currently are copying failing tests, and the related private methods. And then changing the bits we want to change.

the change

I'd say that if the change is trivial, we can take advantage of ruby's meta-programmation tools to just change the parts we want. This would be way more resilient to little changes in the tests.

the benefit and cost

This would make following rails updates so much easier, and even between minors or patches there are these kind of changes. It would reduce my workload. But it would make these local versions way harder to understand, as one would have to rely on understanding the meta change. But I think with good comment and viewing the original method, this should be ok

the plan

I would make these changes progressively : with newly introduced tests, or with the one that are currently ignored and that we should stop ignoring anyway. Once there are a few examples I could refactor the meta-prog tools used (such as search and replace in the original function) to make the changes clearer and the footprint smaller

thanks for explaining. i think the approach sounds good! i'd definitely like to make your maintenance burden easier.

test/support/rake_helpers.rb

BuonOmo · 2024-01-18T19:26:19Z

@rafiss I think you can already start reviewing. The majority of failing tests were already present in previous versions, others are documented, the only ones bothering me are the context dependent ones that do not trigger every time. I think we can still go on with this version as those were also present sometimes in previous versions...

rafiss

nice work!

looks like we are at:

8808 runs, 29847 assertions, 7 failures, 9 errors, 66 skips

are we going to address those last few failures now, or handle them separately?

also, do you think we should squash this all when merging?

BuonOmo · 2024-01-24T17:42:57Z

are we going to address those last few failures now, or handle them separately?

Those failures were here before already. I definitely plan on handling them but I didn't want this to be in the scope of the PR

also, do you think we should squash this all when merging?

Cleaning up now !

This PR includes loads of small changes that won't be mentionned here to adapt to the new Rails code. Loads of tests ignored tests have been added back as they were not error prone anymore. There are likely more of these. We removed the deprecated `configure_connection` method override. It is now unnecessary as CockroachDB supports `SET TIMEZONE <...>`. We dropped support for CockroachDB version before 22.X.X. We removed a lot of copy-pasted code in the test thanks to new file separation in Rails 7.1 test configuration. Signed-off-by: Ulysse Buonomo <buonomo.ulysse@gmail.com>

BuonOmo force-pushed the rails-7.1-support branch 9 times, most recently from 87dad13 to 3d13eb6 Compare January 15, 2024 15:34