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

Allow matched_index.unique to be truthy. #156

Closed
wants to merge 1 commit into from

Conversation

steveklabnik
Copy link
Contributor

Currently, there is an assumption that matched_index.unique will
be true and not truthy. This is not always the case. This can
cause tests to fail, even though they should pass. This allows
matched_index.unique to be truthy.

This is just a first go to see what you think, I'm more than happy to adjust, as I've never even used shoulda before, to be honest.

This was inspired by https://groups.google.com/forum/?fromgroups=#!topic/rubyonrails-core/MM3Lh2M8QBE , more information is there.

@mike-burns
Copy link
Member

That's a good catch, thanks. Is it possible to add a test for this?

@steveklabnik
Copy link
Contributor Author

I think so. Let me try.

@steveklabnik
Copy link
Contributor Author

Hm. The existing tests operate at a pretty high level, and I'm not sure what your style is of testing this kind of thing. Normally, I would stub matched_index to return a number; do you think this is possible?

@mike-burns
Copy link
Member

We try to avoid stubbing the object under test. How about stubbing something on ActiveRecord::Base.connection.indexes?

@steveklabnik
Copy link
Contributor Author

Sure thing. Let me just figure out how you guys stub things...

@steveklabnik
Copy link
Contributor Author

It looks like stub("ActiveRecord::Base", :connection => stub("Foo", :indexes => nil)) is the way to do it?

@mike-burns
Copy link
Member

We use mocha for this project, so that seems close. But I think it's:

columns = [] # whatever is needed to make this test fail
indexes = stub('indexes', :columns => columns)
ActiveRecord::Base.stubs(:connection).returns(stub('connection', :indexes => indexes))

@steveklabnik
Copy link
Contributor Author

Whew, this is looking nasty. Any thoughts? Am I on the right track?

    stub("ActiveRecord::Base", :connection => stub("Foo", :indexes => stub("bar", :columns => [:age])))

    db_connection = create_table 'superheros' do |table|
      table.integer :age
    end

    define_model_class 'Superhero'
    Superhero.new.should have_db_index(:age).unique(true)

@mike-burns
Copy link
Member

That all seems right except for the first line; the stub. Since we're referencing the ActiveRecord::Base object directly we stub it like this:

ActiveRecord::Base.stubs(:connection => ...)

@steveklabnik
Copy link
Contributor Author

Ahhh, I was trying to use AR::Base.stub and it threw an error, I need stubs.

I'm leaving for the Frozen Rails party at the moment, but I'll try to complete this in a few hours. ;)

@mike-burns
Copy link
Member

Oh I should do that too.

@steveklabnik
Copy link
Contributor Author

So the issue with doing that is that trying to define the table throws errors on the connection. Since it's now a mock.

:(

@mike-burns
Copy link
Member

Arhhhh. Right.

OK, stub the SUT, and hopefully the test's description will explain why we're doing such a crazy thing.

@steveklabnik
Copy link
Contributor Author

Updated. Lemme know what you think. Confirmed that it fails without my refactoring, passes with.

Currently, there is an assumption that `matched_index.unique` will
be `true` and not truthy. This is not always the case. This can
cause tests to fail, even though they should pass. This allows
`matched_index.unique` to be truthy.
@mike-burns
Copy link
Member

Merged, thanks!

@mike-burns mike-burns closed this Sep 21, 2012
@steveklabnik
Copy link
Contributor Author

Woot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants