rspec-expectations no longer required by rspec-core #647

Closed
eregon opened this Issue Jul 12, 2012 · 13 comments

Comments

Projects
None yet
4 participants
Contributor

eregon commented Jul 12, 2012

Hello,

I guess this is expected behavior, but it caused some breakage between 2.10.1 and 2.11.

Having a file with:

RSpec::Matchers

(RSpec::Matchers is used in RSpec::Matchers.define for example, and this still happens after RSpec.configure)

and running it with rspec <file>, fails on latest RSpec. Doing a git bisect, I found 2e20683 which no longer requires rspec/expectations/handler.

require 'rspec' in spec_helper.rb solves this problem, but I did not expected it to be needed given I'm running the specs with the rspec command.
I understand this is to allow to use RSpec with other expectations frameworks, so this is probably fine as is.

This issue is there to mention the change and discuss about it.

Contributor

eregon commented Jul 12, 2012

The exact error is:

pwd/t.rb:1:in `<top (required)>': cannot load such file -- rspec/matchers (LoadError)
    from gems/rspec-core-2.11.0/lib/rspec/core/configuration.rb:780:in `load'
    from gems/rspec-core-2.11.0/lib/rspec/core/configuration.rb:780:in `block in load_spec_files'
    from gems/rspec-core-2.11.0/lib/rspec/core/configuration.rb:780:in `map'
    from gems/rspec-core-2.11.0/lib/rspec/core/configuration.rb:780:in `load_spec_files'
    from gems/rspec-core-2.11.0/lib/rspec/core/command_line.rb:22:in `run'
    from gems/rspec-core-2.11.0/lib/rspec/core/runner.rb:69:in `run'
    from gems/rspec-core-2.11.0/lib/rspec/core/runner.rb:8:in `block in autorun'

It then seems to be due to autoload, coupled with RubyGems, which did not add rspec-expectations lib/ dir to $: as it was not required, and the fact autoload use Kernel.require, not Kernel#require, which is overridden by RubyGems.

Owner

myronmarston commented Jul 13, 2012

I'm trying to reproduce your issue so that I can look into it. Unfortunately, I'm failing :(.

I've got this spec:

RSpec::Matchers.define :be_multiple_of do |factor|
  match do |actual|
    (actual % factor) == 0
  end
end

describe "Something" do
  it 'works' do
    9.should be_multiple_of(3)
  end
end

...and it works just fine:

➜  rspec-core git:(master) ✗ rspec --version
2.11.0
➜  rspec-core git:(master) ✗ rspec foo_spec.rb 
.

Finished in 0.00076 seconds
1 example, 0 failures

Randomized with seed 17047

What am I doing different?

Contributor

eregon commented Jul 13, 2012

I'm trying to reproduce your issue so that I can look into it. Unfortunately, I'm failing :(.

I get the same error as upper both under OSX with the ry version manager (and under all rubies versions, RBX and JRuby too) and Linux (Fedora) with RVM.
Using an older version like 2.10 does not produce the error.

Rubinius says the error happens in an at_exit handler, which makes sense as this is autorun.

Contributor

eregon commented Jul 13, 2012

2e20683 definitely changes things.
That require 'rspec/expectations/handler' did activate rspec-expectations and pushed its lib/ dir on $:, which make standard require (Kernel.require) works.

To make myself clear about the two require:

> method :require
=> #<Method: Object(Kernel)#require> 
> method(:require).source_location
=> [".../2.0.0-dev/lib/ruby/2.0.0/rubygems/custom_require.rb", 34]
> Kernel.method :require
=> #<Method: Kernel.require> 
> Kernel.method(:require).source_location
=> nil # so internal version, not aware of RubyGems

Adding that require in the example you give as the first line works of course.

I think it is wrong doing a autoload :Const, path_in_another_gem unless you activate this gem.
(BTW, all the other autoload in RSpec seem fine, they require files from their own gem)

Owner

myronmarston commented Jul 13, 2012

I get the same error as upper both under OSX with the ry version manager (and under all rubies versions, RBX and JRuby too) and Linux (Fedora) with RVM. Using an older version like 2.10 does not produce the error.

Do you mean the code snippet I posted above gave you the error? Or is there a different code snippet you're running that's getting the error?

2e20683 definitely changes things.

I know, the error you're getting makes sense to me, but as a rule, I need a failing test case so that I can demonstrate that I have in fact fixed it. If I can't repro it, I have not way to verify I've fixed anything.

Please post a code snippet that triggers this error! Without that, I can't make any progress on this issue.

Contributor

eregon commented Jul 13, 2012

@myronmarston Yes, the code snippet you posted (but mine upper as well obviously). And the error is exactly the same (well, except script name).

If you run it under bundle exec or with anything that add all gems paths to $: it won't produce any error.
Maybe you use rubygems-bundler or something similar? (Also, try in a empty directory)

In any case, add puts $:, '', $" in the first line of your snippet.
For me, the result is: https://gist.github.com/a5a7ec9932c4c1d631e8 (only rspec-core is on the load path, and only its files are required (excepted core/stdlib)). Could you show your output?

Owner

myronmarston commented Jul 13, 2012

but mine upper as well obviously

What do you mean by this? I don't see a snippet from you...

Maybe you use rubygems-bundler or something similar? (Also, try in a empty directory)

It may have been rubygems-bundler--I've noticed that rvm has started installing it even though I don't want it. However, now it's suddenly failing for me even though I didn't change anything.

So...a few thoughts on this:

  1. If you configure RSpec to expect_with :rspec before referencing RSpec::Matchers it'll work. Or if you wait to reference RSpec::Matchers until an example group has been defined, it'll work (that's because RSpec mixes in the expectation framework at that point, and assumes you want to use rspec-expectations if you haven't yet configured something else). Or if you manually require rspec/expectations it'll likewise work.
  2. We could define RSpec.const_missing so that if RSpec::Matchers is referenced it requires and loads it automatically.

I'm on the fence about this; on the one hand, the second option "just works" for end users, but I'm trying to move RSpec in the direction of less magic. But I'm not opposed to it. The first option is les magical, and makes everything very explicit--but it forces users to be be explicit about their use of rspec-expectations. Overall, the rspec gems should work seamlessly together with minimal configuration effort from end users.

Thoughts?

/cc @dchelimsky @alindeman @justinko

Contributor

eregon commented Jul 13, 2012

What do you mean by this? I don't see a snippet from you...

Just

RSpec::Matchers

in a file and running it with rspec should reproduce.
A more realistic example is https://github.com/eregon/epath/blob/master/spec/spec_helper.rb.
If the require 'rspec' is not there, it fails when run with rspec in the project dir.

I'm against having to specify RSpec.configure { |c| c.expect_with :rspec }, because that is a sensible default.
I would not like having to require rspec/expectations explicitly.

require 'rspec' seems already more logical, but it seems like we're doing part of rspec (the binary) 's job.

Waiting for an example group does not make a lot of sense in a spec_helper, as I think the main use for RSpec::Matchers is defining custom matchers.

Would it make sense to defaults to rspec/expectations at the end of a RSpec.configure block?
I would think it would be more logical than waiting for an example group, but it might not solve this problem (if one use RSpec::Matchers before RSpec.configure)

  1. is indeed a bit magic, but it makes a lot of sense to me. If a user types RSpec::Matchers, that's the best advice RSpec can have that he wants rspec/expectations, so doing in a way that works is just the best for him. Also, it isn't more magic than autoload.

I would then go with 2), and if not with require 'rspec', adding that in the custom matchers documentation (and explaining why).

Contributor

graaff commented Jul 15, 2012

While preparing the rspec 2.11 package for Gentoo Linux I also noticed this problem, but only with jruby. Not sure why the others work while jruby fails. I'm getting the error trying to run rspec-core's own test suite.

LoadError: no such file to load -- rspec/matchers
           (root) at ./spec/support/matchers.rb:1
          require at org/jruby/RubyKernel.java:1033
          require at /usr/share/jruby/lib/ruby/site_ruby/1.8/rubygems/custom_require.rb:36
           (root) at ./spec/support/matchers.rb:29
          collect at org/jruby/RubyArray.java:2331
           (root) at /var/tmp/portage/dev-ruby/rspec-core-2.11.0/work/jruby/rspec-rspec-core-f539e2a/spec/spec_helper.rb:29
          prefork at /var/tmp/portage/dev-ruby/rspec-core-2.11.0/work/jruby/rspec-rspec-core-f539e2a/spec/spec_helper.rb:14
           (root) at /var/tmp/portage/dev-ruby/rspec-core-2.11.0/work/jruby/rspec-rspec-core-f539e2a/spec/spec_helper.rb:23
          require at org/jruby/RubyKernel.java:1033
          require at /var/tmp/portage/dev-ruby/rspec-core-2.11.0/work/jruby/rspec-rspec-core-f539e2a/spec/spec_helper.rb:36
           (root) at /var/tmp/portage/dev-ruby/rspec-core-2.11.0/work/jruby/rspec-rspec-core-f539e2a/spec/autotest/discover_spec.rb:1
             load at org/jruby/RubyKernel.java:1058
  load_spec_files at /var/tmp/portage/dev-ruby/rspec-core-2.11.0/work/jruby/rspec-rspec-core-f539e2a/spec/autotest/discover_spec.rb:780
          collect at org/jruby/RubyArray.java:2331
  load_spec_files at /var/tmp/portage/dev-ruby/rspec-core-2.11.0/work/jruby/rspec-rspec-core-f539e2a/lib/rspec/core/configuration.rb:780
              run at /var/tmp/portage/dev-ruby/rspec-core-2.11.0/work/jruby/rspec-rspec-core-f539e2a/lib/rspec/core/command_line.rb:22
              run at /var/tmp/portage/dev-ruby/rspec-core-2.11.0/work/jruby/rspec-rspec-core-f539e2a/lib/rspec/core/runner.rb:69
          autorun at /var/tmp/portage/dev-ruby/rspec-core-2.11.0/work/jruby/rspec-rspec-core-f539e2a/lib/rspec/core/runner.rb:8
             call at org/jruby/RubyProc.java:270
             call at org/jruby/RubyProc.java:224

Given that it's autoload-related I think I'll just patch in a require for rspec/matchers in spec/helper.rb on our end so that specs pass when people install rspec.

Owner

dchelimsky commented Jul 15, 2012

Overall, the rspec gems should work seamlessly together with minimal configuration effort from end users.

I agree with this and think this should not be pushed out to end-users. +1 for using const_missing. I don't see it as any more magical than autoload if it's done properly (define it on RSpec and call super!).

Contributor

graaff commented Jul 17, 2012

I'm now seeing this in the wild as well (e.g. with the specs for diff-lcs 1.1.3), so this may warrant a new release?

Owner

myronmarston commented Jul 18, 2012

I'm now seeing this in the wild as well (e.g. with the specs for diff-lcs 1.1.3), so this may warrant a new release?

I'm going to try to cut a 2.11.1 release with this fix in the next day or two, but my wife is due to give birth in a week, so no promises :).

@myronmarston myronmarston added a commit that referenced this issue Jul 18, 2012

@myronmarston myronmarston Fix the way we autoload RSpec::Matchers.
`autoload`, besides being deprecated by Matz, does not work to require files
that are in unactivated gems--it only works with ruby's built in require, for
files that are available relative to a directory on the load path. Instead,
we use `const_missing` to make it work.

Closes #647.
2e32d6e
Contributor

graaff commented Jul 19, 2012

Noticed that the release was out. Thanks, I've updated our Gentoo package as well. Best wishes for the coming days!

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