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

Cover Rails::DBConsole with tests #6012

Merged
merged 1 commit into from May 3, 2012
Merged

Cover Rails::DBConsole with tests #6012

merged 1 commit into from May 3, 2012

Conversation

avakhov
Copy link
Contributor

@avakhov avakhov commented Apr 27, 2012

Add tests for Rails::DBConsole. I tried to not modify original code, but I should add such changes:

  • extract find_cmd from body of start method (I think to put one method to another was just misprint)
  • Wrap global ENV changes (to not polute ENV in tests)
  • Wrap system exec

I executed all tests also and checked rails db with edge rails works well.

\cc @drogus
Piotr, I see that you covered Rails::Console with tests. What do you think about my commit? I want to suggest some fix in Rails commands, but it's better to test existing code before. If it looks fine I'll add tests for all commands also.

ENV['PGPORT'] = config["port"].to_s if config["port"]
ENV['PGPASSWORD'] = config["password"].to_s if config["password"] && include_password
exec(find_cmd('psql'), config["database"])
set_env('PGUSER', config["username"]) if config["username"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why extract this method? If all that it will do is assign a hash key to a value I prefer the see the assignment.

@avakhov
Copy link
Contributor Author

avakhov commented Apr 28, 2012

Rafael, thanks you for the feedback. I applied your fixes, rebased and checked the test.

drogus added a commit that referenced this pull request May 3, 2012
Cover Rails::DBConsole with tests
@drogus drogus merged commit b55b77f into rails:master May 3, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants