aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorYasuo Honda <yasuo.honda@gmail.com>2019-04-30 22:42:59 +0000
committerYasuo Honda <yasuo.honda@gmail.com>2019-05-01 13:30:11 +0000
commit6030110f56229a123b9ba3219879cc5a544739d2 (patch)
treecd37619ff9cfe41740d6737510840f6eeb957830
parentfef174f5c524edacbcad846d68400e7fe114a15a (diff)
downloadrails-6030110f56229a123b9ba3219879cc5a544739d2.tar.gz
rails-6030110f56229a123b9ba3219879cc5a544739d2.tar.bz2
rails-6030110f56229a123b9ba3219879cc5a544739d2.zip
Remove database specific sql statements from SQLCounter
Every database executes different type of sql statement to get metadata then `ActiveRecord::TestCase` ignores these database specific sql statements to make `assert_queries` or `assert_no_queries` work consistently. Connection adapter already labels these statement by setting "SCHEMA" argument, this pull request makes use of "SCHEMA" argument to ignore metadata queries. Here are the details of these changes: * PostgresqlConnectionTest Each of PostgresqlConnectionTest modified just executes corresponding methods https://github.com/rails/rails/blob/fef174f5c524edacbcad846d68400e7fe114a15a/activerecord/lib/active_record/connection_adapters/postgresql/schema_statements.rb#L182-L195 ```ruby # Returns the current database encoding format. def encoding query_value("SELECT pg_encoding_to_char(encoding) FROM pg_database WHERE datname = current_database()", "SCHEMA") end # Returns the current database collation. def collation query_value("SELECT datcollate FROM pg_database WHERE datname = current_database()", "SCHEMA") end # Returns the current database ctype. def ctype query_value("SELECT datctype FROM pg_database WHERE datname = current_database()", "SCHEMA") end ``` * BulkAlterTableMigrationsTest mysql2 adapter executes `SHOW KEYS FROM ...` to see if there is an index already created as below. I think the main concerns of these tests are how each database adapter creates or drops indexes then ignoring `SHOW KEYS FROM` statement makes sense. https://github.com/rails/rails/blob/fef174f5c524edacbcad846d68400e7fe114a15a/activerecord/lib/active_record/connection_adapters/mysql/schema_statements.rb#L11 ```ruby execute_and_free("SHOW KEYS FROM #{quote_table_name(table_name)}", "SCHEMA") do |result| ``` * Temporary change not included in this commit to show which statements executed ```diff $ git diff diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb index 8e8ed494d9..df05f9bd16 100644 --- a/activerecord/test/cases/migration_test.rb +++ b/activerecord/test/cases/migration_test.rb @@ -854,7 +854,7 @@ def test_adding_indexes classname = ActiveRecord::Base.connection.class.name[/[^:]*$/] expected_query_count = { - "Mysql2Adapter" => 3, # Adding an index fires a query every time to check if an index already exists or not + "Mysql2Adapter" => 1, # Adding an index fires a query every time to check if an index already exists or not "PostgreSQLAdapter" => 2, }.fetch(classname) { raise "need an expected query count for #{classname}" @@ -886,7 +886,7 @@ def test_removing_index classname = ActiveRecord::Base.connection.class.name[/[^:]*$/] expected_query_count = { - "Mysql2Adapter" => 3, # Adding an index fires a query every time to check if an index already exists or not + "Mysql2Adapter" => 1, # Adding an index fires a query every time to check if an index already exists or not "PostgreSQLAdapter" => 2, }.fetch(classname) { raise "need an expected query count for #{classname}" $ ``` * Executed these modified tests ```ruby $ ARCONN=mysql2 bin/test test/cases/migration_test.rb -n /index/ Using mysql2 Run options: -n /index/ --seed 8462 F Failure: BulkAlterTableMigrationsTest#test_adding_indexes [/home/yahonda/git/rails/activerecord/test/cases/migration_test.rb:863]: 3 instead of 1 queries were executed. Queries: SHOW KEYS FROM `delete_me` SHOW KEYS FROM `delete_me` ALTER TABLE `delete_me` ADD UNIQUE INDEX `awesome_username_index` (`username`), ADD INDEX `index_delete_me_on_name_and_age` (`name`, `age`). Expected: 1 Actual: 3 bin/test test/cases/migration_test.rb:848 F Failure: BulkAlterTableMigrationsTest#test_removing_index [/home/yahonda/git/rails/activerecord/test/cases/migration_test.rb:895]: 3 instead of 1 queries were executed. Queries: SHOW KEYS FROM `delete_me` SHOW KEYS FROM `delete_me` ALTER TABLE `delete_me` DROP INDEX `index_delete_me_on_name`, ADD UNIQUE INDEX `new_name_index` (`name`). Expected: 1 Actual: 3 bin/test test/cases/migration_test.rb:879 .. Finished in 0.379245s, 10.5473 runs/s, 7.9105 assertions/s. 4 runs, 3 assertions, 2 failures, 0 errors, 0 skips $ ``` * ActiveRecord::ConnectionAdapters::Savepoints Left `self.ignored_sql` to ignore savepoint related statements because these SQL statements are not related "SCHEMA" ``` self.ignored_sql = [/^SAVEPOINT/, /^ROLLBACK TO SAVEPOINT/, /^RELEASE SAVEPOINT/] ``` https://github.com/rails/rails/blob/fef174f5c524edacbcad846d68400e7fe114a15a/activerecord/lib/active_record/connection_adapters/abstract/savepoints.rb#L10-L20 ```ruby def create_savepoint(name = current_savepoint_name) execute("SAVEPOINT #{name}") end def exec_rollback_to_savepoint(name = current_savepoint_name) execute("ROLLBACK TO SAVEPOINT #{name}") end def release_savepoint(name = current_savepoint_name) execute("RELEASE SAVEPOINT #{name}") end ```
-rw-r--r--activerecord/test/cases/adapters/postgresql/connection_test.rb6
-rw-r--r--activerecord/test/cases/migration_test.rb4
-rw-r--r--activerecord/test/cases/test_case.rb16
3 files changed, 7 insertions, 19 deletions
diff --git a/activerecord/test/cases/adapters/postgresql/connection_test.rb b/activerecord/test/cases/adapters/postgresql/connection_test.rb
index 210758f462..9494863a75 100644
--- a/activerecord/test/cases/adapters/postgresql/connection_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/connection_test.rb
@@ -26,19 +26,19 @@ module ActiveRecord
end
def test_encoding
- assert_queries(1) do
+ assert_queries(1, ignore_none: true) do
assert_not_nil @connection.encoding
end
end
def test_collation
- assert_queries(1) do
+ assert_queries(1, ignore_none: true) do
assert_not_nil @connection.collation
end
end
def test_ctype
- assert_queries(1) do
+ assert_queries(1, ignore_none: true) do
assert_not_nil @connection.ctype
end
end
diff --git a/activerecord/test/cases/migration_test.rb b/activerecord/test/cases/migration_test.rb
index 8e8ed494d9..255885b599 100644
--- a/activerecord/test/cases/migration_test.rb
+++ b/activerecord/test/cases/migration_test.rb
@@ -854,7 +854,7 @@ if ActiveRecord::Base.connection.supports_bulk_alter?
classname = ActiveRecord::Base.connection.class.name[/[^:]*$/]
expected_query_count = {
- "Mysql2Adapter" => 3, # Adding an index fires a query every time to check if an index already exists or not
+ "Mysql2Adapter" => 1, # mysql2 supports creating two indexes using one statement
"PostgreSQLAdapter" => 2,
}.fetch(classname) {
raise "need an expected query count for #{classname}"
@@ -886,7 +886,7 @@ if ActiveRecord::Base.connection.supports_bulk_alter?
classname = ActiveRecord::Base.connection.class.name[/[^:]*$/]
expected_query_count = {
- "Mysql2Adapter" => 3, # Adding an index fires a query every time to check if an index already exists or not
+ "Mysql2Adapter" => 1, # mysql2 supports dropping and creating two indexes using one statement
"PostgreSQLAdapter" => 2,
}.fetch(classname) {
raise "need an expected query count for #{classname}"
diff --git a/activerecord/test/cases/test_case.rb b/activerecord/test/cases/test_case.rb
index 5b25432dc0..78dc0a6d9f 100644
--- a/activerecord/test/cases/test_case.rb
+++ b/activerecord/test/cases/test_case.rb
@@ -107,19 +107,7 @@ module ActiveRecord
clear_log
- self.ignored_sql = [/^PRAGMA/, /^SELECT currval/, /^SELECT CAST/, /^SELECT @@IDENTITY/, /^SELECT @@ROWCOUNT/, /^SAVEPOINT/, /^ROLLBACK TO SAVEPOINT/, /^RELEASE SAVEPOINT/, /^SHOW max_identifier_length/, /^BEGIN/, /^COMMIT/]
-
- # FIXME: this needs to be refactored so specific database can add their own
- # ignored SQL, or better yet, use a different notification for the queries
- # instead examining the SQL content.
- oracle_ignored = [/^select .*nextval/i, /^SAVEPOINT/, /^ROLLBACK TO/, /^\s*select .* from all_triggers/im, /^\s*select .* from all_constraints/im, /^\s*select .* from all_tab_cols/im, /^\s*select .* from all_sequences/im]
- mysql_ignored = [/^SHOW FULL TABLES/i, /^SHOW FULL FIELDS/, /^SHOW CREATE TABLE /i, /^SHOW VARIABLES /, /^\s*SELECT (?:column_name|table_name)\b.*\bFROM information_schema\.(?:key_column_usage|tables)\b/im]
- postgresql_ignored = [/^\s*select\b.*\bfrom\b.*pg_namespace\b/im, /^\s*select tablename\b.*from pg_tables\b/im, /^\s*select\b.*\battname\b.*\bfrom\b.*\bpg_attribute\b/im, /^SHOW search_path/i, /^\s*SELECT\b.*::regtype::oid\b/im]
- sqlite3_ignored = [/^\s*SELECT name\b.*\bFROM sqlite_master/im, /^\s*SELECT sql\b.*\bFROM sqlite_master/im]
-
- [oracle_ignored, mysql_ignored, postgresql_ignored, sqlite3_ignored].each do |db_ignored_sql|
- ignored_sql.concat db_ignored_sql
- end
+ self.ignored_sql = [/^SAVEPOINT/, /^ROLLBACK TO SAVEPOINT/, /^RELEASE SAVEPOINT/]
attr_reader :ignore
@@ -132,7 +120,7 @@ module ActiveRecord
sql = values[:sql]
self.class.log_all << sql
- self.class.log << sql unless ignore.match?(sql)
+ self.class.log << sql unless values[:name] == "SCHEMA" || ignore.match?(sql)
end
end