Drop custom tables after each test. #14400

Merged
merged 1 commit into from Mar 20, 2014

Conversation

Projects
None yet
5 participants
Contributor

tgxworld commented Mar 16, 2014

This is a step to prevent side effects when running tests. This will allow us to run them in random order.

Member

chancancode commented Mar 16, 2014

cc @senny

gozzoo commented Mar 16, 2014

dvvzx

Member

senny commented Mar 16, 2014

@tgxworld there should be a with_example_table helper somewhere, maybe you can introduce something similar, or pull it out into a context that other tests can access.

@senny senny and 1 other commented on an outdated diff Mar 16, 2014

...ecord/test/cases/adapters/mysql/mysql_adapter_test.rb
@@ -16,6 +16,10 @@ def setup
eosql
end
+ teardown do
+ @conn.exec_query('drop table if exists ex')
@senny

senny Mar 16, 2014

Member

teardown should only drop tables that setup creates.

@tgxworld

tgxworld Mar 16, 2014

Contributor
      def setup
        @conn = ActiveRecord::Base.connection
        @conn.exec_query('drop table if exists ex')
        @conn.exec_query(<<-eosql)
          CREATE TABLE `ex` (
            `id` int(11) auto_increment PRIMARY KEY,
            `number` integer,
            `data` varchar(255))
        eosql
      end

The setup actually creates it. I think I would probably want to remove @conn.exec_query('drop table if exists ex') from setup as well.

@senny

senny Mar 16, 2014

Member

ah yes! must have looked at the wrong file. Please go ahead and remove the drop table from setup.

On a side note, do all tests use that example table? If it's not a required setup for every table, we should create it on demand using a helper.

@senny senny and 1 other commented on an outdated diff Mar 16, 2014

...d/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb
@@ -71,6 +71,7 @@ def test_invalid_column
def teardown
@senny

senny Mar 16, 2014

Member

can we move this method close to the setup method?

@tgxworld

tgxworld Mar 16, 2014

Contributor

sure :)

Contributor

tgxworld commented Mar 16, 2014

@tgxworld there should be a with_example_table helper somewhere, maybe you can introduce something similar, or pull it out into a context that other tests can access.

It seems like with_example_table is only defined in postgresql_adapted_test. I could move the method into the helpers file but that would add to this # TODO: Move all these random hacks into the ARTest namespace and into the support/ dir :P.

I'm thinking of doing something like this

module ActiveRecord
  module HelperMethods
    def with_example_table
      # do something
    end
  end
end

It probably wouldn't be so straight forward but any thoughts?

Thanks!

Member

senny commented Mar 16, 2014

@tgxworld you can create a module in support called DDLHelpers. The test-cases, which need the method will have to include the module explicitly.

@tgxworld tgxworld and 1 other commented on an outdated diff Mar 17, 2014

activerecord/test/support/ddl_helpers.rb
@@ -0,0 +1,8 @@
+module DdlHelpers
+ def with_example_table(connection, table_name, definition = nil)
+ connection.exec_query("CREATE TABLE #{table_name}(#{definition})")
+ yield connection
+ ensure
+ connection.exec_query("DROP TABLE #{table_name}")
+ end
+end
@tgxworld

tgxworld Mar 17, 2014

Contributor

@senny This is still a WIP but I have a few questions to clarify.

  1. Is the above helper method what you had in mind?
  2. Are there any guidelines on how many test should be sharing a certain variable before it can be defined within setup?
  3. This was not really mentioned in the style guide but is there a limit to the length of each line? If so, how should I indent the long lines. I usually do something like this:
def test_some_name
  with_example_table(
    connection, 
    'ex',
    'internet integer PRIMARY KEY AUTOINCREMENT, number integer not null'
  ) do |connection|
    # assert_something
  end
end 

Thanks in advance!

@senny

senny Mar 17, 2014

Member

@tgxworld :

  1. Yes something like that. You should put the connection variable at the end and assign it ActiveRecord::Base.connection as default. This should be good for most cases. Also I don't think we need to yield the connection. It's just an argument passed in and the caller will have access to it already. A block with no arguments should be good.
  2. There are no hard guidelines. I try to only put things in setup that absolutely need to happen for each case. Otherwise the setup method quickly becomes bloated. It also slows down the tests for no reason. There are two things you can do, when many test-cases share the same setup but not all of them:
    1. Create a new TestCase class with the setup and move the tests over
    2. Introduce a helper to easily create the setup within each test.
  3. We do not have a hard limit on Ruby files. I try to avoid really long lines and make it visually appealing. We do wrap *.md files to 80 chars.
@tgxworld

tgxworld Mar 18, 2014

Contributor

Also I don't think we need to yield the connection. It's just an argument passed in and the caller will have access to it already. A block with no arguments should be good.

I had to call it because most of the test will execute queries based on the connection. Technically we could use the @conn or @connection which are defined in setup but I don't think that is a good practice since the instance variables are subjected to changes.

@senny

senny Mar 18, 2014

Member

Please do not yield the connection. If it's passed in, then the caller will have access to it. For example through @connection. There is no need to yield it to the block.

@tgxworld

tgxworld Mar 18, 2014

Contributor

I understand that it will still have access to the variable but I think the problem is that the connection being yielded is now dependent on a variable outside of the helper method which I don't think is a good practice.

def test_pk_and_sequence_for_with_non_standard_primary_key
  with_example_table(
    'ex_with_non_standard_pk',
    '`code` INT(11) auto_increment, PRIMARY KEY  (`code`)'
  ) do

    pk, seq = @conn.pk_and_sequence_for('ex_with_non_standard_pk')
    assert_equal 'code', pk
    assert_equal @conn.default_sequence_name('ex_with_non_standard_pk', 'code'), seq
  end
end

Taking the above test for instance, connection is currently set to the default of ActiveRecord::Base.connection in the helper method. In this case, @conn = ActiveRecord::Base.connection which works just fine. However, there is no direct relationship with the connection yielded within the helper method unless it is passed in as a variable. Therefore, if @conn is changed, the tests will break. Another suggestion I have is to not set the default connection to ActiveRecord::Base.connection and have the user explicitly pass it in. I think that would improve readability and avoid confusions like this

senny self-assigned this Mar 17, 2014

@tgxworld tgxworld commented on an outdated diff Mar 18, 2014

...d/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb
end
def test_select_rows_logged
- sql = "select * from items"
- name = "foo"
-
- assert_logged([[sql, name, []]]) do
- @conn.select_rows sql, name
+ with_example_table(
+ 'items', 'id integer PRIMARY KEY AUTOINCREMENT, number integer', @conn
+ ) do |connection|
+
+ sql = "select * from items"
+ name = "foo"
+ connection.select_rows sql, name
+ assert_equal [sql, name, []], @subscriber.logged[1]
@tgxworld

tgxworld Mar 18, 2014

Contributor

I updated the test to not use assert_logged since I need access to @subscriber.logged[1]. This is because the query to create the table is logged when assert_logged yields within the with_example_table.

@tgxworld tgxworld commented on an outdated diff Mar 18, 2014

...d/test/cases/adapters/sqlite3/sqlite3_adapter_test.rb
- eosql
- assert_equal %w{ items people }.sort, @conn.tables.sort
+ with_example_table(
+ 'items', 'id integer PRIMARY KEY AUTOINCREMENT, number integer', @conn
+ ) do |connection|
+
+ assert_equal %w{ items }, connection.tables
+ with_example_table(
+ 'people',
+ 'id integer PRIMARY KEY AUTOINCREMENT, number integer',
+ connection
+ ) do
+
+ assert_equal %w{ items people }.sort, connection.tables.sort
+ end
+ end
@tgxworld

tgxworld Mar 18, 2014

Contributor

I had to nest with_example_table to replicate the original test

Contributor

tgxworld commented Mar 18, 2014

@senny updated :)

@senny senny commented on an outdated diff Mar 18, 2014

...ecord/test/cases/adapters/mysql/mysql_adapter_test.rb
@@ -38,31 +40,43 @@ def test_client_encoding
end
def test_exec_insert_number
- insert(@conn, 'number' => 10)
+ with_example_table(
+ 'ex',
+ '`id` int(11) auto_increment PRIMARY KEY, `number` integer, `data` varchar(255)'
+ ) do |connection|
@senny

senny Mar 18, 2014

Member

shouldn't this use @conn?

@senny senny and 1 other commented on an outdated diff Mar 18, 2014

...ecord/test/cases/adapters/mysql/mysql_adapter_test.rb
@@ -25,8 +21,14 @@ def test_bad_connection_mysql
end
def test_valid_column
- column = @conn.columns('ex').find { |col| col.name == 'id' }
- assert @conn.valid_type?(column.type)
+ with_example_table(
+ 'ex',
+ '`id` int(11) auto_increment PRIMARY KEY, `number` integer, `data` varchar(255)'
+ ) do |connection|
@senny

senny Mar 18, 2014

Member

This should probably use @conn as it did before.

@tgxworld

tgxworld Mar 18, 2014

Contributor

You should put the connection variable at the end and assign it ActiveRecord::Base.connection as default.

O it doesn't need to since @conn = ActiveRecord::Base.connection which I've updated to be the default connection in the helper method as per your comment above.

Member

senny commented Mar 18, 2014

@tgxworld I just skimmed the changes and it looks like you mixed up the connection usage. Please make sure that every test is using the same connection as before. It's important that such refactorings do not modify the meaning of the test.

Member

senny commented Mar 18, 2014

@tgxworld This change is getting out of hand and I'm loosing the overview here. The diff says +469 −303... can you please make the helper extraction within a limited scope. This will allow us to reason about it again. We will have to make this refactoring in more than one big step.

You're concern about the connection is valid and I think we can make the parameter required to reduce confusion. Looking at the tests from a birds-eye-view I think this patch introduces lots of duplication in table definition and we should probably separate the tests into separate test-cases. This will give us the opportunity to use the setup method for common ground. But again, this can't happen in the current scope. We need to start small and look closer.

Thank you for your work. Let me know when you have the extraction in a limited scope ~ 10 tests.

Contributor

tgxworld commented Mar 18, 2014

@senny Got it. If I understand correctly I've break them down into probably 4 different pull requests for each database type?

EDIT: O ok I think I know what you mean :P

Member

senny commented Mar 18, 2014

@tgxworld let's start with the smallest first, and do the helper extraction. There we can discuss signature and see how it affects the existing cases. We can then move on to other test cases that would also benefit from the new helper.

Contributor

tgxworld commented Mar 19, 2014

@senny Please view the updated commit. Let me know your thoughts :) Thanks!

Member

senny commented Mar 19, 2014

@tgxworld thanks for updating this with a limited scope 💛

Member

senny commented Mar 19, 2014

In the current state this refactoring does make the helper available to other tests but it comes at a cost. We introduced a lot of duplication in the form of:

@connection,
 +          'ex',
 +          'id serial primary key, number integer, data character varying(255)'

I suggest to define a method that calls with_example_table and passes these arguments along. This should reduce duplication again. Something like:

def with_example_table(definition = 'id serial primary key, number integer, data character varying(255)', &block)
  super(@connection, 'ex', definition, &block)
end

@senny senny commented on an outdated diff Mar 19, 2014

activerecord/test/support/ddl_helpers.rb
@@ -0,0 +1,8 @@
+module DdlHelpers
@senny

senny Mar 19, 2014

Member

Rails usually uses the singular Helper. Like ApplicationHelper. Let's go with DdlHelper in that case.

Contributor

tgxworld commented Mar 20, 2014

@senny 👍 on reducing code duplication. Made the changes alot smaller. I wasn't sure how you wanted to review the rest but I pushed up another commit for the next test-case. I'm thinking of separating the changes up into different commits for you to review individually and squash them once the review is done. Do let me know if that is fine :)

Member

senny commented Mar 20, 2014

@tgxworld we are going to merge before continuing with further changes. Can you remove the MySQL commit again?

Member

senny commented Mar 20, 2014

I've reviewed the complete changset and I think we can merge it together as is. This makes the extraction more prominent because it's already being used in the MySQL tests. Let's wait with the rest of though until this PR is merged. Then we can have a look at further changes in a new PR.

@tgxworld The patch looks ready to merge but I noticed that Travis CI is failing with an error related to this change. Can you investigate? https://travis-ci.org/rails/rails/jobs/21150701

  1) Error:
ActiveRecord::ConnectionAdapters::MysqlAdapterTest#test_exec_insert_number:
ActiveRecord::StatementInvalid: Mysql::Error: Table 'ex' already exists: CREATE TABLE ex(`id` int(11) auto_increment PRIMARY KEY, `number` integer, `data` varchar(255))
    /home/travis/build/rails/rails/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb:423:in `query'
    /home/travis/build/rails/rails/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb:423:in `block in exec_without_stmt'
    /home/travis/build/rails/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:370:in `block in log'
    /home/travis/build/rails/rails/activesupport/lib/active_support/notifications/instrumenter.rb:20:in `instrument'
    /home/travis/build/rails/rails/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb:364:in `log'
    /home/travis/build/rails/rails/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb:422:in `exec_without_stmt'
    /home/travis/build/rails/rails/activerecord/lib/active_record/connection_adapters/mysql_adapter.rb:283:in `exec_query'
    /home/travis/build/rails/rails/activerecord/test/support/ddl_helper.rb:3:in `with_example_table'
    /home/travis/build/rails/rails/activerecord/test/cases/adapters/mysql/mysql_adapter_test.rb:138:in `with_example_table'
    /home/travis/build/rails/rails/activerecord/test/cases/adapters/mysql/mysql_adapter_test.rb:39:in `test_exec_insert_number'
@tgxworld tgxworld Extract with_example_table into helper method.
This setups the helper method which other tests can benefit from.
79405a0
Contributor

tgxworld commented Mar 20, 2014

@senny My bad, I didn't realize the build failed. Basically, the other mysql tests are not cleaning up properly. I've removed the mysql commit since I think it is better we merge this separately. I'll create another pull request for the changes for mysql.

@senny senny added a commit that referenced this pull request Mar 20, 2014

@senny senny Merge pull request #14400 from tgxworld/ensure_tables_are_dropped_aft…
…er_test

Drop custom tables after each test.
2ca3f33

@senny senny merged commit 2ca3f33 into rails:master Mar 20, 2014

1 check passed

default The Travis CI build passed
Details
Member

senny commented Mar 20, 2014

@tgxworld thanks. Feel free to mention me on the next PR.

@tgxworld tgxworld added a commit to tgxworld/rails that referenced this pull request Mar 20, 2014

@tgxworld tgxworld Clean up tables after each test.
Follow-Up to rails#14400

This ensures that all tables are removed after each test and thereby
allowing us to run the tests in a random order.
2a8db23

@tgxworld tgxworld added a commit to tgxworld/rails that referenced this pull request Mar 21, 2014

@tgxworld tgxworld Clean up tables after each test.
Follow-Up to rails#14400

This ensures that all tables are removed after each test and thereby
allowing us to run the tests in a random order.
0aba657

@tgxworld tgxworld added a commit to tgxworld/rails that referenced this pull request Mar 23, 2014

@tgxworld tgxworld Clean up tables after each test.
Follow-Up to rails#14400

This ensures that all tables are removed after each test and thereby
allowing us to run the tests in a random order.
1db36c4

@tgxworld tgxworld added a commit to tgxworld/rails that referenced this pull request Mar 25, 2014

@tgxworld tgxworld Refactor test to use DdlHelper.
Follow-Up to rails#14400

This ensures that all tables are removed after each test and thereby
allowing us to run the tests in a random order.
00bbb59

@tgxworld tgxworld added a commit to tgxworld/rails that referenced this pull request Mar 26, 2014

@tgxworld tgxworld Refactor test to use DdlHelper.
Follow-Up to rails#14400

This ensures that all tables are removed after each test and thereby
allowing us to run the tests in a random order.
d513a6b

@tgxworld tgxworld added a commit to tgxworld/rails that referenced this pull request Mar 27, 2014

@tgxworld tgxworld Refactor test to use DdlHelper.
Follow-Up to rails#14400

This ensures that all tables are removed after each test and thereby
allowing us to run the tests in a random order.
41a20f6

@tgxworld tgxworld added a commit to tgxworld/rails that referenced this pull request Mar 28, 2014

@tgxworld tgxworld Refactor test to use DdlHelper.
Follow-Up to rails#14400

This ensures that all tables are removed after each test and thereby
allowing us to run the tests in a random order.
47e6cb4

@tgxworld tgxworld added a commit to tgxworld/rails that referenced this pull request Mar 28, 2014

@tgxworld tgxworld Refactor test to use DdlHelper.
Follow-Up to rails#14400

This ensures that all tables are removed after each test and thereby
allowing us to run the tests in a random order.
c4503c7

@tgxworld tgxworld added a commit to tgxworld/rails that referenced this pull request Mar 28, 2014

@tgxworld tgxworld Refactor test to use DdlHelper.
Follow-Up to rails#14400

This ensures that all tables are removed after each test and thereby
allowing us to run the tests in a random order.
b6b6293

@tgxworld tgxworld added a commit to tgxworld/rails that referenced this pull request Mar 29, 2014

@tgxworld tgxworld Refactor test to use DdlHelper.
Follow-Up to rails#14400

This ensures that all tables are removed after each test and thereby
allowing us to run the tests in a random order.
d67a699

@tgxworld tgxworld added a commit to tgxworld/rails that referenced this pull request Mar 29, 2014

@tgxworld tgxworld Refactor test to use DdlHelper.
Follow-Up to rails#14400

This ensures that all tables are removed after each test and thereby
allowing us to run the tests in a random order.
340174e

@tgxworld tgxworld added a commit to tgxworld/rails that referenced this pull request Mar 29, 2014

@tgxworld tgxworld Refactor test to use DdlHelper.
Follow-Up to rails#14400

This ensures that all tables are removed after each test and thereby
allowing us to run the tests in a random order.
0264762

@eric-chahin eric-chahin added a commit to eric-chahin/rails that referenced this pull request Apr 19, 2014

@tgxworld @eric-chahin tgxworld + eric-chahin Clean up tables after each test.
Follow-Up to rails#14400

This ensures that all tables are removed after each test and thereby
allowing us to run the tests in a random order.
100cd68
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment