Stub with minitest not mocha #33337

utilum · 2018-07-10T21:49:25Z

Step 4 in #33162

rails-bot · 2018-07-10T21:49:35Z

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

rafaelfranca · 2018-07-10T22:01:07Z

activerecord/test/cases/tasks/database_tasks_test.rb

  end

  class DatabaseTasksCreateCurrentTest < ActiveRecord::TestCase
    def setup
      @configurations = {
        "development" => { "database" => "dev-db" },
        "test"        => { "database" => "test-db" },
-        "production"  => { "url" => "prod-db-url" }
+        "production"  => { "url"      => "prod-db-url" }


we don't need to align this. It is better to not do.

rafaelfranca · 2018-07-10T22:01:22Z

activerecord/test/cases/tasks/database_tasks_test.rb

-        ActiveSupport::StringInquirer.new("development")
-      )
+      with_stubbed_configurations_establish_connection do
+


Thank you. Fixed. Also found and fixed some more here and in the postgresql task.

rafaelfranca · 2018-07-10T22:01:32Z

activerecord/test/cases/tasks/database_tasks_test.rb

-      ActiveRecord::Tasks::DatabaseTasks.expects(:create).
-        with("database" => "secondary-test-db")
+      with_stubbed_configurations_establish_connection do
+


rafaelfranca · 2018-07-10T22:01:38Z

activerecord/test/cases/tasks/database_tasks_test.rb

-      ActiveRecord::Tasks::DatabaseTasks.expects(:create).
-        with("url" => "secondary-prod-db-url")
+      with_stubbed_configurations_establish_connection do
+


rafaelfranca · 2018-07-10T22:01:46Z

activerecord/test/cases/tasks/database_tasks_test.rb

-        ActiveSupport::StringInquirer.new("development")
-      )
+      with_stubbed_configurations_establish_connection do
+


rafaelfranca · 2018-07-10T22:01:56Z

activerecord/test/cases/tasks/database_tasks_test.rb

-      )
+
+      with_stubbed_configurations_establish_connection do
+


bogdanvlviv · 2018-07-10T22:24:13Z

Also remove two stub return values following comments on #33326

@utilum Those stubbed returns were removed in 7fc499d.

utilum · 2018-07-10T22:57:38Z

Thank you @bogdanvlviv ... so quick I did not notice ;)

bogdanvlviv · 2018-07-11T09:34:39Z

activerecord/test/cases/tasks/database_tasks_test.rb

+        assert_raise(ActiveRecord::ProtectedEnvironmentError) do
+          ActiveRecord::Tasks::DatabaseTasks.check_protected_environments!
+        end
+


Is there reason adding this? Why do we need to assert it twice?

No reason whatsoever. It's a mix-up. Removing.

bogdanvlviv · 2018-07-11T09:40:46Z

activerecord/test/cases/tasks/database_tasks_test.rb

+
+    private
+
+      def with_stubbed_configuration_establish_connection


It is unused in this PR. Can we remove this changes?

Good catch. Removing.

bogdanvlviv · 2018-07-11T09:54:13Z

activerecord/test/cases/tasks/database_tasks_test.rb

+            ["database" => "dev-db"],
+            ["database" => "test-db"]
+          ],
+          returns: true


This return value seems redundant

Or maybe we should add this for all the kind of tests in order to ensure that dbs were created?

I am playing with it and wonder why changing this to returns: false doesn't cause the test failure

By looking at the implementation of the create method looks like we shouldn't expect returning any value.

bogdanvlviv · 2018-07-11T09:56:17Z

activerecord/test/cases/tasks/database_tasks_test.rb

+            ["database" => "dev-db"],
+            ["database" => "test-db"]
+          ],
+          returns: true


Seme here. Or maybe we should add this for all the kind of tests in order to ensure that dbs were created?

bogdanvlviv · 2018-07-11T10:06:36Z

activerecord/test/cases/tasks/database_tasks_test.rb

    ensure
      ENV["RAILS_ENV"] = old_env
    end

    def test_establishes_connection_for_the_given_environments
-      ActiveRecord::Tasks::DatabaseTasks.stubs(:create)
+      ActiveRecord::Tasks::DatabaseTasks.stub(:create, true) do


i think there should be nil instead of true

bogdanvlviv · 2018-07-11T10:12:14Z

activerecord/test/cases/tasks/database_tasks_test.rb

+          ActiveRecord::Tasks::DatabaseTasks.create_current(
+            ActiveSupport::StringInquirer.new("test")
+          )
+        end



