Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Get logic out of db rake tasks, and into classes and objects #6761

Merged
merged 8 commits into from

8 participants

@pat

Right now, this is a work in progress. I'm pushing it here for a few reasons:

  • Feedback before I get too carried away is welcome.
  • Everyone gets a little more visibility of how (I think) rake tasks should be approached.
  • I'm short of time, so others may wish to contribute (and if so, get in touch).

But as to what's happening in these commits, essentially:

  • I want to get every single rake task that has a db prefix down to a single line of code
  • All the logic ends up in classes and objects.
  • Easier to read, easier to maintain, easier to test, easier to reuse.
  • Currently, I've done this for db:create, db:drop, db:create:all and db:drop:all.
  • My plan is for adapter-specific tasks to end up in DatabaseTasks, but then there'll also be MigrationTasks, SchemaTasks, FixturesTasks, TestTasks, and maybe one or two others.

And finally, my questions for right now:

  • Does anyone want to help?
  • Core committers, would something like this get accepted, or am I wasting my time?
pat added some commits
@pat pat A beginning of sorts.
Minimal implementation that supports db:create SQLite replacement
619453d
@pat pat Confirm connection is not established if file exists. 8aaeaf6
@pat pat db:create for MySQL now much cleaner. d297272
@pat pat db:create for PostgreSQL pulled out into a class. e678d41
@pat pat db:drop and some of db:test:purge. bca52b0
@pat pat One line db:create/db:drop tasks.
Now isn't that better?

And yes, I know that private has no impact on class methods - it's a visual distinction, not a technical one.
4422e03
@jonleighton
Collaborator

Does this work and pass the tests as-is? If so I think we may as well merge now and merge more later if/when you do more work, rather than bikeshed the design - IMO it's a clear improvement so I'd be happy to merge. Thanks!

@josevalim
Owner

I agree with @jonleighton . Thanks @freelancing-god !

@pat

Rightio - let me add a few more tests, and give the tasks a spin - and if you have any inspiration for an elegant way to have integration tests for this, I'd love to hear it.

@drogus
Collaborator

I like that direction. I was thinking about doing this after I needed to modify some of the tasks and realized that they don't have too much tests (or at that moment maybe even no tests) and there is now way to test it in isolation.

@pat

What's the right way to test ActiveRecord code that refers to the Rails module constant?

I was defining it if it wasn't defined in my new test files, and that's fine when they're run by themselves, but not the case when I run the entire set of AR tests.

@drogus
Collaborator

@freelancing-god that's what done in other sets of tests: https://github.com/rails/rails/blob/master/actionpack/test/abstract_unit.rb#L32, what errors do you see when you run entire AR suite?

@spastorino
Owner

@freelancing-god :+1: great work :heart: :heart: :heart:

@pat

@drogus (re)opening the module makes sense, but it doesn't seem to work - I get over 100 failing tests (running just test_sqlite3 in ./activerecord), and most are something like this:

