Add RSpec::Matchers::BuiltIn::BeBetween #405

Merged
merged 6 commits into from Jan 4, 2014

Conversation

Projects
None yet
2 participants
Contributor

sferik commented Jan 3, 2014

Ruby’s #between? instance method is included in the Comparable module. Any object that mixes in Comparable and responds to the <=> operator will also respond to #between?, including the following core classes:

  • Numeric (Fixnum, Bignum, Integer, Float, Complex, and Rational)
  • String
  • Symbol
  • Time

...as well as a few lesser-used ones (e.g. File::Stat, Gem::Version).

BeBetween defines a matcher that takes exactly two arguments—a minimum and a maximum value—mimicking the interface of Comparable.html#between? and ensuring backward compatibility.

Given the following expectation:

expect(year).to be_between(1939, 1945)

Before this patch, using be_between on Comparable objects worked but produced the following failure message:

expected between?(1939, 1945) to return true, got false

After this patch, it will produce this message, instead:

expected 2014 to be between 1939 and 1945

I believe this is a much more informative failure message, as it reveals the actual value, which is typically a variable, in addition to displaying the expectation in plainer English.

@myronmarston myronmarston commented on the diff Jan 3, 2014

spec/rspec/matchers/built_in/be_between_spec.rb
@@ -0,0 +1,30 @@
+require 'spec_helper'
+
+describe "expect(...).to be_between(min, max)" do
+ it_behaves_like "an RSpec matcher", :valid_value => (5), :invalid_value => (11) do
+ let(:matcher) { be_between(1, 10) }
+ end
+
+ it "passes if target is between min and max" do
+ expect(5).to be_between(1, 10)
+ end
+
+ it "fails if target is not between min and max" do
+ expect {
+ # It does not go to 11
@myronmarston

myronmarston Jan 3, 2014

Owner

This made me laugh :).

@myronmarston myronmarston commented on the diff Jan 3, 2014

lib/rspec/matchers/built_in.rb
autoload :YieldWithArgs, 'rspec/matchers/built_in/yield'
autoload :YieldWithNoArgs, 'rspec/matchers/built_in/yield'
- autoload :YieldSuccessiveArgs, 'rspec/matchers/built_in/yield'
@myronmarston

myronmarston Jan 3, 2014

Owner

Thanks for organizing these -- I thought they were already in alphabetical order but I guess not.

@sferik

sferik Jan 3, 2014

Contributor

I moved the alphabetization into a separate commit, to keep things clean.

@myronmarston myronmarston and 1 other commented on an outdated diff Jan 3, 2014

spec/rspec/matchers/description_generation_spec.rb
@@ -46,7 +46,7 @@
end
it "expect(...).to be predicate arg1, arg2 and arg3" do
- expect(5.0).to be_between(0,10)
+ expect(5.0).to be_between(0, 10)
@myronmarston

myronmarston Jan 3, 2014

Owner

Interesting that we were already using be_between even without the better matcher :).

That said, this spec is meant to test the dynamic predicate matchers (e.g. be_xyz => xyz?) but now it's not calling the predicate anymore so it would be good to update the description of this example to indicate it's for be_between and then make a different example for predicate matchers. I was thinking we could use Fixnum#odd? or Fixnum#even? (be_odd and be_even, respectively) but part of the point here is to verify that the arguments are formatted well in the description. I can't think of another built-in ruby predicate that takes multiple arguments. Can you?

@sferik

sferik Jan 3, 2014

Contributor

Hmmm, after looking around for another predicate method that takes multiple arguments, I’m not sure one exists. How would you feel about adding one via monkey-patching (just in the specs)?

@myronmarston

myronmarston Jan 3, 2014

Owner

Maybe just make a test double that defines a multiple-arg predicate? That way we don't have to monkey patch anything.

@myronmarston myronmarston and 1 other commented on an outdated diff Jan 3, 2014

