rspec / rspec-expectations

Provides a readable API to express expected outcomes of a code example
https://rspec.info
MIT License
1.26k stars 397 forks source link

Why is `all` not supported by `not_to` #1239

Closed luke-hill closed 3 years ago

luke-hill commented 3 years ago

Subject of the issue

Curiousity as to why this isn't supported

Your environment

N/A

Steps to reproduce

The matcher not_to all is not supported. I appreciate this is a trickier one, but it has a reasonable amount of valid use cases. I've explained the regular ruby equivalent below

Expected behavior

N/A

Actual behavior

It crashes

Test Candidate: Data: We have various packets of fruit containing - :apple, :orange, :lemon, :pear Issue: I want to find out if my basket contains apples Matcher 1: has_apple? -> have_apple Matcher 2: has_no_apple? -> have_no_apple

Basket 1:

[
  %i[apple pear],
  %i[apple pear],
  %i[apple pear]
]

Tests:

expect(basket).to all have_apple # true
expect(basket.first).to have_apple # true
expect(basket).to all have_no_apple # false (Fails on first check)
expect(basket).not_to all have_no_apple # true (Passes on first check, because we find an apple, thus have_no_apple is false, and not_to negates this)

Basket 2:

[
  %i[orange pear],
  %i[orange orange],
  %i[apple pear]
]

Tests:

expect(basket).to all have_apple # false
expect(basket.first).to have_apple # false
expect(basket).to all have_no_apple # false (Fails on last check)
expect(basket).not_to all have_apple # true (Passes on first check, because the first one doesn't have an apple, so it fails, and not_to negates this)
expect(basket).not_to all have_no_apple # true (After check 1, the all assertion is true - i.e. the spec would fail. Same after 2. After 3, all have_no_apple fails. not_to inverts this and so the spec passes just at the last minute

