notahat / machinist

Fixtures aren't fun. Machinist is.
MIT License
1.12k stars 134 forks source link

DataMapper (>= 1.1.0, and possibly earlier?) support #85

Closed d11wtq closed 13 years ago

d11wtq commented 13 years ago

100% of specs passing. Implementation clean/straightforward and not magic. In terms of what I had to do to make it work. Basically nothing. Just "translate" the AR stuff to DM code. Including the specs, which were what drove the implementation.

I had to namespace the fixtures under module DataMapperEnvironment in order to stop the specs failing due the fact classes User, Post etc already exist at the top-level namespace.

Hopefully you can merge this into the main project.

emmanuel commented 13 years ago

Well, I have almost identical updates in my fork, as well. I got change-happy and updated a few other things along the way, so these changes here are probably a better bet for pulling into the mainline than my pull request (notahat/machinist#79). I'll rebase my work on top of these from @d11wtq to separate out my 'extra' updates from the DM-support.

emmanuel commented 13 years ago

Hmm... I'm currently unable to create a pull request from my updated feature branch to you, @d11wtq, so perhaps you'd like to take a look at emmanuel/machinist@feature/datamapper and merge the two commits I added to your support.

d11wtq commented 13 years ago

Hi Emmanuel,

I've merged in those changes, thanks. I didn't like how ActiveRecord was in the top-level namespace and DM had to be in it's own. It makes sense to namespace them both :)

I think the reason you couldn't submit a pull request to me is because we're on divergent forks and your fork isn't from mine. Or something.

Pete, what's happening with Machinist? There's a fair bit of stuff in your pull queue with no feedback? ;)

Cheers,

Chris

notahat commented 13 years ago

This is awesome stuff! I appreciate all your work on it.

What do you think about making this into a separate machinist-dm gem (that depends on Machinist, obviously)? I'm not an active DM user, and it makes sense to me to have someone who is be responsible for maintaining the DM related code. (Also I'm pretty busy and I'm not giving Machinist the love it deserves, so it removes me as a bottleneck for DM-related patches.)

d11wtq commented 13 years ago

Evening Pete,

Yeah sure, no problem making this into a gem. @emmanuel, do you want to communicate further on this?

Chris

notahat commented 13 years ago

Ping me if you need a hand. I strongly recommend using bundler's gem making stuff. See the "bundle gem" command, and have a look at the way Machinist's Gemfile and machinist.gemspec are set up.

notahat commented 13 years ago

On Sun, Jun 5, 2011 at 3:25 PM, d11wtq < reply@reply.github.com>wrote:

What I'll do, is just check the return value of #save and manually raise the Exception if it fails. Exceptions are obviously desirable if your test fixtures aren't being created, so I guess that's a behaviour I should enforce.

Yep, completely agree with that, and it sounds like the right way to do it.

I had to namespace the fixtures under module DataMapperEnvironment in order to stop the specs failing due the fact classes User, Post etc already exist at the top-level namespace. That showed that where:

module DataMapperEnvironment
 class Post
   # ...
 end

 class User
   # ...
 end
end

Writing a blueprint:

include DataMapperEnvironment

Post.blueprint do
 user  { User.make!(:username => "foo") }
end

confuses things since it's looking for User in the wrong namespace. I had to fully-qualify the class name in the specs, but it's no big deal really. I don't know if there's a way around that.

Again, all sounds sensible.

Nice stuff!

d11wtq commented 13 years ago

What I'll do, is just check the return value of #save and manually raise the Exception if it fails. Exceptions are obviously desirable if your test fixtures aren't being created, so I guess that's a behaviour I should enforce.

Yep, completely agree with that, and it sounds like the right way to do it.

I actually resolved this one in a cleaner way. I didn't realise (at the time) that you can scope the raise_on_save_failure behaviour to an individual instance :)

I'm not sure when I'll get a chance to look at sorting the gem out, but hopefully during the week :)

emmanuel commented 13 years ago

@d11wtq -- I'd be happy to lend a hand with separating and gemifying the DM-specific code if you'd like help.

Otherwise, I'll happily update my Gemfile to use your new gem instead of your fork when the time comes :).

emmanuel commented 13 years ago

@d11wtq—Have you had a chance to extract this into a gem?

d11wtq commented 13 years ago

I haven't yet, sorry. I started working on it not long after this discussion, but ran into some odd issues when running the specs. I don't recall what those issues were and had my MacBook stolen a couple of weeks ago so don't still have that code, but I'll get onto this again. Thanks for the reminder! ;)

d11wtq commented 13 years ago

@emmanuel here's the initial work splitting it into a separate project (not in rubygems yet, as it's broken).

https://github.com/d11wtq/machinist-dm

I've run into the same issue that I ran into last time:

Failures:

  1) Machinist::DataMapper make! should make and save objects
     Failure/Error: post = DM::Post.make!
     NoMethodError:
       undefined method `outside_transaction' for #<Machinist::DataMapper::Blueprint:0x000000035a8040>
     # ./spec/data_mapper_spec.rb:23:in `block (3 levels) in <top (required)>'

  2) Machinist::DataMapper associations support should handle belongs_to associations
     Failure/Error: post = DM::Post.make!
     NoMethodError:
       undefined method `outside_transaction' for #<Machinist::DataMapper::Blueprint:0x00000003583740>
     # ./spec/data_mapper_spec.rb:43:in `block (3 levels) in <top (required)>'

  3) Machinist::DataMapper associations support should handle has_many associations
     Failure/Error: post = DM::Post.make!
     NoMethodError:
       undefined method `outside_transaction' for #<Machinist::DataMapper::Blueprint:0x00000003574470>
     # ./spec/data_mapper_spec.rb:55:in `block (3 levels) in <top (required)>'

  4) Machinist::DataMapper associations support should handle habtm associations
     Failure/Error: post = DM::Post.make!
     NoMethodError:
       undefined method `outside_transaction' for #<Machinist::DataMapper::Blueprint:0x00000003565420>
     # ./spec/data_mapper_spec.rb:72:in `block (3 levels) in <top (required)>'

  5) Machinist::DataMapper associations support should handle overriding associations
     Failure/Error: post = DM::Post.make!
     NoMethodError:
       undefined method `outside_transaction' for #<Machinist::DataMapper::Blueprint:0x0000000354ece8>
     # ./spec/data_mapper_spec.rb:89:in `block (3 levels) in <top (required)>'

Also, for some reason it doesn't work unless activesupport and i18n are included (I've added them as dependencies).

I'll come back to this tomorrow, unless you feel like poking at it ;)

d11wtq commented 13 years ago

Ah, I see. v2.0.0-beta1 and beta2 have this outside_transaction method on Blueprint, while master does not :)

d11wtq commented 13 years ago

Ok, done, pushed to rubygems :)