spec/rspec/matchers/built_in/be_between_spec.rb
+ expect(11).to be_between(1, 10)
+ }.to fail_with("expected 11 to be between 1 and 10")
+ end
+end
+
+describe "expect(...).not_to be_between(min, max)" do
+ it "passes if target is not between min and max" do
+ expect(11).not_to be_between(1, 10)
+ end
+
+ it "fails if target is between min and max" do
+ expect {
+ expect(5).not_to be_between(1, 10)
+ }.to fail_with("expected 5 not to be between 1 and 10")
+ end
+end
@myronmarston

myronmarston Jan 3, 2014

Owner

One thing that's unclear to me (mostly because I never new Comparable#between? existed...) is if between? is inclusive or exclusive. Exclusive would make more sense to me, but regardless, it would be good to document that here with specs showing the behavior on the exact edges of the range and also with a @note in the docs you added for be_between.

@sferik

sferik Jan 3, 2014

Contributor

Good point. It’s inclusive. I’ll add the word “inclusive” to the failure message, in case this confuses anyone else.

@myronmarston

myronmarston Jan 3, 2014

Owner

Yep, including it in the failure message would be helpful. Do you think it's worth supporting exclusive mode like this?

expect(value).to be_between(0, 10).exclusive

# alternately
expect(value).to be_exclusively_between(0, 10)

