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
Added notice when a database is successfully created or dropped. #24551
Conversation
Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @pixeltrix (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
@@ -107,8 +107,9 @@ def current_config(options = {}) | |||
def create(*arguments) | |||
configuration = arguments.first | |||
class_for_adapter(configuration['adapter']).new(*arguments).create | |||
$stdout.puts "Create database '#{configuration['database']}'" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Create --> Created
I am trying fix tests. I don't know how i can delete from test's logs this. Could you help me?
|
Can you push up your changes -- I'm not sure which tests you are trying to run above ^^ |
2dfd730
to
37856a4
Compare
@vipulnsward, i think PR should have label 'activerecord'. |
9063461
to
5a678b8
Compare
5a678b8
to
7e6b0bd
Compare
$stdout.expects(:puts).with("Dropped database 'my-app-db'").once | ||
|
||
ActiveRecord::Tasks::DatabaseTasks.drop @configuration | ||
end | ||
end | ||
|
||
class MySQLPurgeTest < ActiveRecord::TestCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
succsessfylly → successfully
To suppress that output from the test logs, you could introduce an ouput Then, in tests, set logger to That'll capture all output without needing to |
7e6b0bd
to
643555f
Compare
@jeremy, i have got log output:
|
@jeremy, will be correctly if i set in tests?
|
Give it a shot, @bogdanvlviv! How'd it work out for you? |
@jeremy, i am trying make better. |
7a7907b
to
ddf5198
Compare
I have done suppress the output from the test logs. Could this PR be revised? |
@@ -4,6 +4,28 @@ | |||
|
|||
*Sean Griffin* | |||
|
|||
* Added notice when database successfully created or dropped. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when a database is
@@ -15,6 +15,9 @@ def setup | |||
File.stubs(:exist?).returns(false) | |||
ActiveRecord::Base.stubs(:connection).returns(@connection) | |||
ActiveRecord::Base.stubs(:establish_connection).returns(true) | |||
|
|||
@original_stdout = $stdout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't see any use of the @original_stdout
instance variable.
Should we assert on the content of this variable instead of $stdout.expects(:puts)
as @jeremy mentioned?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Edouard-chin, yes, i dont use @original_stdout
. i defined this variable because of i think it is correctly save link to original $stdout
, as you can see i override $stdout
for tests in next line after defining @original_stdout
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set $stdout = @original_stdout
in teardown to ensure we don't hijack stdout for other tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeremy, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
71ccd78
to
1b2d9c7
Compare
@bogdanvlviv To revise the PR, you can rebase your branch against master ( |
Could this PR be merged? |
@@ -8,6 +8,9 @@ def setup | |||
ActiveRecord::Tasks::MySQLDatabaseTasks.stubs(:new).returns @mysql_tasks | |||
ActiveRecord::Tasks::PostgreSQLDatabaseTasks.stubs(:new).returns @postgresql_tasks | |||
ActiveRecord::Tasks::SQLiteDatabaseTasks.stubs(:new).returns @sqlite_tasks | |||
|
|||
@original_stdout = $stdout | |||
$stdout = StringIO.new |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you setting $stdout
back to @original_stdout
somewhere (def teardown …
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I am not but it is good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set $stdout = @original_stdout in teardown to ensure we don't hijack stdout for other tests.
Done!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do a one-line multiple assignment to make this a little more concise:
$stdout, @original_stdout = StringIO.new, $stdout
1b2d9c7
to
5abde0b
Compare
@jeremy, could we finish this PR? |
|
||
def teardown | ||
$stdout = @original_stdout | ||
@original_stdout = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to set this to nil. Just set stdout back to its original value.
5abde0b
to
2686130
Compare
@jeremy, thanks for review! I made changes. |
Great work @bogdanvlviv. Thank you for improving this! |
Thanks @jeremy! |
👍 |
Added notice when a database is successfully created or dropped.
Example:
Changed older notices
blog_development already exists
toDatabase 'blog_development' already exists
andCouldn't drop blog_development
toCouldn't drop database 'blog_development'
.