Skip to content

Extract Rails extension to have matcher #5

Merged
merged 9 commits into from Feb 25, 2014

4 participants

@soulcutter
RSpec member

I'm not sure this is the best way to conditionally load the matcher extension, but I figured it's at least a workable first pass.

This is half of rspec/rspec-rails#808

@soulcutter soulcutter referenced this pull request in rspec/rspec-rails Aug 19, 2013
Merged

Remove the have extension #810

@soulcutter
RSpec member

Looks like this is failing on Travis - it was passing for me locally so I'm not immediately certain what is amiss. HMM.

@hugobarauna hugobarauna commented on an outdated diff Aug 20, 2013
lib/rspec/collection_matchers/rails.rb
@@ -0,0 +1,8 @@
+module RSpec
+ module CollectionMatchers
+ module Rails
@hugobarauna
RSpec member
hugobarauna added a note Aug 20, 2013

Do we need to define an empty module?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@hugobarauna hugobarauna and 1 other commented on an outdated diff Aug 20, 2013
rspec-collection_matchers.gemspec
@@ -21,4 +21,5 @@ Gem::Specification.new do |spec|
spec.add_runtime_dependency "rspec-expectations", ">= 2.99.0.pre"
spec.add_development_dependency "bundler", "~> 1.3"
+ spec.add_development_dependency(%q<activesupport>, [">= 3.0"])
@hugobarauna
RSpec member
hugobarauna added a note Aug 20, 2013

This is just style, but what do you think about changing that to:

 spec.add_development_dependency "activesupport" , ">= 3.0"
@hugobarauna
RSpec member
hugobarauna added a note Aug 20, 2013

By the way, can you just move that dependency definition to the Gemfile? I was following the convention of putting development dependencies in the Gemfile instead of using the gemspec for that.

@soulcutter
RSpec member
soulcutter added a note Aug 20, 2013

Sure. I copied that over from rspec-rails' gemspec, which is where the funky string came from if you were wondering 😄

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

@soulcutter Maybe it's failing because the Rails constant is not defined. If that's the problem, we could fix that by just stubing that constant using RSpec itself.

@soulcutter
RSpec member

It shouldn't have to do with the Rails constant - the spec explicitly requires the have extensions without checking the constant at all. I'm more thinking it may have to do with me testing against the master branches of everything vs travis building with the 2-99-maintenance branch.

@JonRowe
RSpec member
JonRowe commented Aug 20, 2013

@soulcutter Although you've now linked this to master, it needs to work with 2-99-maintenance too, as otherwise there isn't a smooth upgrade path

@JonRowe JonRowe commented on an outdated diff Aug 20, 2013
@@ -7,12 +7,15 @@ gemspec
if File.exist?(library_path)
gem lib, :path => library_path
else
- gem lib, :git => "git://github.com/rspec/#{lib}.git", :branch => "2-99-maintenance"
+ gem lib, :git => "git://github.com/rspec/#{lib}.git", :branch => "master"
@JonRowe
RSpec member
JonRowe added a note Aug 20, 2013

We need to ensure this works against 2-99-maintenance and master

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JonRowe JonRowe and 1 other commented on an outdated diff Aug 20, 2013
end
end
-gem "cucumber", "~> 1.1.9"
-gem "aruba", "~> 0.5"
-gem "rake", "~> 10.0.0"
+gem 'cucumber', '~> 1.1.9'
+gem 'aruba', '~> 0.5'
+gem 'rake', '~> 10.0.0'
@JonRowe
RSpec member
JonRowe added a note Aug 20, 2013

Despacing these is pretty pointliness, goes against what we do in other Gemfiles elsewhere, and is contrary to common practise.

@soulcutter
RSpec member
@JonRowe
RSpec member
JonRowe added a note Aug 20, 2013

It's just an unnecessary change here, out of keeping with the rest of rspec, and yeah, I like lined up things, I find it easier to scan.

@JonRowe
RSpec member
JonRowe added a note Aug 20, 2013

(In my opinion :) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JonRowe JonRowe commented on an outdated diff Aug 20, 2013
lib/rspec/collection_matchers/rails/have_extensions.rb
+
+require 'active_support/concern'
+require 'active_support/core_ext/module/aliasing'
+
+module RSpec
+ module CollectionMatchers
+ module Rails
+ module HaveExtensions
+ extend ActiveSupport::Concern
+
+ # @api private
+ #
+ # Enhances the failure message for `to have(n)` matchers
+ def failure_message_for_should_with_errors_on_extensions
+ return "expected #{relativities[@relativity]}#{@expected} errors on :#{@args[0]}, got #{@actual}" if @collection_name == :errors_on
+ return "expected #{relativities[@relativity]}#{@expected} error on :#{@args[0]}, got #{@actual}" if @collection_name == :error_on
@JonRowe
RSpec member
JonRowe added a note Aug 20, 2013

Could we streamline these two by using interpolation? How about:

if [:errors_on,:error_on].include? @collection_name
  return "expected #{relativities[@relativity]}#{@expected} #{@collection_name.to_s.gsub('_',' ')} :#{@args[0]}, got #{@actual}"
end
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JonRowe JonRowe commented on an outdated diff Aug 20, 2013
lib/rspec/collection_matchers/rails/have_extensions.rb
+
+ # @api private
+ #
+ # Enhances the failure message for `to have(n)` matchers
+ def failure_message_for_should_with_errors_on_extensions
+ return "expected #{relativities[@relativity]}#{@expected} errors on :#{@args[0]}, got #{@actual}" if @collection_name == :errors_on
+ return "expected #{relativities[@relativity]}#{@expected} error on :#{@args[0]}, got #{@actual}" if @collection_name == :error_on
+ return failure_message_for_should_without_errors_on_extensions
+ end
+
+ # @api private
+ #
+ # Enhances the description for `to have(n)` matchers
+ def description_with_errors_on_extensions
+ return "have #{relativities[@relativity]}#{@expected} errors on :#{@args[0]}" if @collection_name == :errors_on
+ return "have #{relativities[@relativity]}#{@expected} error on :#{@args[0]}" if @collection_name == :error_on
@JonRowe
RSpec member
JonRowe added a note Aug 20, 2013

These are similar to lines 16-17, prehaps it's worth extracting?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JonRowe JonRowe and 1 other commented on an outdated diff Aug 20, 2013
...pec/collection_matchers/rails/have_extensions_spec.rb
+ it "provides a failure message including the number actually given" do
+ lambda {
+ [1].should have(3).errors_on(:whatever)
+ }.should raise_error("expected 3 errors on :whatever, got 1")
+ end
+ end
+
+ describe "have something other than error_on or errors_on" do
+ it "has a standard rspec failure message" do
+ lambda {
+ [1,2,3].should have(2).elements
+ }.should raise_error("expected 2 elements, got 3")
+ end
+
+ it "has a standard rspec description" do
+ have(2).elements.description.should == "have 2 elements"
@JonRowe
RSpec member
JonRowe added a note Aug 20, 2013

I was a little bit WTF BBQ at the naked have(2) call, is it worth instantiating the matcher directly to make it more clear what it's doing in this circumstances?

@soulcutter
RSpec member
@JonRowe
RSpec member
JonRowe added a note Aug 20, 2013

Taking the time to refactor when we can is a good thing:)

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

Bump..this needs to still be worked out.

@myronmarston myronmarston referenced this pull request Feb 19, 2014
Closed

Cut a release #14

@hugobarauna
RSpec member

@soulcutter are you going to continue working on this? I can work on that if you're not going to.

@soulcutter
RSpec member

I think I solved the 2.99 compatibility issue, let's see what Travis says…

EDIT: Nope… looks like something is different in 1.8.x rubies.

@soulcutter soulcutter Fix Ruby 1.8.x compatibility
instance_methods returns an array of symbols in Ruby 1.9+
but before that it was an array of strings.
80b75a0
@soulcutter
RSpec member

Looks like the build works now. Any other feedback?

@JonRowe JonRowe commented on an outdated diff Feb 20, 2014
...pec/collection_matchers/rails/have_extensions_spec.rb
+
+ describe "have something other than error_on or errors_on" do
+ it "has a standard rspec failure message" do
+ lambda {
+ [1,2,3].should have(2).elements
+ }.should raise_error("expected 2 elements, got 3")
+ end
+
+ it "has a standard rspec description" do
+ RSpec::CollectionMatchers::Have
+ have(2).elements.description.should == "have 2 elements"
+ end
+ end
+ end
+end
+
@JonRowe
RSpec member
JonRowe added a note Feb 20, 2014

excess white space

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JonRowe JonRowe commented on an outdated diff Feb 20, 2014
...pec/collection_matchers/rails/have_extensions_spec.rb
+ it "provides a description including the name of what the error is on" do
+ have(2).errors_on(:whatever).description.should == "have 2 errors on :whatever"
+ end
+
+ it "provides a failure message including the number actually given" do
+ lambda {
+ [1].should have(3).errors_on(:whatever)
+ }.should raise_error("expected 3 errors on :whatever, got 1")
+ end
+ end
+
+ describe "have something other than error_on or errors_on" do
+ it "has a standard rspec failure message" do
+ lambda {
+ [1,2,3].should have(2).elements
+ }.should raise_error("expected 2 elements, got 3")
@JonRowe
RSpec member
JonRowe added a note Feb 20, 2014

We should change these to expect {}.to raise_error given our preference for the new syntax

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@JonRowe JonRowe and 1 other commented on an outdated diff Feb 20, 2014
lib/rspec/collection_matchers.rb
@@ -1,3 +1,4 @@
require 'rspec/collection_matchers/version'
require 'rspec/collection_matchers/matchers'
require 'rspec/collection_matchers/have'
+require 'rspec/collection_matchers/rails' if defined? Rails
@JonRowe
RSpec member
JonRowe added a note Feb 20, 2014

Is this enough of a check?

@soulcutter
RSpec member
soulcutter added a note Feb 20, 2014

Technically this will break if Rails is defined, but there's no activesupport for alias_method_chain (which is used in the impl).

In practice I'm not sure that will come up, but we could guard against that to be airtight. Otherwise it's not really a problem to include the matcher, even mistakenly.

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

@hugobarauna I'm all done with this one - care to comment / merge?

@hugobarauna hugobarauna and 1 other commented on an outdated diff Feb 24, 2014
lib/rspec/collection_matchers/rails/have_extensions.rb
@@ -0,0 +1,58 @@
+require 'rspec/collection_matchers/have'
@hugobarauna
RSpec member
hugobarauna added a note Feb 24, 2014

I'm not sure if we need that require here, since we're already loading that file in lib/rspec/collection_matchers/rails

@soulcutter
RSpec member
soulcutter added a note Feb 24, 2014

I'm not sure rails.rb really deserves to exist, honestly. It's not actually loaded there, though, it's really coming from lib/rspec/collection_matchers.rb

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

:shipit:

@soulcutter soulcutter merged commit a8024af into master Feb 25, 2014

1 check passed

Details default The Travis CI build passed
@soulcutter soulcutter deleted the rails-have-extensions branch Feb 25, 2014
@alexrothenberg alexrothenberg pushed a commit to alexrothenberg/rspec-rails that referenced this pull request Apr 7, 2014
@soulcutter soulcutter Remove the have extension
This functionality was extracted into
rspec/rspec-collection_matchers#5

Fixes #808
7fd43e9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.