I'm not sure if it's worth the effort to support that (given you'd have to do the comparisons yourself rather than just calling between?) but thought it worth bringing up.

@sferik

sferik Jan 3, 2014

Contributor

Someone else can work on adding an exclusive mode if they want it but I don’t need it and don’t see it as essential for this feature.

@myronmarston

myronmarston Jan 3, 2014

Owner

Works for me.

@myronmarston myronmarston commented on the diff Jan 3, 2014

lib/rspec/matchers.rb
@@ -317,6 +317,16 @@ def be_a_kind_of(expected)
alias_method :be_kind_of, :be_a_kind_of
alias_matcher :a_kind_of, :be_a_kind_of
+ # Passes if actual between min and max
+ #
+ # @example
+ #
+ # expect(5).to be_between(1, 10)
+ # expect(11).not_to be_between(1, 10)
+ def be_between(min, max)
+ BuiltIn::BeBetween.new(min, max)
+ end
@myronmarston

myronmarston Jan 3, 2014

Owner

Please add a matcher alias for this:

alias_matcher :a_value_between, :be_between

...which allows the matcher to read correctly in composed matcher expressions:

expect(ages).to include( a_value_between(10, 20) )

It also makes the failure message read better. You can add a spec to spec/rspec/matchers/aliases_spec.rb.

Owner

myronmarston commented Jan 3, 2014

You know, I wonder if we should also include the predicate matcher failure message? Currently it's:

expected between?(1, 10) to return true, got false

...but it seems like it would be much better to have this:

expected (11).between?(1, 10) to return true, got false

Basically, interpolate "(#{actual.inspect})" in the string.

Contributor

sferik commented Jan 3, 2014

Basically, interpolate "(#{actual.inspect})" in the string.

Yes, that seems like a big improvement. I can work on that even thought it’s not directly related. Do you want it in a separate pull request or can I tack it on to this one?

Owner

myronmarston commented Jan 3, 2014

Yes, that seems like a big improvement. I can work on that even thought it’s not directly related. Do you want it in a separate pull request or can I tack it on to this one?

Do it in a separate PR, please.

Contributor

sferik commented Jan 3, 2014

Do it in a separate PR, please.

#406

@myronmarston myronmarston commented on the diff Jan 3, 2014

spec/rspec/matchers/description_generation_spec.rb
@@ -46,8 +46,14 @@
end
it "expect(...).to be predicate arg1, arg2 and arg3" do
- expect(5.0).to be_between(0,10)
- expect(RSpec::Matchers.generated_description).to eq "should be between 0 and 10"
+ class Parent; end
+ class Child < Parent
+ def child_of?(*parents)
+ parents.all? { |parent| self.is_a?(parent) }
+ end
+ end
+ expect(Child.new).to be_a_child_of(Parent, Object)
+ expect(RSpec::Matchers.generated_description).to eq "should be a child of Parent and Object"
end
@myronmarston

myronmarston Jan 3, 2014

Owner

It would be good to keep an example for the generated description of be_between -- you mind adding that back but with a doc string stating it's for be_between rather then general predicates?

@sferik

sferik Jan 3, 2014

Contributor

Totes.

Owner

myronmarston commented Jan 3, 2014

This LGTM. I left one more comment but I can take care of that when I merge if you prefer .

Contributor

sferik commented Jan 3, 2014

I just noticed there are cucumber features for all the matchers. Before you merge, would you like me to write a cucumber feature for this? I’ve never really used cucumber before so I don’t really know what I’m doing but I can probably figure out enough to make it turn green like a big, ripe cuke!

Owner

myronmarston commented Jan 3, 2014

Before you merge, do I need to write a cucumber feature for this? I’ve never really used cucumber before so I might suck at it but I think I can figure it out enough to turn green like a cuke!

It would be nice but I don't consider it a merge blocker. We like to have all matchers documented with cukes since we've been using relish as our primary docs (although, that's likely changing for RSpec 3). So sure...if you're willing to add a cuke for this, please do!

Owner

myronmarston commented Jan 3, 2014

Actually: maybe we don't need a cuke. be_between has always worked because of the method missing be_xyz predicate support. This doesn't change the API, it just improves the failure message, but I think adding a cuke for that will just be noise. Thoughts?

Contributor

sferik commented Jan 3, 2014

I already started working on the cukes. Let’s see how far I get…

cuke

Owner

myronmarston commented Jan 3, 2014

Actually, I thought of one more thing: with your implementation, expect(some_object_that_doesnt_respond_to_be_between_eh).to be_between(0, 10) will fail with NoMethodError. In the past this has generally been fine (because matchers were generally just checked against one value), but now that we support composing them, it wrongly fails in situations like this:

expect([Object.new, 3]).to include( a_value_between(2, 4), an_instance_of(Object) )

...because it'll blow up when trying to match the object instance against the matcher. Instead, the BeBetween#matches? should return false, and the failure_message can indicate it doesn't response to between?. That'll give a good failure message when used directly against the wrong type of object while still working in composed situations. For an example of this, see the be_within matcher implementation.

Owner

myronmarston commented Jan 4, 2014

This LGTM. I'm happy to merge. Are you still working on the cuke?

Contributor

sferik commented Jan 4, 2014

Are you still working on the cuke?

@myronmarston Yup…and I’m glad I’m doing it. Writing a comprehensive cuke suite has actually revealed a couple of minor bugs.

It’s much slower going, both because I’m unfamiliar with cucumber and making a bunch of rookie mistakes but also because the cucumber suite takes over a minute to run (compared to under a minute for the specs). Why is cucumber so much slower?

Also a couple questions about cucumber:

  1. How to I run a single cuke file/feature from the command line?
  2. How do I describe a symbol in a cuke? It seems to be getting converted to a string.
Owner

myronmarston commented Jan 4, 2014

You can run one file with bundle exec cucumber path/to/feauture/file, or tag your feature or scenario with @wip and run bundle exec cucumber --profile wip.

Also, how do I describe a symbol in a cuke? It seems to be getting converted to a string.

You mean like this?

describe :some_symbol do
  it "is the subject" do
    expect(subject).to eq(:some_symbol)
  end
end

That fails, surprisingly...seems like an rspec-core bug that no one's ever hit. (After all, how often do you describe a symbol?). Why do you need to describe a symbol for this?

Owner

myronmarston commented Jan 4, 2014

Writing a comprehensive cuke suite has actually revealed a couple of minor bugs.

BTW, for any bugs you find, please write specs that expose them, too. The cukes have some nice integration benefits (particularly because it runs specs with a clean environment, so it makes things like test-unit integration easier to test), but given we use them for documentation, and edge case regression tests have little documentation value, I prefer to limit the cukes to just what's needed to doc how something works. Generally I rely only on the spec suite for verifying my refactorings haven't broken anything since the cukes are so much slower.

Contributor

sferik commented Jan 4, 2014

You can run one file with bundle exec cucumber path/to/feauture/file, or tag your feature or scenario with @wip and run bundle exec cucumber --profile wip.

Oh, man. I wish I’d know about that an hour ago. 😉

I’ll push my broken features up now so you can see what I’m trying to do.

Owner

myronmarston commented Jan 4, 2014

Thanks. I wish I had know about that an hour ago.

You can always hop on the rspec IRC channel on freenode to discuss this stuff in real time, especially if you keep contributing (which I hope you do!). I'm not alway on there but try to frequent it.

Contributor

sferik commented Jan 4, 2014

BTW, for any bugs you find, please write specs that expose them, too.

The bug was that I was calling inspect on the actual value but not on the expected min/max values, so it produced inconsistent output when values that inspect differently than they to_s, like symbols.

Owner

myronmarston commented Jan 4, 2014

I just opened an rspec core issue for that.

Anyhow, I appreciate the work you're doing here, but I think that cuking all those different types is overkill:

  • From a documentation perspective, users don't care (or need) to see examples spelled out with every type of object that is supported by this matcher. Having some passing and some failing examples (e.g. provided by one scenario) is sufficient I think, as long as the narrative mentions that the matcher works with many other types of objects (you could maybe even say that it works with anything that defines between? or includes Comparable).
  • From a code maintenance/cost-benefit perspective, I find cukes to be very expensive: consider that our spec suite for this project runs 1215 examples in 0.58259 seconds (at least, that's what I got just now on my box). That's about a half millisecond per example...and at that speed, the specs are basically free compared to the cukes. I think this kind of exhaustive testing is more appropriate for specs, anyway.

So...do you mind converting the scenarios for different types to specs? I think some specs like this will be sufficient:

it 'works with symbols' do
  expect(:bazz).to be_between(:bar, :foo)
  expect {
    expect(:foo).to be_between(:bar, :bazz)
  }.to fail_with("the failure string you expect")
end
Owner

myronmarston commented Jan 4, 2014

Anyhow, I'm heading out to get on the bus and go home. I plan to be back online later tonight, though.

Contributor

sferik commented Jan 4, 2014

You can always hop on the rspec IRC channel on freenode to discuss this stuff in real time, especially if you keep contributing (which I hope you do!). I'm not alway on there but try to frequent it.

To be honest, I’m not a fan of IRC.

Here are my reasons:

  1. IRC is real-time. When someone says something to me or asks me a question, I know there’s a person waiting for me on the other end, so I feel some pressure to respond quickly, not to keep them hanging. This, in turn, causes me to rush and make mistakes, which brings me to…
  2. IRC is immutable. Since I cannot go back and correct mistakes in what I’ve typed, I feel a lot of pressure to type without any mistakes. This is made worse by the fact that IRC channels are often logged (both by individuals and services), so my mistakes are likely to be archived on the internet forever (the horror!). Maybe I’m too much of a perfectionist when it comes to this but I prefer to use services where anything I broadcast on the internet can later be edited or deleted. IRC and public mailing lists are the only services I use where this is not true.
  3. The tension between asynchrony and immutability. “Should I hit send so the other person won’t have to keep waiting or should I proofread it one more time?” This constant question in my head produces a lot of anxiety in me, which leads to even more mistakes, in both my typing and my thinking. “Maybe just one more proofread. Okay, looks good. Hits return. 💥 FUCK! I can’t believe I sent that.” It’s a vicious spiral.
  4. There are no good IRC clients. Believe me, I have tried them all. Colloquy, Textual, LimeChat, Adium. I’ve tried web clients, IRCCloud and Kiwi. I’ve even tried some command line clients, irssi and weechat. They are not all bad but none of them are what I would call “good software”. In the GUI clients, the typography and layout is typically horrendous, resembling Microsoft Outlook. Authors of these programs apparently think design means adding yet another color scheme.

As you can probably tell by now, I prefer corresponding on GitHub, which is less asynchronous, but editable, or Twitter, which is not editable (though mistakes can be deleted), where I don’t feel like anyone is sitting around waiting for my next tweet.

</rant>

Contributor

sferik commented Jan 4, 2014

So...do you mind converting the scenarios for different types to specs? I think some specs like this will be sufficient:

@myronmarston I’d be happy to do that. It was mostly just a learning exercise for me to try out cucumber again. I remember testing cucumber out 5 years ago, shortly after it was released. I didn’t see the point then and I don’t see the point now. That said, I don’t really know what I’m doing, which means I’m probably doing it wrong.

Contributor

sferik commented Jan 4, 2014

The new tests you asked me to add are passing on MRI 1.9 through 2.1 but it looks like the behavior or Symbol#<=> differs between Ruby implementations. Arguably, those are bugs in those Ruby implementations, especially the ones that aim the be compatible with Ruby 1.9 or higher (Rubinius 2 and JRuby in Ruby 1.9-mode).

Contributor

sferik commented Jan 4, 2014

Okay, it looks like Comparable was only mixed-in to Symbol in Ruby 1.9.

It does still seem like a bug in those Ruby implementations that it doesn’t work in Rubinius or JRuby in 1.9-mode or even JRuby 9K, which targets Ruby 2.1.

Contributor

sferik commented Jan 4, 2014

By the way, if you want a list of classes that are Comparable on a particular Ruby implementation, here’s the command I’ve been using:

ObjectSpace.each_object(Class).select{|c| c.ancestors.include?(::Comparable)}.sort_by(&:name)
Owner

myronmarston commented Jan 4, 2014

Ah, the joys of keeping a green build on so many ruby interpreters/versions...

Given that we don't have any logic that is specific to these types, I don't think we actually need specs for each of them given the differences in inspect output, symbol not being comparable on all versions, etc. So my suggestion is to remove the ones that don't pass on all rubies, but keep at least one non-numeric type one that forces the implementation to use inspect in the failure message.

BTW, seeing the failure message for Symbol on 1.8:

expected :baz to be between :bar and :foo (inclusive)

...makes me think that it would be good for it to mention why for cases like these:

expected :baz to be between :bar and :foo (inclusive), but :baz does not respond to `between?`

This is similar to what we do for be_within.

Does that sound reasonable?

Owner

myronmarston commented Jan 4, 2014

BTW, as for testing against JRuby and MRI HEAD: I like the idea of adding those but would want them listed as allowed failures. I don't want failing builds because of failures on them.

Contributor

sferik commented Jan 4, 2014

Fair enough. I just added JRuby head to see whether it was still an issue I should report or whether it has already been fixed on JRuby 9K, which I don’t have installed on the machine I’m using at the moment.

sferik added some commits Jan 3, 2014

@sferik sferik Add RSpec::Matchers::BuiltIn::BeBetween
Ruby's #between? instance method is included in the Comparable module.
Any object that mixes in Comparable and responds to the <=> operator
will also respond to #between?, including the following core classes:

* Numeric (Fixnum, Bignum, Integer, Float, Complex, and Rational)
* String
* Symbol
* Time

...as well as a few lesser-used ones (e.g. File::Stat, Gem::Version).

BeBetween defines a matcher that takes exactly two arguments--a minimum
and a maximum value--mimicking the interface of Comparable.html#between?
and ensuring backward compatibility.

Before this patch, using be_between on Comparable objects worked but
produced the following failure message:

    expected between?(1, 10) to return true, got false

After this patch, it will produce this message, instead:

    expected 5 to be between 1 and 10

I believe this is a much more informative failure message, as it reveals
the actual value, which is often a variable, in addition to displaying
the expectation in plainer English.
1bf592a
@sferik sferik Alphabetize/organize built-in matchers 596dc0a
@sferik sferik Define custom predicate method to test description generation 0ef2bd5
@sferik sferik Note that between is inclusive of min and max values and test edge cases cf2d0a5
@sferik sferik Make BeBetween composable 308ccce
@sferik sferik Test against JRuby and MRI at head 7bbe151
Contributor

sferik commented Jan 4, 2014

Okay, all 💚. Merge it!

myronmarston merged commit 4f44ec9 into rspec:master Jan 4, 2014

1 check passed

default The Travis CI build passed
Details
Owner

myronmarston commented Jan 4, 2014

Thanks, @sferik!

sferik deleted the sferik:be_between branch Jan 5, 2014

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