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

Integration bug with RSpec: can't alias :expect, :assert #6

Closed
dchelimsky opened this issue May 15, 2011 · 8 comments
Closed

Integration bug with RSpec: can't alias :expect, :assert #6

dchelimsky opened this issue May 15, 2011 · 8 comments

Comments

@dchelimsky
Copy link

The README says you can alias assert with expect when using RSpec. This actually doesn't work because rspec-expectations already defines expect (as an alias for lambda). The outcome is that all examples pass whether they should or not.

RSpec recently added a hook to configure the assertion/expectation library to be either rspec-expectations, stdlib (t/u assertions on 1.8, minitest assertion on 1.9), or both. I tried setting this to use stdlib so rspec-expectations wouldn't be loaded (and define expect, but then there were errors because wrong assigns failure_class the value RSpec::Expectations::ExpectationNotMetError, which lives (as its name suggests) in rspec-expectations.

Since this isn' t working as advertised, it'd be great if you changed the README to exclude the bit about aliasing assert with expect.

Once that's done, I'd like to get this working and am happy to collaborate with you to do so. The question is what belongs where? One possibility is for RSpec to expose a configuration option that turns off the lambda, expect alias. Another is for RSpec to expose a better extension point for this sort of integration - something like the mock framework adapters. I'm sure there are several other approaches we could take.

WDYT?

@alexch
Copy link
Collaborator

alexch commented May 15, 2011

README updated.

@dchelimsky
Copy link
Author

Thanks!

I found a way that works, but it feels a bit hack-ish to me:

require "rspec/expectations"
require "wrong/adapters/rspec"
RSpec.configuration.expect_with :stdlib
Wrong.config.alias_assert :expect

This bypasses requiring "rspec/matchers", which is where expect is defined, but includes "rspec/expectations", which is where RSpec::Expectations:: ExpectationNotMetError is defined.

Another alternative would be for Wrong to define its own error class: Wrong::AssertionFailedError. I actually think this would be the most truth-telling option (since it's not really an rspec, test/unit, or minitest failure), and would eliminate the dependency on RSpec. WDYT?

@dchelimsky
Copy link
Author

Maybe, in the interest of aligning with the lib's name and the wisdom of yoda: Wrong::YouGotIt?

@alexch
Copy link
Collaborator

alexch commented May 15, 2011

Wrong already has its own error class (you guessed it, Wrong::Assert::AssertionFailedError) but the RSpec adapter changes it to an RSpec::Expectations::ExpectationNotMetError so that RSpec reports the failure as an F and not an E. I think this is appropriate, and is actually one of the main features of the adapter.

But I don't see what that has to do with the expect-vs-expect problem. I see "def expect" in two RSpec files, example_methods.rb and block_aliases.rb, and both are

      def expect(&block)
        block.extend BlockAliases
      end

This seems pretty intrinsic to RSpec's whole DSL and I'm not sure why the example_methods one isn't working even though the block_aliases one is skipped since matchers.rb is skipped...

Regardless, it would be nice if users could use both Wrong and RSpec expectations in the same file, so maybe the right solution is to make Wrong's RSpec adapter either

  1. not allow alias_assert :expect
  2. use metaprogramming to unbind RSpec's 'expect' if someone called alias_assert :expect

#2 is ugly but easy:

module RSpec::Matchers
  remove_method(:expect)
end

alexch added a commit to alexch/wrong that referenced this issue May 15, 2011
@dchelimsky
Copy link
Author

The one in example_methods is from rspec-1, before mocks and expectations got extracted to their own gems.

I like the idea of allowing this alias, so option #1 seems a shame. With option #2 I'm concerned that people trying out wrong would get confused if their existing use of expect starts failing or raising errors. It could be documented, but the removal would be surprising.

What about something like this?

Wrong.config.alias_assert :expect, :override_rspec => true

This ^^ would work as expected, but without :override_rspec => true it would raise an error if expect is already defined. WDYT? Crazy?

@dchelimsky
Copy link
Author

... and of course the error message would instruct the user to add :override_rspec => true (or similar).

@alexch
Copy link
Collaborator

alexch commented May 15, 2011

I think it's not crazy enough!!!

Actually it sounds just fine. Glad we're on the same page. And that we could resolve this without patching RSpec.

@alexch
Copy link
Collaborator

alexch commented May 18, 2011

fixed in 4de18d4 and f8bdcc5

@alexch alexch closed this as completed May 18, 2011
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

No branches or pull requests

2 participants