✂️ empty line

Thank you, removing.

bogdanvlviv · 2018-07-11T10:13:19Z

activerecord/test/cases/tasks/database_tasks_test.rb

+          ActiveRecord::Tasks::DatabaseTasks.create_current(
+            ActiveSupport::StringInquirer.new("production")
+          )
+        end



✂️ empty line

Thank you, removing.

bogdanvlviv · 2018-07-11T10:14:06Z

activerecord/test/cases/tasks/database_tasks_test.rb

+            ActiveSupport::StringInquirer.new("development")
+          )
+        end
+


✂️ empty line

Thank you, removing.

bogdanvlviv · 2018-07-11T10:15:59Z

activerecord/test/cases/tasks/database_tasks_test.rb

+            ActiveSupport::StringInquirer.new("development")
+          )
+        end
+


✂️ empty line

Thank you, removing.

bogdanvlviv · 2018-07-11T10:18:21Z

activerecord/test/cases/tasks/database_tasks_test.rb

-      ActiveRecord::Tasks::DatabaseTasks.stubs(:create)
-
-      ActiveRecord::Base.expects(:establish_connection).with(:development)
+      ActiveRecord::Tasks::DatabaseTasks.stub(:create, true) do


should be nil instead of true

bogdanvlviv · 2018-07-11T10:21:46Z

activerecord/test/cases/tasks/database_tasks_test.rb

-      assert_not_called(ActiveRecord::Tasks::DatabaseTasks, :drop) do
-        ActiveRecord::Tasks::DatabaseTasks.drop_all
+      ActiveRecord::Base.stub(:configurations, @configurations) do
+        $stderr.stub(:puts, nil) do


this stub seems extra

bogdanvlviv · 2018-07-11T10:23:53Z

activerecord/test/cases/tasks/database_tasks_test.rb

-      ActiveRecord::Tasks::DatabaseTasks.expects(:drop).
-        with("database" => "test-db")
+      ActiveRecord::Base.stub(:configurations, @configurations) do
+


✂️ empty line

Thank you, removing.

bogdanvlviv · 2018-07-11T10:24:09Z

activerecord/test/cases/tasks/database_tasks_test.rb

+          ActiveRecord::Tasks::DatabaseTasks.drop_current(
+            ActiveSupport::StringInquirer.new("development")
+          )
+        end



✂️ empty line

Thank you, removing.

bogdanvlviv · 2018-07-11T10:24:42Z

activerecord/test/cases/tasks/database_tasks_test.rb

-        ActiveSupport::StringInquirer.new("development")
-      )
+      ActiveRecord::Base.stub(:configurations, @configurations) do
+


✂️ empty line

Thank you, removing.

bogdanvlviv · 2018-07-11T11:00:21Z

activerecord/test/cases/tasks/postgresql_rake_test.rb

-        )
+        ActiveRecord::Base.stubs(:establish_connection).returns(true)
+        ActiveRecord::Base.stub(:connection, @connection) do
+


✂️ extra empty line

bogdanvlviv · 2018-07-11T11:02:41Z

activerecord/test/cases/tasks/postgresql_rake_test.rb

-        )
-
-        ActiveRecord::Tasks::DatabaseTasks.purge @configuration
+        ActiveRecord::Base.stubs(:establish_connection).returns(true)


Can we get rid of ActiveRecord::Base.stubs(:establish_connection).returns(true) here?

We can get rid of the return value. Still need ActiveRecord::Base.respond_to?(:establish_connection) == true. Fixing.

bogdanvlviv · 2018-07-11T11:04:50Z

activerecord/test/cases/tasks/postgresql_rake_test.rb

+
+        def with_stubbed_connection_clear_active_connections
+          ActiveRecord::Base.stub(:connection, @connection) do
+            ActiveRecord::Base.stub(:clear_active_connections!, true) do


why do we need to stub clear_active_connections!?

We don't! Removing.

bogdanvlviv · 2018-07-11T11:05:22Z

activerecord/test/cases/tasks/postgresql_rake_test.rb

-        @connection.expects(:encoding)
-        ActiveRecord::Tasks::DatabaseTasks.charset @configuration
+        ActiveRecord::Base.stub(:connection, @connection) do
+          ActiveRecord::Base.stub(:clear_active_connections!, true) do


why do we need to stub clear_active_connections!?

We don't! Removing.

bogdanvlviv · 2018-07-11T11:06:32Z

activerecord/test/cases/tasks/postgresql_rake_test.rb

