search

SchemaPlus / schema_associations

ActiveRecord extension that automatically (DRY) creates associations based on the schema
Other
46 stars 8 forks source link

Confused by a test #8

Closed tovodeverett closed 11 years ago

tovodeverett commented 11 years ago

I'm confused a little by the spec in https://github.com/lomba/schema_associations/blob/master/spec/association_spec.rb#L555. That spec reads:

      it "should define associations before needed by relation" do
        Post.joins(:comments).all
        expect { Post.joins(:comments).all }.to_not raise_error

      end

I can't figure out why Post.joins(:comments).all is run both outside the expect block and inside it. If it's really supposed to happen twice, shouldn't both of them be expected to not raise errors. And if it only needs to run once, wouldn't the second line be sufficient? I've tracked this code back to https://github.com/lomba/schema_associations/commit/36c4100ba3995cb799e430e17c72bc1e9131d8cc, but it's just a fresh block of code there.

Thoughts?

I noticed it because I'm modifying that test in my Rails 4 clean up, and if we only really need to call it once in the expect block then I can fix that in my clean up.

ronen commented 11 years ago

oops. you're right, it should just happen once, inside the expect. must have been left behind during editing.

i probably didn't notice since the spec does do what it should anyway: if the bug is there the error will be raised and the spec will fail, and if the bug isn't there the spec will pass.

but still, yeah, if you're going in there and cleaning things up you may as well fix it.

thanks