Skip to content

Comments

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

Merged
kamipo merged 1 commit intorails:masterfrom
kamipo:table_options_ahead_of_force_cascade
Aug 27, 2017
Merged

Prefer to place a table options before force: :cascade#28005
kamipo merged 1 commit intorails:masterfrom
kamipo:table_options_ahead_of_force_cascade

Conversation

@kamipo
Copy link
Member

@kamipo kamipo commented Feb 14, 2017 •
edited
Loading

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
@kamipo
Prefer to place a table options before force: :cascade
edd652e 
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 •
edited
Loading

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
@yahonda
Prefer to place a table options before force: :cascade
ff41e5c 
Oracle enhanced adapter implements table comments options
Refer rails/rails#28005 and rsim#1439
@yahonda yahonda mentioned this pull request Aug 29, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@rafaelfranca rafaelfranca

Labels

activerecord

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@kamipo @yahonda @rafaelfranca @maclover7