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

Prefer to place a table options before force: :cascade #28005

Merged
merged 1 commit into from Aug 27, 2017

Conversation

kamipo
Copy link
Member

@kamipo kamipo commented Feb 14, 2017

I was added a table options after force: :cascade in #17569 for not
touching existing tests (reducing diff). But force: :cascade is not an
important information. So I prefer to place a table options before
force: :cascade.

@kamipo kamipo force-pushed the table_options_ahead_of_force_cascade branch from f513e93 to b8b6502 Compare June 18, 2017 03:55
@kamipo kamipo force-pushed the table_options_ahead_of_force_cascade branch from b8b6502 to 22e118b Compare July 9, 2017 11:52
@kamipo kamipo force-pushed the table_options_ahead_of_force_cascade branch from 22e118b to 92bfe11 Compare August 20, 2017 06:05
I was added a table options after `force: :cascade` in rails#17569 for not
touching existing tests (reducing diff). But `force: :cascade` is not an
important information. So I prefer to place a table options before
`force: :cascade`.
@kamipo kamipo force-pushed the table_options_ahead_of_force_cascade branch from 92bfe11 to edd652e Compare August 25, 2017 09:39
@kamipo kamipo changed the title Prefer to see a table options ahead of force: :cascade Prefer to place a table options before force: :cascade Aug 25, 2017
@kamipo kamipo merged commit 376e19c into rails:master Aug 27, 2017
@kamipo kamipo deleted the table_options_ahead_of_force_cascade branch August 27, 2017 13:16
@yahonda
Copy link
Member

yahonda commented Aug 28, 2017

I'm working on implementing this pull request into Oracle enhanced adapter. The implementation itself is easy but I'm having some difficulty to find failing test cases.

Created a https://github.com/yahonda/rails/tree/diag28005 branch which just reverts ActiveRecord::SchemaDumper logic and keep the updated tests as they are. I expected these test failed but actually the passed.

$ for i in sqlite3 mysql2 postgresql; do echo $i; ARCONN=$i bin/test test/cases/comment_test.rb:104 -v; done
sqlite3
Using sqlite3
Run options: -v --seed 36838

# Running:


Finished in 0.001261s, 0.0000 runs/s, 0.0000 assertions/s.

0 runs, 0 assertions, 0 failures, 0 errors, 0 skips
mysql2
Using mysql2
Run options: -v --seed 33639

# Running:

CommentTest#test_schema_dump_with_comments = 0.44 s = .

Finished in 0.452178s, 2.2115 runs/s, 30.9613 assertions/s.

1 runs, 14 assertions, 0 failures, 0 errors, 0 skips
postgresql
Using postgresql
Run options: -v --seed 20944

# Running:

CommentTest#test_schema_dump_with_comments = 0.45 s = .

Finished in 0.461127s, 2.1686 runs/s, 30.3604 assertions/s.

1 runs, 14 assertions, 0 failures, 0 errors, 0 skips
$ for i in sqlite3 mysql2 postgresql; do echo $i; ARCONN=$i bin/test test/cases/schema_dumper_test.rb:67 -v; done
sqlite3
Using sqlite3
Run options: -v --seed 15255

# Running:

SchemaDumperTest#test_schema_dump_uses_force_cascade_on_create_table = 0.19 s = .

Finished in 0.219432s, 4.5572 runs/s, 9.1144 assertions/s.

1 runs, 2 assertions, 0 failures, 0 errors, 0 skips
mysql2
Using mysql2
Run options: -v --seed 40232

# Running:

SchemaDumperTest#test_schema_dump_uses_force_cascade_on_create_table = 0.33 s = .

Finished in 0.352691s, 2.8353 runs/s, 5.6707 assertions/s.

1 runs, 2 assertions, 0 failures, 0 errors, 0 skips
postgresql
Using postgresql
Run options: -v --seed 25193

# Running:

SchemaDumperTest#test_schema_dump_uses_force_cascade_on_create_table = 0.34 s = .

Finished in 0.359240s, 2.7837 runs/s, 5.5673 assertions/s.

1 runs, 2 assertions, 0 failures, 0 errors, 0 skips
$

Would you give me some advice how to change these tests fail if ActiveRecord::SchemaDumper changes reverted?

@kamipo
Copy link
Member Author

kamipo commented Aug 29, 2017

This change is only affected to adapters that is supported dumping table options (:options, :comment). So sqlite3 adapter isn't affected from this change.

For failing test_schema_dump_with_comments.

--- a/activerecord/test/cases/comment_test.rb
+++ b/activerecord/test/cases/comment_test.rb
@@ -111,7 +111,7 @@ def test_schema_dump_with_comments
 
       # And check that these changes are reflected in dump
       output = dump_table_schema "commenteds"
-      assert_match %r[create_table "commenteds",.*\s+comment: "A table with comment"], output
+      assert_match %r[create_table "commenteds",.*\s+comment: "A table with comment", force: :cascade do |t|$], output
       assert_match %r[t\.string\s+"name",\s+comment: "Comment should help clarify the column purpose"], output
       assert_match %r[t\.string\s+"obvious"\n], output
       assert_match %r[t\.string\s+"content",\s+comment: "Whoa, content describes itself!"], output

For failing test_schema_dump_uses_force_cascade_on_create_table, this is only affected to mysql2 adapter, isn't affected to sqlite3 and postgresql adapters.

--- a/activerecord/test/cases/schema_dumper_test.rb
+++ b/activerecord/test/cases/schema_dumper_test.rb
@@ -66,7 +66,7 @@ def test_schema_dump
 
   def test_schema_dump_uses_force_cascade_on_create_table
     output = dump_table_schema "authors"
-    assert_match %r{create_table "authors",.* force: :cascade}, output
+    assert_match %r{create_table "authors",.* force: :cascade do |t|$}, output
   end
 
   def test_schema_dump_excludes_sqlite_sequence

@yahonda
Copy link
Member

yahonda commented Aug 29, 2017

Thanks for the update. I got to know it depends each database adapter implements ActiveRecord::ConnectionAdapters::SchemaStatements#table_options . Oracle enhanced adapter just implemented it by rsim/oracle-enhanced#1439. I'm going to update Oracle enhanced adapter specs.

Thanks again for commenting the closed pull requests.

yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Aug 29, 2017
Oracle enhanced adapter implements table comments options
Refer rails/rails#28005 and rsim#1439
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants