Rewritten rspec integration to use RSpec's configure for matchers loading #204

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

Contributor
hron commented Dec 10, 2012

I've tried to use RSpec.configure {|c| c.include ...} for loading shoulda matchers. This fixes random failures that are mentioned in #174. At least for my case. ;)

Member
gabebw commented Dec 10, 2012

I appreciate you fixing #174 (at least for your case :)).

However, we don't use RSpec features like this because this library is also used with thoughtbot/shoulda-context , which is used with Test::Unit. For that reason, we can't depend on RSpec as a runtime dependency in the way you're using it here.

@gabebw gabebw closed this Dec 10, 2012
Contributor
hron commented Dec 10, 2012

I think this part of code from lib/shoulda/matchers.rb is exactly to guard from using RSpec features unless they are not available:

if defined?(RSpec)
  require 'shoulda/matchers/integrations/rspec'
end

I am also pretty sure that if you want to integrate with some tool like RSpec you should use the interface they provide, instead of defining matchers inside RSpec::Matcher and pray that it should work. ;)

Contributor
hron commented Dec 10, 2012

@gabebw any comments on my remark? Thanks.

dnagir commented Dec 12, 2012

@gabebw I agree with @hron. Any chance it being merged?

I can confirm that it fixes the issue. And indeed for sure the proper RSpec interface should be used.

+1 for this fix. Otherwise we have some random failures in CI environment.

@gabebw gabebw reopened this Dec 12, 2012
Member
gabebw commented Dec 12, 2012

This looks good. If Travis passes, I'll merge it.

Thanks for being persistent, @hron :)

@regedarek regedarek referenced this pull request in thoughtbot/shoulda Dec 25, 2012
Closed

[Semaphoreapp]All Shoulda specs fails on random branches #221

Member
gabebw commented Dec 25, 2012

Merged!

@gabebw gabebw closed this Dec 25, 2012
dnagir commented Jan 15, 2013

When do you guys are planning to release (so that this fix can be included)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment