I think it would be a bit excessive to have a whole separate file for this particular test case
I'd say we can find a place for this scenario somewhere around existing create tests - https://github.com/rails/rails/blob/f5a1075e71b79eb7eb0f4f6323468ef9c7286ee9/activerecord/test/cases/persistence_test.rb#L499

Good point! In my modified PR, the test is moved into persistence_test.rb under a new method.

While it's understood that by reaching assert true line without failing we imply that Post.create! was successful and no exception was raised. But generally I find an explicit assertion like assert_nothing_raised { Post.create! } to be even more reader friendly.

However, what I find even more friendly is presence of a clear assertion. In our case we expect that next_sequence_value is what will define the primary key value for us, so perhaps let's add an explicit expectation by making next_sequence_value constant and asserting for the actual value?

For example:

def next_sequence_value
  123_456
end

def test_create_prefetches_primary_key_value
  post = assert_nothing_raised { Post.create! }
  assert_equal 123_456, post.id
end

We can use one of the existing AR test models, Post for example - https://github.com/rails/rails/blob/f5a1075e71b79eb7eb0f4f6323468ef9c7286ee9/activerecord/test/models/post.rb#L3
but we could define a subclass which defines the prefetch behavior:

  class PostWithPrefetchedPk < Post
    class << self
      def prefetch_primary_key?
        true
      end

      def next_sequence_value
        Random.rand(1000000)
      end
    end
  end

Or perhaps it's unnecessary to inherit from the existing one and our post can be a standalone model like

  class PostWithPrefetchedPk < ActiveRecord::Base
    class << self
      def prefetch_primary_key?
        true
      end

      def next_sequence_value
        Random.rand(1000000)
      end
    end
  end
end

but with a unique name to avoid conflicts

Great suggestions! The test can definitely be made more meaningful.

After doing some further research, it appears the assert_nothing_raised is deprecated (source: https://apidock.com/ruby/Test/Unit/Assertions/assert_nothing_raised).

Would it be okay to just add an explicit expectation for next_sequence_value?

It's okay to omit assert_nothing_raised since we will have a much better explicit assertion for the primary key value but just wanted to mention that it must be a wrong apidock link since assert_nothing_raised at least the one defined in Rails itself is not deprecated and available
https://api.rubyonrails.org/classes/ActiveSupport/Testing/Assertions.html#method-i-assert_nothing_raised

Thank you for the reply. What the article is saying is strange because I see assert_nothing_raised in other parts of the Rails codebase.

I thought the potential fix should wrap the id_value into an array like Array(id_value) since this is what _create_record expects. Not saying that the current solution is wrong, I haven't looked close enough

Sounds interesting. I'll look into this further.

From what I have though, my fix appears to be passing the tests.

Just my 2 cents: I think the question here boils down to "what do we do if returning and id_value are both passed?" IMO, the way it's been done here makes the most sense – I would personally expect that the method return the values for all columns specified in returning, not just the ID on its own in an array. I guess that technically opens up another question though: should the ID in the array when returning is passed be id_value or just be whatever the database returned? I personally would expect it to just be what the DB returned untouched, as is done here.

One issue I can see here is that insert() is a public method and technically Rails 7.1 established that id_value will take precedence over any other parameter passed and by changing it to favor returning: we are technically changing the API which can't be done in a bugfix.

However, we can put it as if Rails 7.1 made a mistake by favoring id_value and it supposed to favor returning: from the beginning. Favoring returning: in Rails 7.1 wouldn't have been a breaking change since existing usages will continue pass returning: nil keeping the priority of id_value

So personally I think we can go with this fix! We may want to adjust the documentation for the method though.

Speaking a bit ahead I feel like there is an option for future version of Rails to deprecate passing id_value
It feels excessive to pass id_value to this method which is not used for anything other than a return value.
Consumers can just use the id_value argument directly without relying on it to be returned.
But that's a different story

I understand your concern about changing the API. Changing the API seems to be necessary in order to fix this issue.

I like your idea of adjusting documentation of this API to make it more clear for users what it's returning.

I'm looking at creating a new PR to update documentation.

The new model can point to the existing posts table by defining self.table_name = "posts"
Usually there is no need to have a new table.

However, I wonder if in this we actually do want a new table with special handling for the id column. I assume we would want to drop the auto-increment property from our table since the value for the primary key will be pre-fetched. I assume this will represent a more realistic pk prefetching setup. However I have to admit I'm not sure how a real world db schema would look in this case.

But the schema definition needs to go to
https://github.com/rails/rails/blob/main/activerecord/test/schema/schema.rb#L974

Good point about removing the explicit schema definition.

I agree with your point about the new table being a more realistic setup, but for now I will follow your suggestion and have the new model point to the existing posts table.

Edit: After removing the schema definition, all checks are passing. Thank you for the suggestion!!

One issue I can see here is that insert() is a public method and technically Rails 7.1 established that id_value will take precedence over any other parameter passed and by changing it to favor returning: we are technically changing the API which can't be done in a bugfix.

However, we can put it as if Rails 7.1 made a mistake by favoring id_value and it supposed to favor returning: from the beginning. Favoring returning: in Rails 7.1 wouldn't have been a breaking change since existing usages will continue pass returning: nil keeping the priority of id_value

So personally I think we can go with this fix! We may want to adjust the documentation for the method though.

Speaking a bit ahead I feel like there is an option for future version of Rails to deprecate passing id_value
It feels excessive to pass id_value to this method which is not used for anything other than a return value.
Consumers can just use the id_value argument directly without relying on it to be returned.
But that's a different story