113) Error:
test_serialize_should_be_reversible(SerializationTest):
NoMethodError: undefined method `backtrace_cleaner' for Rails:Module
    /Users/pat/Code/ruby/rails/activemodel/lib/active_model/mass_assignment_security/sanitizer.rb:37:in `backtrace'
    /Users/pat/Code/ruby/rails/activemodel/lib/active_model/mass_assignment_security/sanitizer.rb:47:in `block in process_removed_attributes'
    /Users/pat/.rvm/rubies/ruby-1.9.3-p125/lib/ruby/1.9.1/logger.rb:371:in `add'
    /Users/pat/.rvm/rubies/ruby-1.9.3-p125/lib/ruby/1.9.1/logger.rb:443:in `warn'
    /Users/pat/Code/ruby/rails/activemodel/lib/active_model/mass_assignment_security/sanitizer.rb:45:in `process_removed_attributes'
    /Users/pat/Code/ruby/rails/activemodel/lib/active_model/mass_assignment_security/sanitizer.rb:10:in `sanitize'
    /Users/pat/Code/ruby/rails/activemodel/lib/active_model/mass_assignment_security.rb:232:in `sanitize_for_mass_assignment'
    /Users/pat/Code/ruby/rails/activerecord/lib/active_record/attribute_assignment.rb:75:in `assign_attributes'
    /Users/pat/Code/ruby/rails/activerecord/lib/active_record/attribute_assignment.rb:36:in `attributes='
    /Users/pat/Code/ruby/rails/activemodel/lib/active_model/serializers/xml.rb:191:in `from_xml'
    /Users/pat/Code/ruby/rails/activerecord/test/cases/serialization_test.rb:30:in `block in test_serialize_should_be_reversible'
    /Users/pat/Code/ruby/rails/activerecord/test/cases/serialization_test.rb:28:in `each'
    /Users/pat/Code/ruby/rails/activerecord/test/cases/serialization_test.rb:28:in `test_serialize_should_be_reversible'
    /Users/pat/.rvm/gems/ruby-1.9.3-p125/gems/mocha-0.11.4/lib/mocha/integration/mini_test/version_230_to_262.rb:28:in `run'
    /Users/pat/Code/ruby/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:37:in `block in run'
    /Users/pat/Code/ruby/rails/activesupport/lib/active_support/callbacks.rb:347:in `_run_callbacks_70224487360180'
    /Users/pat/Code/ruby/rails/activesupport/lib/active_support/callbacks.rb:75:in `run_callbacks'
    /Users/pat/Code/ruby/rails/activesupport/lib/active_support/testing/setup_and_teardown.rb:36:in `run'

All I'm doing in the tests I've added that use Rails is the following, module Rails; end

@jonleighton
Collaborator

@freelancing-god personally I would inject the Rails constant as a dependency of the class under test. i.e.

class Foo
  def initialize(environment = Rails)
    @environment = environment
  end
end

You can then inject a mock in your test. I see you are stubbing methods on Rails in your tests which is also another valid approach (but I prefer injecting as it forces you to be explicit about the dependencies of the object).

@pat

Thanks @jonleighton, took the approach you suggested. Not thrilled at the result for one of the tests, but it'll do for the moment. Just given the rake tests a run in a local test app now...

@pat

@jonleighton @josevalim tests are green, and I ran through running db:create, db:drop, db:create:all and db:drop:all for each of SQLite, MySQL and PostgreSQL in a quick test application running off my branch, all worked smoothly. If you are keen to merge this in now, go for it.

@jonleighton jonleighton merged commit 7571408 into from
@jonleighton
Collaborator

Awesome, merged

@josevalim
Owner

@freelancing-god injecting Rails is fine, but if you can inject more granular configuration, it would be better.

@josevalim
Owner

I see you did what I just said, ignore me. :)

@bjeanes

@jonleighton: personally I would inject the Rails constant as a dependency of the class under test

:heart: :heart: :heart:

I wish more people thought like you, Jon :)

@carlosantoniodasilva

@freelancing-god really great work :+1: :heart:. From now on it'll be easier to improve these tasks.

@rubiii

awesome :) i recently had to dig through rails' rake db code and actually planned to refactor it.
thank you very much for this!

@pat

@rubiii no worries - although please, don't hold back from continuing on and cleaning up the other tasks (there's a lot of them!). I'll try to get through some more when I have the chance, but no idea when that'll be.

@rubiii

@freelancing-god i didn't take a look at the other rake tasks yet, but i hope i can contribute something soon.
thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 16, 2012
  1. @pat

    A beginning of sorts.

    pat authored
    Minimal implementation that supports db:create SQLite replacement
Commits on Jun 17, 2012
  1. @pat
  2. @pat
  3. @pat
  4. @pat

    db:drop and some of db:test:purge.

    pat authored
  5. @pat

    One line db:create/db:drop tasks.

    pat authored
    Now isn't that better?
    
    And yes, I know that private has no impact on class methods - it's a visual distinction, not a technical one.
  6. @pat
  7. @pat
Something went wrong with that request. Please try again.