Fix tests in activerecord/test/cases/tasks/database_tasks_test.rb

[bogdanvlviv]

After #33637 some tests in activerecord/test/cases/tasks/database_tasks_test.rb
don't assert anything.
We used to stub ActiveRecord::Base::configurations method in those
tests like ActiveRecord::Base.stub(:configurations, @configurations) {}.
Since #33637 ActiveRecord::Base::configurations is a ActiveRecord::DatabaseConfigurations
object(not a Hash object) we can't do so anymore.
ActiveRecord::DatabaseConfigurations object builds during ActiveRecord::Base::configurations=.
We can replace ActiveRecord::Base.stub(:configurations, @configurations) {} to

begin
  old_configurations = ActiveRecord::Base.configurations
  ActiveRecord::Base.configurations = @configurations
  # ...
ensure
 ActiveRecord::Base.configurations = old_configurations
end

Also I fixed tests in activerecord/test/cases/tasks/legacy_database_tasks_test.rb
But currently It looks like duplication of
activerecord/test/cases/tasks/database_tasks_test.rb.
We should improve those tests or remove them.

I've tried (in activerecord/test/cases/tasks/legacy_database_tasks_test.rb file):

def with_stubbed_configurations
  old_configurations = ActiveRecord::Base.configurations.to_h
  ActiveRecord::Base.configurations = @configurations

  ActiveRecord::Base.stub(:configurations, ActiveRecord::Base.configurations.to_h) do
    yield
  end
ensure
  ActiveRecord::Base.configurations = old_configurations
end

but it causes erros in tests cases.

@eileencodes What exactly do we want to test in
activerecord/test/cases/tasks/legacy_database_tasks_test.rb file?

Related to #33637
/cc @eileencodes

[rails-bot]

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

[eileencodes]

I'm not super clear on why we prefer to stub some of these instead of just setting/unsetting in setup/teardown. Can you explain? In general I think these tests need to be re-written.

The legacy db tasks test is for testing the to_h version of the configurations.

[bogdanvlviv]

stubbing isn't preferable over setting/unsetting in setup/teardown, and vice versa. I just reverted changes were made in activerecord/test/cases/tasks/database_tasks_test.rb by #33637, and fixed the tests.
The problem is that after #33637 touched tests in activerecord/test/cases/tasks/database_tasks_test.rb they haven't been testing properly, and tests added to activerecord/test/cases/tasks/legacy_database_tasks_test.rb haven't been testing either. The main reason is
that occurances of ActiveRecord::Base.stub(:configurations, @configurations) { ... } were chananged to ActiveRecord::Base.configurations { ... } that is incorrect since block passed to ActiveRecord::Base::configurations isn't supposed to be executed:

ActiveRecord::Base::configurations do
  flunk # it'll be never executed
end

the same issues in the activerecord/test/cases/tasks/legacy_database_tasks_test.rb file. You can detect it by running

rails/activerecord$ bin/test test/cases/tasks/legacy_database_tasks_test.rb

Using sqlite3
Run options: --seed 55195

# Running:

................................

Finished in 0.058142s, 550.3800 runs/s, 0.0000 assertions/s.
32 runs, 0 assertions, 0 failures, 0 errors, 0 skips

As you can see 0 assertions.

I guessed you want tests ActiveRecord::Tasks::DatabaseTasks in activerecord/test/cases/tasks/legacy_database_tasks_test.rb file when ActiveRecord::Base::configurations returns ActiveRecord::Base.configurations.to_h version so I suggested in #33781 (comment) to achive this behavior by implementation

def with_stubbed_configurations
  old_configurations = ActiveRecord::Base.configurations.to_h
  ActiveRecord::Base.configurations = @configurations

  ActiveRecord::Base.stub(:configurations, ActiveRecord::Base.configurations.to_h) do
    yield
  end
ensure
  ActiveRecord::Base.configurations = old_configurations
end

but it causes erros in tests cases like

ActiveRecord::LegacyDatabaseTasksCreateAllTest#test_warning_for_remote_databases:
NoMethodError: undefined method `configs_for' for {"development"=>{"database"=>"my-db", "host"=>"my.server.tld"}}:Hash

I'm also not sure whether it should work in that case.

In general I think these tests need to be re-written.

Why do you think so, these tests react on changes in the rails codebase and that is ok?

[eileencodes]

Ok, yea I get why the legacy one fails - the rake tasks will still use the object version internally (and should) so the legacy test isn't that necessary anymore. It was when I had a previous API implemented but I ended up changing everything and forgetting that that test wasn't really necessary once I changed the API. Can you delete the legacy file and push up so it's one commit?

[bogdanvlviv]

Agreed. I removed activerecord/test/cases/tasks/legacy_database_tasks_test.rb file.