NB: This is equivalent in ruby to performing the following logic (Because asking ALL not to have ONE is the same as asking SOME to have ONE

basket_has_any_apples = basket.any? { |packet| packet.has_apple? }

expect(basket_has_any_apples).to be true
JonRowe commented 3 years ago

In the original PR for this feature, there was a discussion about a possible negated interface for this:

https://github.com/rspec/rspec-expectations/pull/491#discussion_r10632728

The TL;DR; is that it is ambiguous grammatically. The inverse of to all x strictly speaking would be that any value which doesn't pass x would cause to not all x to pass, but thats not what it says, to not all x in our language implies that all values should not pass x.

Instead it was preferred to use negated compound matchers like to all(not(x)) etc.

luke-hill commented 3 years ago

Fair point about it being ambiguous grammatically. That seems fair. However the negated matcher/s for my example already exist (i.e. no apples present in packet of fruit). Or am I missing another step?

when you say to all(not(x)) do you mean something like to all(have_no_apple) Or something else?

JonRowe commented 3 years ago

when you say to all(not(x)) do you mean something like to all(have_no_apple) Or something else?

Sorry I think I was misremembering the solution, we originally talked about not(matcher) being possible but I don't think it was finished, I think eventually we went with this as the solution:

https://relishapp.com/rspec/rspec-expectations/v/3-10/docs/define-negated-matcher

So you could define :not_all or :have_no_apple more easily.

luke-hill commented 3 years ago

Hi @JonRowe Sadly this wouldn't work, as the two are not functionally equivalent. Looking at the define negated matcher. This would work for something that is of ordinality 1. I.e. a collection, but when you have a collection of collections and as such need the all bit, that's when this breaks down.

expect( collection ).not_to all be_missing -> This would validate just a single presence (good for our use case).

Another option (Thinking about mitigating the language barrier), is instead of having the not not variety. To have the any variety. So in english terms, using the any? predicate you would get.

expect( collection ).to any be_present - I appreciate this sounds crap. However I also had another thought. An alias which sounds nicer.

expect( collection ).to partially be_present

Thoughts / Ideas? If I'm completely being do-lally just say so.

JonRowe commented 3 years ago

expect( collection ).to any be_present

We already support this via expect(collection).to include(be_present), and we support defining your own aliases.

luke-hill commented 3 years ago

I think you're not seeing the issue, probably because I'm explaining it poorly. I'll give it one more go then just leave it and work around it.

How would you test that inside this "basket" there is at least one even number full stop, or one number greater than 4?

2.7.2 :010 > basket = [pack1,pack2,pack3]
 => [[1, 2, 3], [2, 3, 4], [3, 4, 5]] 

I know you can do include on a collection, but that only iterates, it does not go "into" them, like all does.

class Array; def has_even?; self.any?(&:even?); end; end

2.7.2 :014 > [1,3,5].has_even?
 => false 
2.7.2 :015 > [1,3,5,6].has_even?
 => true 
2.7.2 :016 > expect(basket).to include have_even
Traceback (most recent call last):
        5: from /usr/share/rvm/rubies/ruby-2.7.2/bin/irb:23:in `<main>'
        4: from /usr/share/rvm/rubies/ruby-2.7.2/bin/irb:23:in `load'
        3: from /usr/share/rvm/rubies/ruby-2.7.2/lib/ruby/gems/2.7.0/gems/irb-1.2.6/exe/irb:11:in `<top (required)>'
        2: from (irb):16
        1: from (irb):16:in `include'
TypeError (wrong argument type RSpec::Matchers::BuiltIn::Has (expected Module))
2.7.2 :017 > 2.7.2 :017 > expect(basket).to all have_even
 => true 
JonRowe commented 3 years ago

How would you test that inside this "basket" there is at least one even number full stop, or one number greater than 4?

RSpec::Matchers.alias_matcher :a_collection_matching, :include

RSpec.describe do
  let(:basket) { [[1, 2, 3], [2, 3, 4], [3, 4, 5]] }

  specify do
    expect(basket).to include(a_collection_matching(be_even)).or include(a_collection_matching(be > 4))
  end
end
luke-hill commented 3 years ago

I'll try get this working on my own but thanks for your help. I currently can't get it going in irb, but I'm guessing I need more than just bog-standard rspec. I'll try it inside one of my gem repos.

I'm guessing the reason for the alias matcher is just to improve legibility. They in theory shouldn't need aliasing right?

JonRowe commented 3 years ago

Sorry your changing question threw me, all and include have the same semantics in that they both take a matcher, from your original question I assumed your matchers took in the collection, e.g.

RSpec.describe do
  let(:basket) do
    [
      %i[apple pear],
      %i[apple pear],
      %i[apple pear]
    ]
  end

  specify do
    expect(basket).to all have_apple
  end
end

Produces:

     Failure/Error: expect(basket).to all have_apple

       expected [[:apple, :pear], [:apple, :pear], [:apple, :pear]] to all have apple

          object at index 0 failed to match:
             expected [:apple, :pear] to respond to `has_apple?`

          object at index 1 failed to match:
             expected [:apple, :pear] to respond to `has_apple?`

          object at index 2 failed to match:
             expected [:apple, :pear] to respond to `has_apple?`

However if you implement `have_apple?

RSpec::Matchers.define(:have_apple) do
  match { |actual| actual.include? :apple }
end

Then it works, and so does include and not_to include. So either your custom matcher was doing the work, or you have arrays that are monkey patched with a has_<n> magic method.

Happy to help further, just be mindful I'm likely to not reply for a bit as I have a busy work day ahead of me and RSpec is not my job.

luke-hill commented 3 years ago

There's more than enough here for me to go on. I'm happy with where we've got. Think the "glueing" together of include include wasn't something I thought of. But it makes sense.

luke-hill commented 3 years ago

Coming back to this after xmas, sadly the double include doesn't like working in cucumber tests. Even with using an alias. So I've had to revert back to the original suggestion of

some_apples = basket.any?(&:has_apple?)

expect(some_apples).to be true

I guess it works, but it sucks a little I can't write something more elegant like

expect(basket).to partially have_apple
JonRowe commented 3 years ago

From memory in cucumber the require order is important, as there can be some conflicting methods? Thats out of scope for us but may help!