-        @connection.expects(:collation)
-        ActiveRecord::Tasks::DatabaseTasks.collation @configuration
+        ActiveRecord::Base.stub(:connection, @connection) do
+          ActiveRecord::Base.stub(:clear_active_connections!, true) do


why do we need to stub clear_active_connections!?

We don't! Removing.

bogdanvlviv · 2018-07-12T08:35:31Z

@utilum Please let me know when it is ready for review.

…e future We sometimes ask "✂️ extra blank lines" to a contributor in reviews like rails#33337 (comment). It is preferable to deal automatically without depending on manpower.

Step 4 in rails#33162

utilum · 2018-07-14T22:40:19Z

Thank you for the generous detailed comments. I believe I have responded to all, following suggestions in most cases, and offering an explanations where only partly followed, or not at all.

Remove extra stub of `ActiveRecord::Base::connection` in `activerecord/test/cases/tasks/mysql_rake_test.rb`. Remove extra stub of `File::exist?` in `activerecord/test/cases/tasks/sqlite_rake_test.rb`. `ActiveRecord::Base::establish_connection` shouldn't return `true` in test cases. Related to rails#33337.

bogdanvlviv · 2018-07-15T21:06:20Z

@utilum Excellent 😍! Thank you.
I just have few corrections of this PR, but since this is merged I opened new PR #33370.

Missed these in preparing rails#33337

A correct, but not obvious use of `ActiveSupport::Testing::MethodCallAssertions`, which might also have been part of rails#33337 or rails#33391.

rails-bot assigned schneems Jul 10, 2018

rafaelfranca reviewed Jul 10, 2018

View reviewed changes

utilum force-pushed the stub_with_minitest_not_mocha branch 2 times, most recently from 6172fca to e3295b3 Compare July 10, 2018 22:56

utilum mentioned this pull request Jul 10, 2018

Stop using Mocha #33162

Merged

8 tasks

bogdanvlviv reviewed Jul 11, 2018

View reviewed changes

utilum force-pushed the stub_with_minitest_not_mocha branch 3 times, most recently from 898b684 to ddec0cb Compare July 12, 2018 07:03

kamipo mentioned this pull request Jul 12, 2018

Enable Layout/EmptyLinesAroundBlockBody to reduce review cost in the future #33350

Merged

utilum force-pushed the stub_with_minitest_not_mocha branch 3 times, most recently from 092b0b7 to de4bfdd Compare July 12, 2018 22:05

Stub with Minitest instead of Mocha

837d603

Step 4 in rails#33162

utilum force-pushed the stub_with_minitest_not_mocha branch from de4bfdd to 837d603 Compare July 14, 2018 22:33

kamipo merged commit 5a71a77 into rails:master Jul 15, 2018

bogdanvlviv mentioned this pull request Jul 15, 2018

Clarify test cases #33370

Merged

utilum deleted the stub_with_minitest_not_mocha branch July 15, 2018 23:14

utilum added a commit to utilum/rails that referenced this pull request Jul 16, 2018

Replace some more Mocha#stub calls with Minitest

7afdc6c

Missed these in preparing rails#33337

utilum mentioned this pull request Jul 16, 2018

Remove/Replace some more calls to Mocha#stubs #33375

Merged

utilum mentioned this pull request Jul 20, 2018

Use MethodCallAssertions instead of Mocha part 2 #33409

Merged

utilum added a commit to utilum/rails that referenced this pull request Jul 22, 2018

Fix Mocha replacement that slipped out of rails#33337

a94d6ae

utilum added a commit to utilum/rails that referenced this pull request Jul 22, 2018

Replace Mocha#stubs with assert_called_with

d899375

A correct, but not obvious use of `ActiveSupport::Testing::MethodCallAssertions`, which might also have been part of rails#33337 or rails#33391.

Stub with minitest not mocha #33337

Stub with minitest not mocha #33337

Conversation

utilum commented Jul 10, 2018 • edited

rails-bot commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdanvlviv commented Jul 10, 2018

utilum commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdanvlviv Jul 11, 2018 • edited

Choose a reason for hiding this comment

bogdanvlviv Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdanvlviv Jul 11, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

utilum Jul 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bogdanvlviv commented Jul 12, 2018

utilum commented Jul 14, 2018

bogdanvlviv commented Jul 15, 2018

utilum commented Jul 10, 2018 •

edited

bogdanvlviv Jul 11, 2018 •

edited

bogdanvlviv Jul 11, 2018 •

edited

bogdanvlviv Jul 11, 2018 •

edited

utilum Jul 14, 2018 •

edited