Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unnecessary Mocha stubs #33309

Merged
merged 1 commit into from
Jul 9, 2018

Conversation

utilum
Copy link
Contributor

@utilum utilum commented Jul 6, 2018

Step 1 in #33162

While modifying tests to replace use of Mocha it became evident that some calls to Mocha#stub and Mocha#stubs do not have any impact on the test case. The same is true of some calls to returns on such objects. This patch removes both.

I speculate that the setup for one test was sometimes copied from onto another without verifying necessity. Tracing each of these instances to its origins and determining the moment it became unnecessary is fascinating, but a lot of work.

For example, @connection = stub("Connection", create_database: true) was introduced six years ago as part of quite a refactoring. I'd need a whole new stack to verify it was necessary at the time, and it's gone through several changes since.

So I did not complete the historical research.

@rails-bot
Copy link

r? @schneems

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

@utilum utilum mentioned this pull request Jul 6, 2018
8 tasks
@bogdanvlviv
Copy link
Contributor

I also found few unnecessary Mocha stubs on the current branch.
Let's apply it too and see if the tests fail
here is diff:

diff --git a/activerecord/test/cases/tasks/mysql_rake_test.rb b/activerecord/test/cases/tasks/mysql_rake_test.rb
index 6cddfaefeb..d39122d9ef 100644
--- a/activerecord/test/cases/tasks/mysql_rake_test.rb
+++ b/activerecord/test/cases/tasks/mysql_rake_test.rb
@@ -77,7 +77,6 @@ def test_create_when_database_exists_outputs_info_to_stderr

     class MysqlDBCreateWithInvalidPermissionsTest < ActiveRecord::TestCase
       def setup
-        @connection    = stub("Connection", create_database: true)
         @error         = Mysql2::Error.new("Invalid permissions")
         @configuration = {
           "adapter"  => "mysql2",
@@ -86,7 +85,6 @@ def setup
           "password" => "wossname"
         }

-        ActiveRecord::Base.stubs(:connection).returns(@connection)
         ActiveRecord::Base.stubs(:establish_connection).raises(@error)

         $stdout, @original_stdout = StringIO.new, $stdout
diff --git a/activerecord/test/cases/tasks/postgresql_rake_test.rb b/activerecord/test/cases/tasks/postgresql_rake_test.rb
index 6be929d1af..2f41f6a556 100644
--- a/activerecord/test/cases/tasks/postgresql_rake_test.rb
+++ b/activerecord/test/cases/tasks/postgresql_rake_test.rb
@@ -141,7 +141,6 @@ def setup
         }

         ActiveRecord::Base.stubs(:connection).returns(@connection)
-        ActiveRecord::Base.stubs(:clear_active_connections!).returns(true)
         ActiveRecord::Base.stubs(:establish_connection).returns(true)
       end

@@ -219,7 +218,6 @@ def test_db_retrieves_collation

     class PostgreSQLStructureDumpTest < ActiveRecord::TestCase
       def setup
-        @connection    = stub(schema_search_path: nil, structure_dump: true)
         @configuration = {
           "adapter"  => "postgresql",
           "database" => "my-app-db"
@@ -355,7 +353,6 @@ def with_structure_dump_flags(flags)

     class PostgreSQLStructureLoadTest < ActiveRecord::TestCase
       def setup
-        @connection    = stub
         @configuration = {
           "adapter"  => "postgresql",
           "database" => "my-app-db"
diff --git a/activerecord/test/cases/tasks/sqlite_rake_test.rb b/activerecord/test/cases/tasks/sqlite_rake_test.rb
index 0594e304e3..c9e7badf76 100644
--- a/activerecord/test/cases/tasks/sqlite_rake_test.rb
+++ b/activerecord/test/cases/tasks/sqlite_rake_test.rb
@@ -45,8 +45,6 @@ def test_db_create_when_file_exists
       end

       def test_db_create_with_file_does_nothing
-        File.stubs(:exist?).returns(true)
-
         ActiveRecord::Base.expects(:establish_connection).never

         ActiveRecord::Tasks::DatabaseTasks.create @configuration, "/rails/root"

@utilum utilum force-pushed the remove_unnecessary_mocha_stubs branch from 87fa1f6 to 80d257a Compare July 7, 2018 08:41
@utilum
Copy link
Contributor Author

utilum commented Jul 7, 2018

Looks good! Thank you.

@kaspth
Copy link
Contributor

kaspth commented Jul 7, 2018

Why are these stubs not needed? Were they never needed?

Can you elaborate a bit about how you found out they weren't needed in your commit message?

Thanks for working on this, both of you!

Step 1 in rails#33162

[utilum + bogdanvlviv]
@utilum utilum force-pushed the remove_unnecessary_mocha_stubs branch from 80d257a to 8c1e172 Compare July 7, 2018 16:22
@utilum
Copy link
Contributor Author

utilum commented Jul 7, 2018

Following @bogdanvlviv 's additional deletions, I rescanned the code and found several more such cases. Just pushed.

@rafaelfranca rafaelfranca merged commit 2cc514a into rails:master Jul 9, 2018
@utilum utilum deleted the remove_unnecessary_mocha_stubs branch July 10, 2018 04:29
bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Jul 10, 2018
Remove returning of `false` value for stubbed `lock_thread=` methods
since there aren't any needs in it.

Remove unnecessary returning of `true` for stubbed `drop_database` method.
Follow up rails#33309.

Related to rails#33162, rails#33326.
utilum added a commit to utilum/rails that referenced this pull request Jul 16, 2018
Should have been removed in rails#33309.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants